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

Replace execInWindow with execInThread #6885

Merged
merged 3 commits into from Mar 14, 2017
Merged

Replace execInWindow with execInThread #6885

merged 3 commits into from Mar 14, 2017

Conversation

michaelDCurran
Copy link
Member

Replace nvdaHelperRemote's execInWindow with execInThread, which uses PostThreadMessage rather than PostMessage.
This still allows us to execute code in a thread via the message queue (avoiding Gecko E10s issues with SendMessage) but also still fixes #6422 as the message will still be received even if the window is destroyed.

…sed in a text box in Mozilla applications."

This changes execInWindow to again use PostMessage rather than SendMessageCallback.

This reverts commit 657fb56.
… PostThreadMessage rather than PostMessage. This still allows us to execute code in a thread via the message queue (avoiding Gecko E10s issues with SendMessage) but also still fixes #6422 as the message will still be received even if the window is destroied.
@michaelDCurran michaelDCurran added the p3 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority label Feb 14, 2017
Copy link
Contributor

@jcsteh jcsteh left a comment

Choose a reason for hiding this comment

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

Just a reminder to explain the reasons for this in the commit message when merging to master.

// Wait in a message loop until the callback fires and sends WM_QUIT.
// We also abort the loop if WaitMessage fails for some reason.
while (GetMessage(&msg, NULL, WM_QUIT, WM_QUIT) > 0);
bool execInThread(long threadID, execInThread_funcType func) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment here explaining why we are using this approach instead of SendMessage, since SendMessage does seem more intuitive. It should explain the outgoing cross-process COM call failure and the fact that PostMessage avoids re-entrance (which the code can't handle).

CloseHandle(handles[1]);
return false;
}
if(WaitForMultipleObjects(2,handles,false,INFINITE)!=WAIT_OBJECT_0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful to know specifically that the thread died while waiting (WAIT_OBJECT_0 + 1) as opposed to some other error? Not sure, but thought I'd raise it as food for thought.

@jcsteh
Copy link
Contributor

jcsteh commented Mar 7, 2017

Mozilla bug 1342456 got filed concerning crashes in inProcess_winEventCallback like this one. My memory is a bit vague, but I assume this PR should fix those? (This crash was in a build without this fix.) I just want to comment on the bug accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NVDA lock by pressing enter on the password field in the "Authentication Required" in firefox
3 participants