Skip to content

Replace execInWindow with execInThread#6885

Merged
michaelDCurran merged 3 commits into
masterfrom
execInThread
Mar 14, 2017
Merged

Replace execInWindow with execInThread#6885
michaelDCurran merged 3 commits into
masterfrom
execInThread

Conversation

@michaelDCurran

Copy link
Copy Markdown
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

@jcsteh jcsteh left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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).

Comment thread nvdaHelper/remote/inProcess.cpp Outdated
CloseHandle(handles[1]);
return false;
}
if(WaitForMultipleObjects(2,handles,false,INFINITE)!=WAIT_OBJECT_0) {

Copy link
Copy Markdown
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

jcsteh commented Mar 7, 2017

Copy link
Copy Markdown
Contributor

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