Skip to content

exp/shiny: windriver: AccessDenied for DestroyWindow #25671

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

Closed
ktye opened this issue May 31, 2018 · 7 comments
Closed

exp/shiny: windriver: AccessDenied for DestroyWindow #25671

ktye opened this issue May 31, 2018 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@ktye
Copy link

ktye commented May 31, 2018

What version of Go are you using (go version)?

go version go1.9.2 windows/amd64

Does this issue reproduce with the latest release?

That should be the case.

What operating system and processor architecture are you using (go env)?

windows/amd64

What did you do?

Trying to close a window programmatically by sending a lifecycle event.
I'll send a comment with a link to an example program.

What did you expect to see?

The window to disappear.

What did you see instead?

For the main window this works, but client windows are not destroyed.

Description

This is a follow up to CL 115515.
It's a similar topic but an independent problem.

I tracked it down to shiny/driver/internal/win32/win32.go:121
Release calls DestroyWindow on the handle.
Adding a Printf shows that it returns AccessDenied.
I am not sure about the reason, it might be related to "running on the same thread".

Proposed fix

shiny/driver/internal/win32/win32.go:121
Replace _DestroyWindow(hwnd) with SendMessage(hwnd, _WM_CLOSE, 0, 0)
It works for me.

If you agree that this is the right fix, I'll send a CL.
Should I also remove the TODO's from @andlabs around it? I'm not sure if they are appropriate anymore.

ktye added a commit to ktye/goissues that referenced this issue May 31, 2018
This example demonstrates the bug reported in golang/go#25671
@ktye
Copy link
Author

ktye commented May 31, 2018

To run the example I promised:
go get github.com/ktye/goissues/25671

@andlabs
Copy link
Contributor

andlabs commented May 31, 2018

Is Release being called from another OS thread? DestroyWindow isn't allowed to destroy windows from another OS thread. Sadly it's been too long since I wrote that code, so I have no idea how Shiny works now (I'm surprised it has activity again!) or what the thread structure of your code is. But yes, you can use WM_CLOSE to do this; you can either accept the behavior of DefWindowProc or do it yourself and the TODOs can go away too.

Maybe I should peek back in...

@ktye
Copy link
Author

ktye commented May 31, 2018

@andlabs: it might be.
I create the client window in a goroutine, as it should have it's own event loop:
https://github.com/ktye/goissues/blob/master/25671/main.go#L104

I haven't seen any other shiny examples where multiple windows are handled.
Maybe there are none, as this (and my last fix) have not been reported before.
I came across these issues implementing a shiny backend for mjl-/duit.

@ianlancetaylor
Copy link
Contributor

CC @nigeltao

@ianlancetaylor ianlancetaylor added this to the Unreleased milestone May 31, 2018
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 31, 2018
@alexbrainman
Copy link
Member

I will debug this on weekend.

Alex

ktye added a commit to ktye/duitdraw that referenced this issue Jun 1, 2018
This will work, once golang/go#25671 is resolved.
@alexbrainman
Copy link
Member

Release calls DestroyWindow on the handle.
Adding a Printf shows that it returns AccessDenied.

That error was there all the time.

I am not sure about the reason, it might be related to "running on the same thread".

I am certain you get that error because DestroyWindows is called on a different thread from the message pump thread. I added GetCurrentThreadId to check.

Replace _DestroyWindow(hwnd) with SendMessage(hwnd, _WM_CLOSE, 0, 0)
It works for me.

Sounds good to me.

If you agree that this is the right fix, I'll send a CL.

Please, do.

Should I also remove the TODO's from @andlabs around it? I'm not sure if they are appropriate anymore.

Please, remove whatever TODOs you think are inappropriate. We will make final decision once we review your CL.

Thank you.

Alex

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/115998 mentions this issue: shiny/driver/internal/win32: send WM_CLOSE on Release

@golang golang locked and limited conversation to collaborators Jun 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants