1385014 - Clean up MessageChannel::WaitForSyncNotifyWithA11yReentry once content process message loop no longer uses Win32 message pump

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P1)

Description

[(No longer employed by Mozilla) Aaron Klotz]

It should basically stay as-is in the parent, but in the content process we can s/CoWaitForMultipleHandles/WaitForSingleObjectEx/ with bAlertable = TRUE.

This eliminates some message pumping and other user32 calls that happen under the hood in CoWaitForMultipleHandles.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: s/CoWaitForMultipleHandles/WaitForSingleObjectEx/

Since we only use MessageChannel::WaitForSyncNotifyWithA11yReentry in content, I just did a wholesale swap of waiting functions without adding separate code paths for parent vs content.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8890997 [details] [diff] [review]
s/CoWaitForMultipleHandles/WaitForSingleObjectEx/

Review of attachment 8890997 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not competent to review this code. Is there a difference between the two versions? They seem equivalent based on the MSDN documentation. Why is it only okay to do this once we stop using the Win32 message pump?

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Bill McCloskey (:billm) from comment #2)
> Comment on attachment 8890997 [details] [diff] [review]
> I'm not competent to review this code. Is there a difference between the two
> versions? They seem equivalent based on the MSDN documentation. Why is it
> only okay to do this once we stop using the Win32 message pump?

CoWaitForMultipleHandles does message pumping under the hood to service STA COM. WaitForSingleObjectEx does not.

Since we are getting rid of the message pump and the sandbox is tightening, the likelihood of being deadlocked on COM messages becomes nil. Therefore we can cut out the extra COM and messaging goop and cut straight to waiting on the event.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Just updating this bug a bit since it showed up on my Bugzilla dashboard.

Attachment #8890997 [details] [diff] would work once Win32k lockdown was enabled. It must land simultaneiously with the enabling of lockdown, though.

Given that now everything needs to go through Phabricator and Lando, I think that the best way forward for this bug would be to change this patch to use two code paths (one for with lockdown, one for without), and automagically select the right one depending on the actual Win32k lockdown state.

At some point I will update this patch to do so, and then push it to Phabricator.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:aklotz, could you have a look please?

Comment 6

[Bob Owen (:bobowen) PTO until 8th Oct]

aklotz - now we have an at least partly usable browser with win32k lockdown enabled, we can see these failing. Do you have an idea as to what type of thing we should see broken as a result of that?

Presumably we can land a version of your patch that switches on IsWin32kLockedDown between the current code and your patch.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Bob Owen (:bobowen) from comment #6)

aklotz - now we have an at least partly usable browser with win32k lockdown enabled, we can see these failing. Do you have an idea as to what type of thing we should see broken as a result of that?

IIRC this method is used in content for synchronous messages, so you'll probably see broken sync IPC calls from child to parent. A11y is probably one area where this would be apparent.

Presumably we can land a version of your patch that switches on IsWin32kLockedDown between the current code and your patch.

That's probably the best idea, yeah. Would you like me to prepare that, or are you going to handle it?

Comment 8

[Bob Owen (:bobowen) PTO until 8th Oct]

Attachment: Bug 1385014: Use WaitForSingleObjectEx in WaitForSyncNotifyWithA11yReentry when win32k is disabled. r=aklotz!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87a2ebdf7dfe
Use WaitForSingleObjectEx in WaitForSyncNotifyWithA11yReentry when win32k is disabled. r=aklotz

Comment 10

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/87a2ebdf7dfe