1319640 - [e10s a11y] Remote retrieval of native accessibles for windowed plugins

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Alice0775 White]

Attachment: testcase

[Tracking Requested - why for this release]: Tab crashes

This bug was filed from the Socorro interface and is 
report bp-d78e1d5d-909e-41de-89d4-398772161123.
=============================================================

Build Identifier:
https://hg.mozilla.org/releases/mozilla-aurora/rev/0b59b8da8f7566f70eb2ac013e60192d1d1c6adb
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0 ID:20161122004020

When open a page with wmode=diret Flash Player 23.0.0.207,
Aurora52.0a2 tab crashes if set dom.ipc.plugins.asyncdrawing.enabled=false and e10s.

Bug 1311748 does not fix the crash in this case.


Reproducible: 100%

Steps To Reproduce:
1. Disable asyncdrawing 
   set dom.ipc.plugins.asyncdrawing.enabled=false
   (I think this is default value for 52 when it will be released)
2. Restart browser
3. Open http://www3.nhk.or.jp/nhkworld/en/live/
   or 
   Open attached testcase

Actual Results:
Tab crashes

Expected Results:
Not crash

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: crash tabs if dom.ipc.plugins.asyncdrawing.enabled=false && security.sandbox.content.level=1


Setting security.sandbox.content.level=2 seems fix the crash on Aurora52.0a2.
And Setting security.sandbox.content.level=1 triggers the crash on Nightly53.0a1.

Comment 2

[Alice0775 White]

Regression window(w/ dom.ipc.plugins.asyncdrawing.enabled=false && security.sandbox.content.level=1):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b6cf6d7a95f993f1b3f656ccaad5445368c85cb2&tochange=183c29790c23279326021e0f6ea6e2c72fb47fc9

Indeed, setting accessibility.force_disabled=1 fixes the crash.

Comment 3

[Alice0775 White]

Reproduced the crash on Beta51.0b2 dom.ipc.plugins.asyncdrawing.enabled=false;

Comment 4

[Alice0775 White]

[Tracking Requested - why for this release]:

(In reply to Alice0775 White from comment #2)
> Regression window(w/ dom.ipc.plugins.asyncdrawing.enabled=false &&
> security.sandbox.content.level=1):
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=b6cf6d7a95f993f1b3f656ccaad5445368c85cb2&tochange=183c
> 29790c23279326021e0f6ea6e2c72fb47fc9
> 
> Indeed, setting accessibility.force_disabled=1 fixes the crash.


Via local build,
Last Good: ad2a728e5832
First Bad: bf20e2879a4e

Regressed by: bf20e2879a4e	Aaron Klotz — Bug 1288841: Add a typelib containing info for IServiceProvider and IEnumVARIANT; r=tbsaunde, mshal

Comment 5

[Alice0775 White]

Attachment: about:suppot

Comment 6

[Alice0775 White]

51.0b2        bp-2dfe236d-efae-4ea9-865c-a8cf62161123
Aurora52.0a2  bp-ad413ead-f130-4049-a824-de8672161123
Nightly53.0a1 bp-9c5b73c0-aa7c-4133-b495-c751f2161123

Comment 7

[Michael Shal [:mshal]]

I reviewed bug 1288841 for the build config changes - I am not sure why the patch would cause a crash. Alexander, are you or someone else on the accessibility team able to take a look?

Comment 8

[Gerry Chang [:gchang]]

Track 51+ as this is reproduce crash.

Comment 9

[Trevor Saunders (:tbsaunde)]

It seems likely this is wacky windows shit, and I don't have time to look for a couple days, aklotz can you look?

Comment 10

[Alice0775 White]

It seems to require ATOK2016 Japanese IME to activate accessibility.
(trial version is available, see https://bugzilla.mozilla.org/show_bug.cgi?id=1297437#c15)

Comment 11

[Alice0775 White]

Tab also crashes with wmode=windowed.

Comment 12

[Alice0775 White]

http://emk.name/test/swftxt.html page is affected too.

Comment 13

[Benjamin Smedberg]

Related, bug 1320192. Aaron, fallout from e10s a11y?

Comment 14

[Benjamin Smedberg]

->jimm since Aaron isn't accepting NI

Comment 15

[Jim Mathies [:jimm]]

aklotz will look at this and bug 1320192 next.

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Wow, this is a crazy bug that sits at the convergence between e10s, a11y, sandboxing, and plugins!

In a11y, when we encounter an embedded windowed plugin, we create a HTMLWin32ObjectAccessible. Its job is to resolve to a COM proxy that points to the "real" implementation via the plugin's HWND. We call AccessibleObjectFromWindow to do this.

BUT: If we're sandboxed, this call will fail because it cannot send WM_GETOBJECT to a HWND outside the sandbox!

The second issue falls out from the first: We return a nullptr native interface, but our code in ChildrenEnumVariant::Next doesn't check for that, so it ends up stuffing an invalid pointer into its output and returning success.

Any resulting code that processes that output will not fare well, including MainThreadHandoff (where this particular crash hit) and/or the a11y client itself.

This fix will require two things:

1) Make HTMLWin32ObjectAccessible::GetNativeInterface resolve its COM proxy in a way that is sandboxing-aware. The quick 'n' dirty way would be via a sync call to chrome. :-/

2) Make sure that ChildrenEnumVariant::Next notices when AccessibleWrap::NativeAccessible fails and does not produce bogus output.

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch

Comment 18

[Julien Cristau [:jcristau] (back April 22)]

tracking this e10s/a11y related bug for 52

Comment 19

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8815456 [details] [diff] [review]
Patch

># HG changeset patch
># User Aaron Klotz <[e-mail]>
># Date 1480445664 25200
>#      Tue Nov 29 11:54:24 2016 -0700
># Node ID fad3f82ccf826390f9333177327a47ee56276873
># Parent  7e25717850b9d0eb40b8dbd53bfb109d77b01d68
>Bug 1319640: Ensure that a11y::ChildrenEnumVariant does not output bad native accessible pointers and make obtaining of plugin IAccessible go through Chrome process on Sandboxed builds; r=tbsaunde

I think ideally these would be separate patches since they do independent things, and can be reviewed completely independently.

>+  // one that belongs to its child (see HTMLWin32ObjectAccessible).
>+  HWND childWnd = ::GetWindow(reinterpret_cast<HWND>(aHwnd), GW_CHILD);
>+  if (!childWnd) {
>+    return IPC_FAIL(this, "GetWindow failed");

can't this fail for a windowless plugin? I guess we don't have a gecko window either then?

>+  }
>+
>+  IAccessible* rawAccPlugin = nullptr;
>+  HRESULT hr = ::AccessibleObjectFromWindow(childWnd, OBJID_WINDOW,
>+                                            IID_IAccessible,
>+                                            (void**)&rawAccPlugin);
>+  if (FAILED(hr)) {
>+    return IPC_FAIL(this, "AccessibleObjectFromWindow failed");

doesn't that fail if the plugin doesn't deal with accessibility and handle WM_GETOBJECT?

>     VariantInit(aItems + countFetched);
>-    aItems[countFetched].pdispVal = AccessibleWrap::NativeAccessible(mCurAcc);
>+
>+    IDispatch* accNative = AccessibleWrap::NativeAccessible(mCurAcc);
>+
>+    mCurAcc = mAnchorAcc->GetChildAt(++mCurIndex);

I'd really rather that the increment was a separate statement.

>   mHwnd = aHwnd;
>   if (mHwnd) {
>+#if defined(MOZ_CONTENT_SANDBOX)

do we need the ifdefing? it seems like it should be fine to always have this code.

Thinking about this more I'm not actually sure its easy to avoid the sync message since the accessible wants to live in the content process.

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: IEnumVariant Fix

(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> Comment on attachment 8815456 [details] [diff] [review]
> Patch
> 
> ># HG changeset patch
> ># User Aaron Klotz <[e-mail]>
> ># Date 1480445664 25200
> >#      Tue Nov 29 11:54:24 2016 -0700
> ># Node ID fad3f82ccf826390f9333177327a47ee56276873
> ># Parent  7e25717850b9d0eb40b8dbd53bfb109d77b01d68
> >Bug 1319640: Ensure that a11y::ChildrenEnumVariant does not output bad native accessible pointers and make obtaining of plugin IAccessible go through Chrome process on Sandboxed builds; r=tbsaunde
> 
> I think ideally these would be separate patches since they do independent
> things, and can be reviewed completely independently.
Sure, I've split them up.

> >     VariantInit(aItems + countFetched);
> >-    aItems[countFetched].pdispVal = AccessibleWrap::NativeAccessible(mCurAcc);
> >+
> >+    IDispatch* accNative = AccessibleWrap::NativeAccessible(mCurAcc);
> >+
> >+    mCurAcc = mAnchorAcc->GetChildAt(++mCurIndex);
> 
> I'd really rather that the increment was a separate statement.

Fixed.

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: HTMLWin32ObjectAccessible Fix

(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> Comment on attachment 8815456 [details] [diff] [review]
> Patch
> 
> >+  // one that belongs to its child (see HTMLWin32ObjectAccessible).
> >+  HWND childWnd = ::GetWindow(reinterpret_cast<HWND>(aHwnd), GW_CHILD);
> >+  if (!childWnd) {
> >+    return IPC_FAIL(this, "GetWindow failed");
> 
> can't this fail for a windowless plugin? I guess we don't have a gecko
> window either then?

Content doesn't make the call up to chrome unless the plugin is Windowed. The HWND would be null.

> 
> >+  }
> >+
> >+  IAccessible* rawAccPlugin = nullptr;
> >+  HRESULT hr = ::AccessibleObjectFromWindow(childWnd, OBJID_WINDOW,
> >+                                            IID_IAccessible,
> >+                                            (void**)&rawAccPlugin);
> >+  if (FAILED(hr)) {
> >+    return IPC_FAIL(this, "AccessibleObjectFromWindow failed");
> 
> doesn't that fail if the plugin doesn't deal with accessibility and handle
> WM_GETOBJECT?

Possibly. If they pass WM_GETOBJECT to DefWindowProc I think it still succeeds with a system-provided IAccessible, but it could fail. I fixed this just in case.

> >   mHwnd = aHwnd;
> >   if (mHwnd) {
> >+#if defined(MOZ_CONTENT_SANDBOX)
> 
> do we need the ifdefing? it seems like it should be fine to always have this
> code.

Yes, it's fine, it's more that non-sandboxed builds can still make the call directly.

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/522ef8286421b04ade4f963542ac35aa51e72e6a
Bug 1319640: Ensure that a11y::ChildrenEnumVariant does not output bad native accessible pointers; r=tbsaunde

https://hg.mozilla.org/integration/mozilla-inbound/rev/96749829f50afb4e1fd86195b8ca2e6b269c1ba6
Bug 1319640: Make obtaining of plugin IAccessible go through Chrome process on Sandboxed builds; r=tbsaunde

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/522ef8286421
https://hg.mozilla.org/mozilla-central/rev/96749829f50a

Comment 24

[Alice0775 White]

No longer crash on latest Nightly53.0a3[1].
I verified that the problem is fixed.

[1]
https://hg.mozilla.org/mozilla-central/rev/bd9e81439725f3d4135652cc3d65f2bfba527b7b
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20161205030204

Comment 25

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8816718 [details] [diff] [review]
IEnumVariant Fix

Approval Request Comment
[Feature/Bug causing the regression]: Windows+a11y+e10s+sandboxing+plugins
[User impact if declined]: Crashes either in Firefox or in their a11y clients
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: The other patch posted in this bug.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple patch
[String changes made/needed]: None

Comment 26

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8816718 [details] [diff] [review]
IEnumVariant Fix

windows a11y/e10s/sandboxing/... fix, for aurora52

Comment 27

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8816719 [details] [diff] [review]
HTMLWin32ObjectAccessible Fix

windows a11y/e10s/sandboxing/... fix, for aurora52

Comment 28

[Carsten Book [:Tomcat]]

needs rebasing for aurora

Comment 29

[Gerry Chang [:gchang]]

Track 51- as the volume of crash in 51 is low.

Comment 30

[(No longer employed by Mozilla) Aaron Klotz]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/08f5f2490cec7d4e4eaa175a1a03b7a6bb50a882
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/0644964811f1423a2c0958e141fa92ee41695279

Comment 31

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8816718 [details] [diff] [review]
IEnumVariant Fix

Approval Request Comment
[Feature/Bug causing the regression]: a11y
[User impact if declined]: Potential crashes due to null pointers being accessed by injected DLLs
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None. Furthermore, this patch may be uplifted independently of the other patch in this bug.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple patch, verified fix
[String changes made/needed]: None

Comment 32

[Gerry Chang [:gchang]]

Comment on attachment 8816718 [details] [diff] [review]
IEnumVariant Fix

Fix a crash. Beta51+. Should be in 51 beta 10.

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5e4495f87ea2