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

fix: com error handling #25

Merged
merged 5 commits into from
Dec 4, 2024
Merged

fix: com error handling #25

merged 5 commits into from
Dec 4, 2024

Conversation

Snshadow
Copy link
Contributor

@Snshadow Snshadow commented Dec 3, 2024

  • fix win32.GetClientRect error handling following the semantic
  • fix com error handling as the last status maybe set to an error value in previous Windows versions

- fix win32.GetClientRect error handling following the semantic
- fix com error handling as the last status maybe set to an error value in previous Windows versions
@Snshadow
Copy link
Contributor Author

Snshadow commented Dec 3, 2024

As the AddRef and Release return a reference count as a uint32 value, I tried changing them to return uint32 value instead of an error(as internally used last status value could be handled inappropriately.) However, I realized that changing the whole AddRef and Release function to return uint32 makes wails build fail as it expects error value returning from Release function. I have reverted the part that causes the build to fail to return an error,

  • // AddRef increments the reference count of ICoreWebView2Deferral interface
    func (i *ICoreWebView2Deferral) AddRef() error {
    _, _, err := i.Vtbl.AddRef.Call(uintptr(unsafe.Pointer(i)))
    if err != nil && !errors.Is(err, windows.ERROR_SUCCESS) {
    return err
    }
    return nil
    }
    // Release decrements the reference count of ICoreWebView2Deferral interface
    func (i *ICoreWebView2Deferral) Release() error {
    _, _, err := i.Vtbl.AddRef.Call(uintptr(unsafe.Pointer(i)))
    if err != nil && !errors.Is(err, windows.ERROR_SUCCESS) {
    return err
    }
    return nil
    }
  • func (i *ICoreWebView2WebResourceResponse) AddRef() error {
    _, _, err := i.vtbl.AddRef.Call(uintptr(unsafe.Pointer(i)))
    if err != nil && !errors.Is(err, windows.ERROR_SUCCESS) {
    return err
    }
    return nil
    }
    func (i *ICoreWebView2WebResourceResponse) Release() error {
    _, _, err := i.vtbl.Release.Call(uintptr(unsafe.Pointer(i)))
    if err != nil && !errors.Is(err, windows.ERROR_SUCCESS) {
    return err
    }
    return nil
    }
  • func (i *IStream) AddRef() error {
    _, _, err := i.vtbl.AddRef.Call(uintptr(unsafe.Pointer(i)))
    if err != nil && !errors.Is(err, windows.ERROR_SUCCESS) {
    return err
    }
    return nil
    }
    func (i *IStream) Release() error {
    _, _, err := i.vtbl.Release.Call(uintptr(unsafe.Pointer(i)))
    if err != nil && !errors.Is(err, windows.ERROR_SUCCESS) {
    return err
    }
    return nil
    }

    but it would need to be handled correctly.

pkg/edge/chromium.go Outdated Show resolved Hide resolved
@Snshadow
Copy link
Contributor Author

Snshadow commented Dec 3, 2024

Some errors were accidentally changed to return nil instead of an error. Fixed them with the latest commit.

@leaanthony
Copy link
Member

Thanks for taking the time to do this - a much needed improvement. I think we'll figure out what to do with the 3 functions above in a separate PR 👍

@leaanthony leaanthony merged commit bbb67a3 into wailsapp:main Dec 4, 2024
@Snshadow Snshadow deleted the fix-com-call branch December 5, 2024 00:09
@Snshadow
Copy link
Contributor Author

Snshadow commented Dec 5, 2024

Thanks for taking the time to do this - a much needed improvement. I think we'll figure out what to do with the 3 functions above in a separate PR 👍

I have create an issue in wails for follow up.

@@ -293,13 +473,10 @@ func (e *ICoreWebView2Environment) CreateWebResourceResponse(content []byte, sta
}

if response == nil {
if err == nil {
Copy link
Collaborator

@stffabi stffabi Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we remove this?

I mean isn't it better to have at least GetLastError (which might give us more information) returned instead of always unknown error.

Copy link
Contributor Author

@Snshadow Snshadow Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But are GetLastError values guaranteed to contain meaningful data(not to be misleading if unrelated function had call SetLastError and not return it to ERROR_SUCCESS)? Or could it be used just to get indirect gleam of what happened? It thats the case.. I will have to get back all err return value from Proc.Call.. 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leaanthony Any thoughts?

Copy link
Collaborator

@stffabi stffabi Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, those are the same thoughts I had during a fix of the error handling in that function. It's not clear that GetLastError might be helpful or not, that really depends on what's going on behind the scenes which is completely out of our control.

Maybe we could include it has a hint. return fmt.Errorf("uknown error: hint=%w", err)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That all goes into the direction what I brought up in the other issue: wailsapp/wails#3931 (comment)

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.

We should probably really start a discussion how to handle errors depending on the method being called. E.g. check pointer to Ole-Objects returned to detect if call was successful, and probably other checks, what we should do in the case above...

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

Successfully merging this pull request may close these issues.

3 participants