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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: WIP

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1273635: Enable alertable waits in content process main thread;

Review commit: https://reviewboard.mozilla.org/r/65056/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65056/

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

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 4

[Jim Mathies [:jimm]]

Comment on attachment 8772180 [details]
Bug 1273635: Enable alertable waits in content process main thread;

https://reviewboard.mozilla.org/r/65056/#review62932

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48d06bcb9c65
Enable alertable waits in content process main thread; r=jimm

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d97ff06ef63
Backed out changeset 48d06bcb9c65 for Mochitest failures

Comment 7

[Iris Hsiao [:ihsiao]]

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.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

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/

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Try is taking > 48 hours. I disabled Windows XP for now.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

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/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0372c7296be3
Enable alertable waits in content process main thread; r=jimm

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0372c7296be3

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Bug 1293480 appears to have started with this patch's initial landing to autoland.

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Follow-up - reset the channel's event

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.

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e1b1bab46e9

Comment 17

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

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?

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d210b682f07a
This is fixing the talos problem.

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

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).

Comment 21

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

OK, cool.

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

this improves talos for damp on win7 by 3.28%:
https://treeherder.mozilla.org/perf.html#/alerts?id=2509

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0e32b3372e23