Replace execInWindow with execInThread #6885

Merged
merged 3 commits into from Mar 14, 2017

Conversation

Projects
None yet
3 participants
@michaelDCurran
Contributor

michaelDCurran commented Feb 14, 2017

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.

michaelDCurran added some commits Feb 14, 2017

Revert "Fix freeze when pressing enter to dismiss a dialog while focu…
…sed in a text box in Mozilla applications."

This changes execInWindow to again use PostMessage rather than SendMessageCallback.

This reverts commit 657fb56.
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 destroied.

@michaelDCurran michaelDCurran requested a review from jcsteh Feb 14, 2017

@michaelDCurran michaelDCurran added the p2 label Feb 14, 2017

@jcsteh

jcsteh approved these changes Feb 15, 2017

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

nvdaHelper/remote/inProcess.cpp
- // 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) {

This comment has been minimized.

@jcsteh

jcsteh Feb 15, 2017

Contributor

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

@jcsteh

jcsteh Feb 15, 2017

Contributor

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

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

This comment has been minimized.

@jcsteh

jcsteh Feb 15, 2017

Contributor

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 Feb 15, 2017

Contributor

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.

michaelDCurran added a commit that referenced this pull request Feb 15, 2017

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Mar 7, 2017

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.

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