Skip to content

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

Merged
jcsteh merged 3 commits into
masterfrom
vbufPostMessage
Nov 14, 2016
Merged

nvdaHelperRemote: Use SendMessageCallback instead of SendMessage to execute most cross-thread calls.#6380
jcsteh merged 3 commits into
masterfrom
vbufPostMessage

Conversation

@jcsteh

@jcsteh jcsteh commented Sep 16, 2016

Copy link
Copy Markdown
Contributor

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.

…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

jcsteh commented Sep 16, 2016

Copy link
Copy Markdown
Contributor Author

@michaelDCurran, can you please review? Thanks!

…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 Use PostMessage and an event instead of SendMessage to initialise virtual buffer backend render threads. nvdaHelperRemote: Use PostMessage and an event instead of SendMessage to execute most cross-thread calls. Sep 21, 2016
@jcsteh

jcsteh commented Sep 21, 2016

Copy link
Copy Markdown
Contributor Author

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

jcsteh commented Sep 22, 2016 via email

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown

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

jcsteh commented Sep 22, 2016

Copy link
Copy Markdown
Contributor Author

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

… 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

jcsteh commented Oct 5, 2016

Copy link
Copy Markdown
Contributor Author

@michaelDCurran, could you please review this again?

@michaelDCurran

Copy link
Copy Markdown
Member

Fine with me.

jcsteh added a commit that referenced this pull request Oct 5, 2016
@jcsteh jcsteh changed the title nvdaHelperRemote: Use PostMessage and an event instead of SendMessage to execute most cross-thread calls. 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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants