Closed Bug 1319640 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Disability Access APIs, defect, critical)

51 Branch
x86
Windows
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 - fixed
firefox52 + fixed
firefox53 + fixed

People

(Reporter: alice0775, Assigned: aklotz)

References

()

Details

(5 keywords, Whiteboard: aes+)

Crash Data

Attachments

(4 files, 1 obsolete file)

Attached file 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
[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.
Blocks: 1246505
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.
Summary: Crash in mozilla::mscom::IsProxy , when open wmode-direct Flash Player → [e10s a11y] Crash in mozilla::mscom::IsProxy , when open wmode-direct Flash Player
Reproduced the crash on Beta51.0b2 dom.ipc.plugins.asyncdrawing.enabled=false;
[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
Blocks: 1288841
Flags: needinfo?(tbsaunde+mozbugs)
Flags: needinfo?(mshal)
Version: 52 Branch → 51 Branch
Attached file about:suppot
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?
Flags: needinfo?(mshal) → needinfo?(surkov.alexander)
Track 51+ as this is reproduce crash.
It seems likely this is wacky windows shit, and I don't have time to look for a couple days, aklotz can you look?
Flags: needinfo?(tbsaunde+mozbugs)
Flags: needinfo?(surkov.alexander)
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)
Keywords: jp-critical
Tab also crashes with wmode=windowed.
Summary: [e10s a11y] Crash in mozilla::mscom::IsProxy , when open wmode-direct Flash Player → [e10s a11y] Crash in mozilla::mscom::IsProxy , when open wmode=direct/windowed Flash Player
http://emk.name/test/swftxt.html page is affected too.
Related, bug 1320192. Aaron, fallout from e10s a11y?
->jimm since Aaron isn't accepting NI
Flags: needinfo?(jmathies)
aklotz will look at this and bug 1320192 next.
Flags: needinfo?(jmathies)
Assignee: nobody → aklotz
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.
Status: NEW → ASSIGNED
Component: IPC → Disability Access APIs
OS: Windows 10 → Windows
Attached patch Patch (obsolete) — Splinter Review
Attachment #8815456 - Flags: review?(tbsaunde+mozbugs)
Summary: [e10s a11y] Crash in mozilla::mscom::IsProxy , when open wmode=direct/windowed Flash Player → [e10s a11y] Remote retrieval of native accessibles for windowed plugins
tracking this e10s/a11y related bug for 52
Whiteboard: aes+
Comment on attachment 8815456 [details] [diff] [review]
Patch

># HG changeset patch
># User Aaron Klotz <aklotz@mozilla.com>
># 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.
Attachment #8815456 - Flags: review?(tbsaunde+mozbugs) → review+
Attached patch IEnumVariant FixSplinter Review
(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> Comment on attachment 8815456 [details] [diff] [review]
> Patch
> 
> ># HG changeset patch
> ># User Aaron Klotz <aklotz@mozilla.com>
> ># 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.
Attachment #8815456 - Attachment is obsolete: true
Attachment #8816718 - Flags: review+
(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.
Attachment #8816719 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/522ef8286421
https://hg.mozilla.org/mozilla-central/rev/96749829f50a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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 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
Attachment #8816718 - Flags: approval-mozilla-aurora?
Comment on attachment 8816718 [details] [diff] [review]
IEnumVariant Fix

windows a11y/e10s/sandboxing/... fix, for aurora52
Attachment #8816718 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8816719 [details] [diff] [review]
HTMLWin32ObjectAccessible Fix

windows a11y/e10s/sandboxing/... fix, for aurora52
Attachment #8816719 - Flags: approval-mozilla-aurora+
needs rebasing for aurora
Flags: needinfo?(aklotz)
Track 51- as the volume of crash in 51 is low.
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
Attachment #8816718 - Flags: approval-mozilla-beta?
Comment on attachment 8816718 [details] [diff] [review]
IEnumVariant Fix

Fix a crash. Beta51+. Should be in 51 beta 10.
Attachment #8816718 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.