Closed Bug 1273635 Opened 4 years ago Closed 3 years ago

Mitigate potential a11y deadlocks due to COM calls originating from chrome process

Categories

(Core :: Disability Access APIs, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

(Blocks 1 open bug)

Details

(Whiteboard: aes+)

Attachments

(2 files, 1 obsolete file)

tbsaunde pointed out a potential deadlock scenario with a11y clients that have injected into our parent process.

If the client's injected DLL makes a synchronous COM call into the content process, but the content process is waiting on a sync IPDL message that is enqueued in the parent, the COM call will never make it onto the main thread.

I've put a lot of thought into this, and I think that the best way to take care of this is to modify the wait functions for both the main message loop and sync IPC in content to do an alertable wait.

Any a11y requests coming in on COM MTA background threads will be routed to the main thread via QueueUserAPC(), which can be roughly considered equivalent to (but much safer than) UNIX signals.

I have already built a POC locally that makes everything work. IMHO this is not much different than dispatching a11y calls in GetMessage, other than the fact that APCs are much more performant.

Currently my implementation sets the alertable stuff when ACCESSIBILITY is defined and we're in a content process. I might further refine this code to only do it when a11y is actually active.
Attached patch WIP (obsolete) — Splinter Review
Whiteboard: aes-win
Whiteboard: aes-win → aes+
Depends on: 1287002
Comment on attachment 8772180 [details]
Bug 1273635: Enable alertable waits in content process main thread;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65056/diff/1-2/
Comment on attachment 8772180 [details]
Bug 1273635: Enable alertable waits in content process main thread;

https://reviewboard.mozilla.org/r/65056/#review62932
Attachment #8772180 - Flags: review?(jmathies) → review+
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/48d06bcb9c65
Enable alertable waits in content process main thread; r=jimm
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d97ff06ef63
Backed out changeset 48d06bcb9c65 for Mochitest failures
I have backed this push out for Mochitest failures, e.g. Windows XP Mochitest failures from your push, https://treeherder.mozilla.org/logviewer.html#?job_id=1055273&repo=autoland#L3104
Please help to check the issues, thanks.
Flags: needinfo?(aklotz)
CoWaitForMultipleHandles in the child process IPC channel wait is failing with E_ACCESSDENIED on XP.

Why? We don't know. I can probably change this to just use WaitForMultipleObjectsEx instead.
Flags: needinfo?(aklotz)
Comment on attachment 8772180 [details]
Bug 1273635: Enable alertable waits in content process main thread;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65056/diff/2-3/
Try is taking > 48 hours. I disabled Windows XP for now.
Comment on attachment 8772180 [details]
Bug 1273635: Enable alertable waits in content process main thread;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65056/diff/3-4/
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0372c7296be3
Enable alertable waits in content process main thread; r=jimm
https://hg.mozilla.org/mozilla-central/rev/0372c7296be3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1293185
Bug 1293480 appears to have started with this patch's initial landing to autoland.
Depends on: 1293480
Depends on: 1293847
Blocks: 1294206
I mistakenly missed the fact that MessageChannel uses a manual-reset event. This patch brings WaitForSyncNotifyWithA11yRentry into sync with the other WaitFor* functions with respect to resetting the event and handling errors from the wait function.
Attachment #8753933 - Attachment is obsolete: true
Attachment #8781202 - Flags: review?(wmccloskey)
Attachment #8781202 - Flags: review?(jmathies)
Comment on attachment 8781202 [details] [diff] [review]
Follow-up - reset the channel's event

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

Could you please put an assertion somewhere that REQUIRE_A11Y_REENTRY and REQUIRE_DEFERRED_MESSAGE_PROTECTION aren't both set?

Also, does the a11y COM stuff ever cause a Win32 window in the child process to be created and somehow have its event loop attached to the main process?
Attachment #8781202 - Flags: review?(wmccloskey) → review+
Attachment #8781202 - Flags: review?(jmathies) → review+
(In reply to Bill McCloskey (:billm) from comment #17)
> Comment on attachment 8781202 [details] [diff] [review]
> Follow-up - reset the channel's event
> 
> Review of attachment 8781202 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could you please put an assertion somewhere that REQUIRE_A11Y_REENTRY and
> REQUIRE_DEFERRED_MESSAGE_PROTECTION aren't both set?

Will do.

> 
> Also, does the a11y COM stuff ever cause a Win32 window in the child process
> to be created and somehow have its event loop attached to the main process?

There already is a window by virtue of the fact that we've always initialized our main thread in the single-threaded apartment. But it is not attached to anything.
Also, I specifically designed the a11y code to avoid sending a11y IPC through the Windows message pump. We're using the multithreaded apartment to send stuff to content background threads using MSRPC, and then forwarding those requests to the main thread via asynchronous procedure calls (hence the need for alertable waiting).
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e32b3372e2309fa7dc6b00f1a9e2fcf3ea763fc
Bug 1273635: Follow-up - ensure that MessageChannel's event is reset when signalled; r=jimm, billm
this improves talos for damp on win7 by 3.28%:
https://treeherder.mozilla.org/perf.html#/alerts?id=2509
Blocks: 1301167
You need to log in before you can comment on or make changes to this bug.