Closed Bug 1385014 Opened 7 years ago Closed 4 years ago

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

Categories

(Core :: IPC, enhancement, P1)

Product:
Core
Component:
IPC
Platform:
Unspecified
Windows
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
90 Branch
Tracking Flags:
Tracking Status
firefox90 --- fixed

People

(Reporter: bugzilla, Assigned: bobowen)

References

Details

(Whiteboard: [changes need to land with win32k switch over])

Attachments

(1 file, 1 obsolete file)

Bug 1385014: Use WaitForSingleObjectEx in WaitForSyncNotifyWithA11yReentry when win32k is disabled. r=aklotz!
(deleted), text/x-phabricator-request
Details
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.
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.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8890997 - Flags: review?(wmccloskey)
Attachment #8890997 - Flags: review?(jmathies)
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?
Attachment #8890997 - Flags: review?(wmccloskey)
(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.
Priority: -- → P2
Whiteboard: sb+
Attachment #8890997 - Flags: review?(jmathies) → review+
Depends on: 1423628

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.

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?

Flags: needinfo?(aklotz)
Flags: needinfo?(aklotz)
Status: ASSIGNED → NEW
Whiteboard: sb+ → [changes need to land with win32k switch over]

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.

Flags: needinfo?(aklotz)

(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?

Flags: needinfo?(aklotz)
Assignee: aklotz → bobowencode
Status: NEW → ASSIGNED
Priority: P2 → P1
Pushed by bobowencode@gmail.com: https://hg.mozilla.org/integration/autoland/rev/87a2ebdf7dfe Use WaitForSingleObjectEx in WaitForSyncNotifyWithA11yReentry when win32k is disabled. r=aklotz

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox90: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
Attachment #8890997 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: