wailsapp
Improve go-webview2 com object release
This is a followup from https://github.com/wailsapp/go-webview2/pull/25#issuecomment-2513894748 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.
- https://github.com/wailsapp/wails/blob/6345b64a2241804389778d363ddee27076662f2a/v2/pkg/assetserver/webview/request_windows.go#L125-L127
- https://github.com/wailsapp/wails/blob/6345b64a2241804389778d363ddee27076662f2a/v2/pkg/assetserver/webview/request_windows.go#L135-L137
- https://github.com/wailsapp/wails/blob/6345b64a2241804389778d363ddee27076662f2a/v2/pkg/assetserver/webview/request_windows.go#L155-L161
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).
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?
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.
@Snshadow - given we can create a new v2 version of this library, what are your recommendations? Shall we just copy what's there for now and make the additional breaking changes above?
I think that we could leave the Addref() and Release() return an error as it were before, we could check the reference count of a object then return an error if the functions are called on a object which reference count has reached zero after usage. Looking the comment from go-webview2 and the implementation of IUnknown::Release() in the combridge, the idea of tracking the reference count of the object when calling Addref() or Release() exists there. If we were to use the combridge for handling COM object, there are some things that we should be aware of,
- Instead of panicking if Release if called on destroyed object, it should do error handling(logging or something else?)
func (c *comObject) release() int32 {
c.l.Lock()
defer c.l.Unlock()
if c.refCount <= 0 {
panic("call on destroyed com object")
}
...
}
- As there are bunch of codes that are already written to handle COM objects, doing this would make a lot of work.
Instead, we could add a refCount field in the COM object structs of keep track of reference count when calling AddRef or Release after QueryInterface to prevent any undefined behaviours(exception, crash, ...), then increment the refCount value with refCount++ in QueryInterface() or other COM object creation functions such as CreateWebResourceResponse that can fill in the interface specific functions.
e.g
type _ICoreWebView2WebResourceResponseVtbl struct {
_IUnknownVtbl
GetContent ComProc
PutContent ComProc
GetHeaders ComProc
GetStatusCode ComProc
PutStatusCode ComProc
GetReasonPhrase ComProc
PutReasonPhrase ComProc
refCount uint32
}
func (i *ICoreWebView2WebResourceResponse) AddRef() error {
if i.refCount <= 0 {
return fmt.Errorf("called Addref() on object with reference count zero")
}
v, _, _ := i.vtbl.AddRef.Call(uintptr(unsafe.Pointer(i)))
_ = v // do something with this value for debugging/testing purpose?
i.refCount++
return nil
}
func (i *ICoreWebView2WebResourceResponse) Release() error {
if i.refCount <= 0 {
return fmt.Errorf("called Release() on object with reference count zero")
}
v, _, _ := i.vtbl.Release.Call(uintptr(unsafe.Pointer(i)))
_ = v // do something with this value for debugging/testing purpose?
i.refCount--
return nil
}
The return value of AddRef or Release can be used only in some situations as https://learn.microsoft.com/en-us/windows/win32/com/rules-for-managing-reference-counts states that
In some situations, the return values of AddRef and Release may be unstable and should not be relied upon; they should be used only for debugging or diagnostic purposes.
Next, for the function that could return windows.S_OK but return an unexpected output like this, instead of just returning unknown error by ignoring the lasterror value, we could show hint as @stffabi said in https://github.com/wailsapp/go-webview2/pull/25#discussion_r1870882489.
I wonder if this is possible by changing the code generator.
Please be aware of the differences between the combridge and the other com code. The combridge is used to implement com-objects in go, there we implement com-interfaces in go, that get passed to com. So the real com-object is managed in Go. The combridge is currently only used in rare cases for bootstrapping the webview2, where we need to create and pass a com-object to webview2.
Whereas e.g. ICoreWebView2WebResourceResponse the real com object is in native code outside of the Go runtime, those object are created by Webview2 and we only have a thin wrapper to call the native implementation. So in the ICoreWebView2WebResourceResponse case the refCount tracking needs to be done by the real object and we shouldn't do that somehow in our wrapper.
I would refrain from doing a refCounting somehow on our wrappers, those will never be in sync with the real one of the object. So people might see a refCount=0 when debugging, but still the real object on the native side might have a refCount>0 and still consume memory. We should only be a thin wrapper between Go and the real com-interface so we can call methods on the com object not more and not less. As per definition of IUnknown AddRef and Release can never return an error, so we should either remove the error return from the method definition or simply always return nil.
As for the panic you mentioning for func (c *comObject) release() int32 { if that happens, there is something really strange going on. That would mean some native code still has a pointer to a com-object that has already been freed. So there's no guarantee what is going on, the pointer that the native code used to call release() might now point to something new. As a result the program is already going a nondeterministic path and calling potentially unsafe memory locations. So personally I prefer a panic if we can detect memory corruption instead of silently ignoring it and have a nondeterministic program execution. Sure it is not guaranteed we can detect the corruption and panic, but at least if know memory corruption is going on, we should abort the program
@stffabi Oh, there are lots of things that were going on behind the scene.., thank you for the explanation! I have seen some cases when calling Release() on a already destroyed object result in a crash with something like
Exception 0xc0000005 0x1 0x150 0x7ffbbc5857eb
PC=0x7ffbbc5857eb
which indicates an access violation when I accidentally called Release after ole.VARIANT.Clear() while using the go-ole package which calls VariantClear as it calls Release for VT_DISPATCH(IDispatch) object which inherits IUnknown interface,
however, as wails and go-webview2 implements the use of COM object itself, the checking the reference count would not be necessary if AddRef() and Release() are used with care.