nvdaHelperRemote: Use SendMessageCallback instead of SendMessage to execute most cross-thread calls. #6380

Merged
merged 3 commits into from Nov 14, 2016

Conversation

Projects
None yet
4 participants
@jcsteh
Contributor

jcsteh commented Sep 16, 2016

This is necessary for Firefox multi-process. When we use SendMessage to execute calls in the main thread, outgoing cross-process COM calls fail with RPC_E_CANTCALLOUT_ININPUTSYNCCALL; see Mozilla bug 1297549 comment 14. We avoid this by instead using PostMessage and waiting on an event.
Even though this is only relevant to Firefox multi-process right now, this applies to any case where we might call a COM proxy in a cross-thread call.
execInWindow now uses SendMessageCallback. Virtual buffer backends now use execInWindow to initialize/terminate the render thread instead of their own message/hook.

Use PostMessage and an event instead of SendMessage to initialise vir…
…tual buffer backend render threads.

This is necessary for Firefox multi-process. When we use SendMessage to do this, outgoing cross-process COM calls fail with RPC_E_CANTCALLOUT_ININPUTSYNCCALL; see Mozilla bug 1297549 comment 14. We avoid this by instead using PostMessage and waiting on an event.
@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Sep 16, 2016

Contributor

@michaelDCurran, can you please review? Thanks!

Contributor

jcsteh commented Sep 16, 2016

@michaelDCurran, can you please review? Thanks!

Also use PostMessage and an event instead of SendMessage for execInWi…
…ndow, which is used for the in-process Mozilla text code.

Fixes problems with editable text fields in Firefox multi-process.

@jcsteh jcsteh changed the title from Use PostMessage and an event instead of SendMessage to initialise virtual buffer backend render threads. to nvdaHelperRemote: Use PostMessage and an event instead of SendMessage to execute most cross-thread calls. Sep 21, 2016

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Sep 21, 2016

Contributor

@michaelDCurran, I updated this to also do this for execInWindow. I've changed the title and body of the PR accordingly.

Contributor

jcsteh commented Sep 21, 2016

@michaelDCurran, I updated this to also do this for execInWindow. I've changed the title and body of the PR accordingly.

nvdaHelper/vbufBase/backend.cpp
+ this->renderThread_initialize();
+ };
+ LOG_DEBUG(L"Calling execInWindow");
+ execInWindow((HWND)UlongToHandle(rootDocHandle),func);

This comment has been minimized.

@michaelDCurran

michaelDCurran Sep 22, 2016

Contributor

I'm a little unsure if this is completely safe as it is a call from one instance of the CRT to another, using a c++ class or specific feature. I really have no evidence that it won't work, but just something we should keep an eye on in various operating systems. For instance, we avoided ever passing c++ strings for this reason. At worse

@michaelDCurran

michaelDCurran Sep 22, 2016

Contributor

I'm a little unsure if this is completely safe as it is a call from one instance of the CRT to another, using a c++ class or specific feature. I really have no evidence that it won't work, but just something we should keep an eye on in various operating systems. For instance, we avoided ever passing c++ strings for this reason. At worse

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Sep 22, 2016

Contributor
Contributor

jcsteh commented Sep 22, 2016

jcsteh added a commit that referenced this pull request Sep 22, 2016

@Brian1Gaff

This comment has been minimized.

Show comment
Hide comment
@Brian1Gaff

Brian1Gaff Sep 22, 2016

Would this affect anything else using IE code such as Outlook Express etc?
Brian

bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal email to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
----- Original Message -----
From: "James Teh" notifications@github.com
To: "nvaccess/nvda" nvda@noreply.github.com
Sent: Thursday, September 22, 2016 3:56 AM
Subject: Re: [nvaccess/nvda] nvdaHelperRemote: Use PostMessage and an event
instead of SendMessage to execute most cross-thread calls. (#6380)

As discussed, I tested this with Firefox, Chrome and IE in Windows 10,
as well as Firefox and IE in Windows XP, and did not encounter any
crashes or other instability. Hopefully, this multiple CRT thing won't
be an issue, but if it is, it should get picked up during incubation.

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#6380 (comment)

Would this affect anything else using IE code such as Outlook Express etc?
Brian

bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal email to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
----- Original Message -----
From: "James Teh" notifications@github.com
To: "nvaccess/nvda" nvda@noreply.github.com
Sent: Thursday, September 22, 2016 3:56 AM
Subject: Re: [nvaccess/nvda] nvdaHelperRemote: Use PostMessage and an event
instead of SendMessage to execute most cross-thread calls. (#6380)

As discussed, I tested this with Firefox, Chrome and IE in Windows 10,
as well as Firefox and IE in Windows XP, and did not encounter any
crashes or other instability. Hopefully, this multiple CRT thing won't
be an issue, but if it is, it should get picked up during incubation.

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#6380 (comment)

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Sep 22, 2016

Contributor

I don't anticipate that it will break anything, but yes, this new code does get used in anything that uses IE.

Contributor

jcsteh commented Sep 22, 2016

I don't anticipate that it will break anything, but yes, this new code does get used in anything that uses IE.

Fix freeze when pressing enter to dismiss a dialog while focused in a…
… text box in Mozilla applications.

In this case, the window dies before the message is sent, but the return value of PostMessage wasn't being checked. Therefore, we were waiting forever for an event that would never be set.
However, it's also possible that the thread might die after the message is sent but before it is handled. Therefore, switch to using SendMessageCallback, which calls the callback with a 0 result if the thread dies.
This also Fixes a leak, since event handles in the previous code weren't being closed.
Fixes #6422.
@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Oct 5, 2016

Contributor

@michaelDCurran, could you please review this again?

Contributor

jcsteh commented Oct 5, 2016

@michaelDCurran, could you please review this again?

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Oct 5, 2016

Contributor

Fine with me.

Contributor

michaelDCurran commented Oct 5, 2016

Fine with me.

jcsteh added a commit that referenced this pull request Oct 5, 2016

@jcsteh jcsteh changed the title from nvdaHelperRemote: Use PostMessage and an event instead of SendMessage to execute most cross-thread calls. to nvdaHelperRemote: Use SendMessageCallback instead of SendMessage to execute most cross-thread calls. Nov 14, 2016

@jcsteh jcsteh merged commit b1a0516 into master Nov 14, 2016

@nvaccessAuto nvaccessAuto added this to the 2016.4 milestone Nov 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment