1390652 - Level 3 sandbox breaks IAccessible::get_accParent on top-level documents

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Seeing a lot of E_ACCESSDENIED errors popping up. They only happen on sandbox level 3.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

From what I am seeing, the reason why this is happening is because the proxy to the parent is an IAccessible, but get_accParent provides IDispatch.

When COM goes to re-marshal the parent proxy, it sees this disparity and tries to do a remote QI to retrieve the IDispatch interface (it doesn't understand that IAccessible derives from IDispatch).

I think I might be able to work around this by sending the parent COM proxy down to content as an IDispatch instead of an IAccessible.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

I've written a patch but I'm still seeing failures. I'm going to have to do some more debugging to see why this is still failing.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

I eliminated the QI, but it's still try to do a RemoteAddRef. Which makes sense but is unfortunate ;-)

More work is needed...

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Send parent accessibles as IDispatch instead of IAccessible

See the comments in the changes to PDocAccessible.ipdl for explanation.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Enable MSHLFLAGS_TABLESTRONG in ProxyStream

Since we're already preserving the marshaled stream, we now meet the prerequisite for being able to pass MSHLFLAGS_TABLESTRONG to CoMarshalInterface.

This scheme handles reference counting of proxies differently from MSHLFLAGS_NORMAL: content doesn't need to attempt to adjust the object's reference count every time the proxy marshaled out.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Enable MSHLFLAGS_TABLESTRONG in ProxyStream (r2)

Rebased

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Enable MSHLFLAGS_TABLESTRONG in ProxyStream (r3)

Comment cleanup

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Enable MSHLFLAGS_TABLESTRONG in ProxyStream (r4)

Caught a typo. Sorry!

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8898047 [details] [diff] [review]
Part 1: Send parent accessibles as IDispatch instead of IAccessible

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

nice. thanks

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8898054 [details] [diff] [review]
Part 2: Enable MSHLFLAGS_TABLESTRONG in ProxyStream (r4)

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

nice.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8977bc6f3cbc2e9d7bbf612fd6b42d367117cd2a
Bug 1390652: Part 1 - Send parent COM proxies to content as IDispatch instead of IAccessible to match the outparam type of IAccessible::get_accParent; r=yzen

https://hg.mozilla.org/integration/mozilla-inbound/rev/d932e4e9c4a1b3470d4749904449f0d472d8b144
Bug 1390652: Part 2 - Add support for TABLESTRONG marshaling to mscom::ProxyStream; r=jimm

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing a11y's accessible/tests/mochitest/jsat/test_content_integration.html on Windows debug (after asserting):

https://hg.mozilla.org/integration/mozilla-inbound/rev/5be6a158d6b616d287bcda770bbf82e8068f045e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3c98277c1e45eda2e3b0fa0e3b317ff82a76dc

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d932e4e9c4a1b3470d4749904449f0d472d8b144&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=124218972&repo=mozilla-inbound

19:51:44     INFO -  294 INFO None295 INFO TEST-START | accessible/tests/mochitest/jsat/test_content_integration.html
19:51:44     INFO -  GECKO(4576) | ++DOMWINDOW == 66 (000001FC189D0800) [pid = 4576] [serial = 309] [outer = 000001FC26441800]
19:51:44     INFO -  GECKO(4576) | ++DOCSHELL 000001FC18BE3000 == 11 [pid = 4576] [id = {bdfc5ce8-a615-445c-a2e5-38beecc04ddd}]
19:51:44     INFO -  GECKO(4576) | ++DOMWINDOW == 67 (000001FC18BE3800) [pid = 4576] [serial = 310] [outer = 0000000000000000]
19:51:44     INFO -  GECKO(4576) | ++DOMWINDOW == 68 (000001FC18BE7800) [pid = 4576] [serial = 311] [outer = 000001FC18BE3800]
19:51:44     INFO -  GECKO(4576) | ++DOCSHELL 000001FC1A016800 == 12 [pid = 4576] [id = {946b1b58-1b6d-44f2-b7e0-43c70495079b}]
19:51:44     INFO -  GECKO(4576) | ++DOMWINDOW == 69 (000001FC1A02C000) [pid = 4576] [serial = 312] [outer = 0000000000000000]
19:51:44     INFO -  GECKO(4576) | ++DOCSHELL 000001FC1D32B000 == 13 [pid = 4576] [id = {b3e54d7b-128e-48cf-81e8-bc0bd698ff91}]
19:51:44     INFO -  GECKO(4576) | ++DOMWINDOW == 70 (000001FC1D4A6000) [pid = 4576] [serial = 313] [outer = 0000000000000000]
19:51:44     INFO -  GECKO(4576) | ++DOMWINDOW == 71 (000001FC1DC07000) [pid = 4576] [serial = 314] [outer = 000001FC1D4A6000]
19:51:44     INFO -  GECKO(4576) | ++DOMWINDOW == 72 (000001FC1D4A9000) [pid = 4576] [serial = 315] [outer = 000001FC1A02C000]
19:51:44     INFO -  GECKO(4576) | ++DOMWINDOW == 73 (000001FC1DF6E000) [pid = 4576] [serial = 316] [outer = 000001FC1D4A6000]
19:51:44     INFO -  GECKO(4576) | WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
19:51:44     INFO -  GECKO(4576) | pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14
19:51:44     INFO -  GECKO(4576) | windowPrivacyMatches@chrome://browser/content/browser-sidebar.js:194:12
19:51:44     INFO -  GECKO(4576) | startDelayedLoad@chrome://browser/content/browser-sidebar.js:207:10
19:51:44     INFO -  GECKO(4576) | _delayedStartup/<@chrome://browser/content/browser.js:1664:7
19:51:44     INFO -  GECKO(4576) | promise callback*_delayedStartup@chrome://browser/content/browser.js:1655:5
19:51:44     INFO -  GECKO(4576) | EventListener.handleEvent*onLoad@chrome://browser/content/browser.js:1381:5
19:51:44     INFO -  GECKO(4576) | onload@chrome://browser/content/browser.xul:1:1
19:51:44     INFO -  GECKO(4576) | ++DOMWINDOW == 74 (000001FC22B21800) [pid = 4576] [serial = 317] [outer = 000001FC1D4A6000]
19:51:45     INFO -  GECKO(4576) | [Parent 4576] WARNING: [nsFrameLoader] ReallyStartLoadingInternal tried but couldn't show remote browser.
19:51:45     INFO -  GECKO(4576) | : file z:/build/build/src/dom/base/nsFrameLoader.cpp, line 794
19:51:45     INFO -  GECKO(4576) | ++DOCSHELL 0000010DF2219000 == 1 [pid = 4644] [id = {59be82cb-ed9b-457f-aa1b-0b3e77403593}]
19:51:45     INFO -  GECKO(4576) | ++DOMWINDOW == 1 (0000010DF221A000) [pid = 4644] [serial = 1] [outer = 0000000000000000]
19:51:45     INFO -  GECKO(4576) | ++DOMWINDOW == 2 (0000010DF2235800) [pid = 4644] [serial = 2] [outer = 0000010DF221A000]
19:51:45     INFO -  GECKO(4576) | ++DOMWINDOW == 3 (0000010DF26B5800) [pid = 4644] [serial = 3] [outer = 0000010DF221A000]
19:51:45     INFO -  GECKO(4576) | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to c:\users\genericworker\appdata\local\temp\tmprl0bxx.mozrunner\runtests_leaks_tab_pid4644.log
19:51:45     INFO -  GECKO(4576) | Unable to read VR Path Registry from C:\Users\GenericWorker\AppData\Local\openvr\openvrpaths.vrpath
19:51:45     INFO -  GECKO(4576) | Assertion failure: (((HRESULT)(unmarshalResult)) >= 0), at z:/build/build/src/ipc/mscom/ProxyStream.cpp:81
19:51:59     INFO -  GECKO(4576) | #01: mozilla::detail::RunnableFunction<<lambda_f354ef0b17c040a487f7d96390bd1695> >::Run() [obj-firefox/dist/include/nsThreadUtils.h:527]
19:51:59     INFO -  GECKO(4576) | #02: nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:1034]
19:51:59     INFO -  GECKO(4576) | #03: NS_ProcessNextEvent(nsIThread *,bool) [xpcom/threads/nsThreadUtils.cpp:521]
19:51:59     INFO -  GECKO(4576) | #04: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:369]
19:51:59     INFO -  GECKO(4576) | #05: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:320]
19:51:59     INFO -  GECKO(4576) | #06: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:300]
19:51:59     INFO -  GECKO(4576) | #07: nsThread::ThreadFunc(void *) [xpcom/threads/nsThread.cpp:425]
19:52:00     INFO -  GECKO(4576) | #08: PR_NativeRunThread [nsprpub/pr/src/threads/combined/pruthr.c:406]
19:52:00     INFO -  GECKO(4576) | #09: pr_root [nsprpub/pr/src/md/windows/w95thred.c:96]
19:52:00     INFO -  GECKO(4576) | #10: ucrtbase.dll + 0x20369
19:52:00     INFO -  GECKO(4576) | #11: KERNEL32.DLL + 0x12774
19:52:00     INFO -  GECKO(4576) | #12: ntdll.dll + 0x70d61

19:57:07    ERROR -  296 INFO TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/jsat/test_content_integration.html | Test timed out.
19:57:07     INFO -  reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:121:7
19:57:07     INFO -  TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:142:7
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:163:5
19:57:07     INFO -  TestRunner.runTests@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:380:5
19:57:07     INFO -  RunSet.runtests@chrome://mochikit/content/tests/SimpleTest/setup.js:194:3
19:57:07     INFO -  RunSet.runall@chrome://mochikit/content/tests/SimpleTest/setup.js:173:5
19:57:07     INFO -  hookupTests@chrome://mochikit/content/tests/SimpleTest/setup.js:266:5
19:57:07     INFO -  parseTestManifest@chrome://mochikit/content/manifestLibrary.js:38:5
19:57:07     INFO -  getTestManifest/req.onload@chrome://mochikit/content/manifestLibrary.js:49:11
19:57:07     INFO -  EventHandlerNonNull*getTestManifest@chrome://mochikit/content/manifestLibrary.js:45:3
19:57:07     INFO -  hookup@chrome://mochikit/content/tests/SimpleTest/setup.js:246:5
19:57:07     INFO -  linkAndHookup@chrome://mochikit/content/harness.xul:59:3
19:57:07     INFO -  parseTestManifest@chrome://mochikit/content/manifestLibrary.js:38:5
19:57:07     INFO -  getTestManifest/req.onload@chrome://mochikit/content/manifestLibrary.js:49:11
19:57:07     INFO -  EventHandlerNonNull*getTestManifest@chrome://mochikit/content/manifestLibrary.js:45:3
19:57:07     INFO -  getTestList@chrome://mochikit/content/chrome-harness.js:260:3
19:57:07     INFO -  loadTests@chrome://mochikit/content/harness.xul:38:3
19:57:07     INFO -  EventListener.handleEvent*@chrome://mochikit/content/harness.xul:62:5

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Grrr... okay, I decided to bite the bullet and implement a definitive solution.

I will be uploading an additional patch that fixes the rest of this but I want to outline the design here:

First of all, I need to give Jamie some credit for the high-level concept here: During our interview he had proposed to me a solution to this problem that involved just passing a child ID into content and eventually out to the client, whereby the handler would perform the final resolution of that ID into an IAccessible.

We both realized that this solution would not work with our a11y code in its present state due to the inability to compile both 32-bit and 64-bit handler DLLs in the same build.

Another concern of mine is that right now our implementation still mostly works even when the handler is turned off (notwithstanding live regions it is entirely a perf optimization), but the above solution forces the handler as a requirement at all times.

My solution in this upcoming patch riffs off of Jamie's idea, but instead of passing the child ID, we still send a serialized proxy. The difference is that we never reconstitute the proxy in content! We just pass around the parent's "interface" which is actually storing the serialized byte stream. Whenever COM wants to marshal that proxy as an outparam to the a11y client, we just memcpy that byte stream into the output stream. Only when the proxy reaches the client is it unmarshaled for real.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

I am reasonably happy with my patch but I need to throw some more testing at it.

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3: Add mscom::PassthruProxy and use it to wrap IDispatch accessibles destined for content

In addition to comment 13, here is some additional explanation:

When marshaling, COM serializes an interface pointer as an IPID, which is a GUID-like value that uniquely identifies the interface. When marshaling a PassthruProxy, we can't just give COM the wrapped interface pointer; COM would just marshal the wrapped interface instead of the PassthruProxy. Instead, we need COM to generate a distinct IPID for the PassthruProxy itself.

To make this happen, PassthruProxy generates a fake version of the interface that it is wrapping. This fake interface has valid IUnknown vtable entries, which are necessary to preserve object identity and to facilitate querying of the other, real interfaces. The remaining vtable entries for that interface are null. Those entries (obviously) must not and will not be called. I allocate and null out those pointers to assert that no code ever

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b913e8d64b10a86b623e33a3ad1668a73123871e
Bug 1390652: Part 1 - Send parent COM proxies to content as IDispatch instead of IAccessible to match the outparam type of IAccessible::get_accParent; r=yzen

https://hg.mozilla.org/integration/mozilla-inbound/rev/755011dc20ce53c723e30e3c51bb74ef748865e7
Bug 1390652: Part 2 - Add support for TABLESTRONG marshaling to mscom::ProxyStream; r=jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d36c75b0a3324b5d960aad6ce50aba4de87792
Bug 1390652: Part 3 - Add proxy wrapper that passes its inner proxy through content as a blob; r=jimm

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/b913e8d64b10
https://hg.mozilla.org/mozilla-central/rev/755011dc20ce
https://hg.mozilla.org/mozilla-central/rev/f2d36c75b0a3