Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve go-webview2 com object release #3931

Open
Snshadow opened this issue Dec 5, 2024 · 2 comments
Open

Improve go-webview2 com object release #3931

Snshadow opened this issue Dec 5, 2024 · 2 comments

Comments

@Snshadow
Copy link

Snshadow commented Dec 5, 2024

This is a followup from wailsapp/go-webview2#25 (comment) which couldn't be fixed properly as wails build fails in Windows if it were changed.

The cause of the build failure is due to these codes that expects error return from Release function which only returns reference count as a uint32 value, not an error, which can cause unexpected erroring as Proc.Call states that

The returned error is always non-nil, constructed from the result of GetLastError. Callers must inspect the primary return value to decide whether an error occurred (according to the semantics of the specific function being called) before consulting the error. The error always has type Errno.

These parts expect Release to return an error which prevent building from succeeding.

If this were to be fixed, wails and go-webview2 would need to be fixed at the same time(as wails depends on go-webview2).

@stffabi
Copy link
Collaborator

stffabi commented Dec 5, 2024

The reference count returned from AddRef and Release is almost never really of interest.

Microsoft also states https://learn.microsoft.com/en-us/windows/win32/api/unknwn/nf-unknwn-iunknown-release#return-value

The method returns the new reference count. This value is intended to be used only for test purposes.

So I personally would not change the signature to return uint32, which would basically then always be ignored in the Wails code. Also changing the interface means we should probably bump go-webview2 to v2 so people won't end up with a brocken Wails application by the means of just calling a go get upgrade dependencies.

We should probably just leave the interface (and change the implementation to return nil in go-webview2) as it is so we don't break all the Wails v2 apps out there. Then clearly define a set of rules how errors should be handled and detected for com call and afterwards bring go-webview2 as v2 up to that and use it in Wails v3.

@leaanthony any thoughts?

@leaanthony
Copy link
Member

leaanthony commented Dec 5, 2024

Agree with you @stffabi . We don't want past decisions to hold us back so let's create a v2 directory and create what we think is the right API in there. I started looking at the composition controller too and realised that the main components of the package should probably change too.

The code generator needs to be updated to output the better error handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants