813442 - Crash opening/closing a <select> while entering/leaving fullscreen mode

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires prefs, focus, and holding Alt+Down)

1. Create a profile directory with the following prefs.js file:

  user_pref("browser.fullscreen.autohide", false);
  user_pref("browser.tabs.autoHide", true);
  user_pref("full-screen-api.allow-trusted-requests-only", false);

2. Run Firefox like this:

  firefox -foreground -profile ~/px/b/ r.html

3. Verify that r.html is the only open tab, and it is entering and exiting full-screen mode on its own.

4. Hold <Alt+Down> until it crashes.

Result: Crash, usually within 15 seconds.

Comment 1

[Jesse Ruderman]

Attachment: stack traces from ASan

ASan isn't required for reproducing this bug, but it does produce a useful set of stack traces.

Comment 2

[Jesse Ruderman]

Attachment: non-ASan mac crash report

Comment 3

[Andrew McCreight [:mccr8]]

Guessing sec-high based on UAF, though it is a read and not a write.

Comment 4

[Mats Palmgren (inactive)]

I can't reproduce the crash in ASan and non-ASan debug builds on OSX 10.7.

I get these non-fatal assertions on both OSX and Linux64:
###!!! ASSERTION: We shouldn't have a widget before we need to display the popup: '!view->HasWidget()', file layout/forms/nsComboboxControlFrame.cpp, line 394
###!!! ASSERTION: We already have a window for this view? BAD: 'Error', file view/src/nsView.cpp, line 675

So the underlying issue might be the same as bug 798230.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Stack for fatal assertion on Linux64

Comment 6

[Mats Palmgren (inactive)]

I can reproduce a fatal assertion in ASan debug build on Linux64:
(with the stack in attachment 708395 [details])

###!!! ASSERTION: frame state bit was set but frame has no view: 'value', file layout/generic/nsFrame.cpp, line 4392
Assertion failure: view, at layout/forms/nsComboboxControlFrame.cpp:1545

http://hg.mozilla.org/mozilla-central/annotate/faf255e44002/layout/forms/nsComboboxControlFrame.cpp#l1545

Comment 7

[Mats Palmgren (inactive)]

Attachment: stack for "We shouldn't have a widget before we need to display the popup"

The reason is that we do the "view->DestroyWidget()" async from an event
(added in bug 682041).

Comment 8

[Mats Palmgren (inactive)]

Attachment: stack for "frame state bit was set but frame has no view"

Comment 9

[Mats Palmgren (inactive)]

Attachment: Do the DestroyWidget() synchronously with a strong ref on the widget so that it's not deleted, drop the ref asynchronously later

This mostly reverts the fix for bug 682041 so that DestroyWidget()
is called synchronously and now only delay dropping the ref to the
widget to avoid it being deleted while being inside widget code
on the stack (which was the cause of the crash in bug 682041
if I understand it correctly).  I've tested this with the STR
in bug 682041 on Windows and Linux.

This fixes the two assertions in comment 4 which were caused
by calling ShowDropDown with a pending DestroyWidgetRunnable.

Comment 10

[Mats Palmgren (inactive)]

Attachment: CaptureRollupEvents(false) before calling anything that can destroy us

This fixes the crash which is caused by the frame being
deleted in ShowList *before* CaptureRollupEvents(false)
so it was still registered as the rollup listener.
See the stack in attachment 708395 [details].

Comment 11

[Mats Palmgren (inactive)]

Attachment: Remove a couple of unnecessary do_QueryFrames and null checks

Comment 12

[Mats Palmgren (inactive)]

Green on Try:  https://tbpl.mozilla.org/?tree=Try&rev=9bc209b82cb3

I've tested the STR in comment 0 on Win7, OSX and Linux --
no crash, no assertions.

Comment 13

[Mats Palmgren (inactive)]

Steven, maybe this will help with bug 836236 / bug 682208 ?

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Yes, it's possible.

Simona at bug 682208 can reproduce the crashes (though most of her crashes are probably bug 836236).  I'll post a link at bug 682208 to your 10.7 opt tryserver build, and ask her to test it.

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 708609 [details] [diff] [review]
Do the DestroyWidget() synchronously with a strong ref on the widget so that it's not deleted, drop the ref asynchronously later

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
don't know

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
no

Which older supported branches are affected by this flaw?
all branches

If not all supported branches, which bug introduced the flaw?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
the patches will apply as is with minor modifications

How likely is this patch to cause regressions; how much testing does it need?
medium risk because this code is notoriously error prone and our test
coverage is poor

Comment 16

[Mats Palmgren (inactive)]

Fwiw, it has been confirmed that this also fixes bug 682208 which is the #10 crash
for Firefox 19.0b3 on Mac OS X:
https://crash-stats.mozilla.com/topcrasher/byos/Firefox/19.0b3/Mac%20OS%20X/7/browser
(so it's probably worth taking in 19b5 if there is one)

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 708609 [details] [diff] [review]
Do the DestroyWidget() synchronously with a strong ref on the widget so that it's not deleted, drop the ref asynchronously later

sec-approval+ = dveditz

Shouldn't need to take this in Fx19 on security grounds, no one is likely to find a working PoC based on the patch. Whether the topcrash fix is worth the (small? medium?) regression risk is up to release managers. We should take this on the ESR-17 and b2g18 releases that match when this ships on mainline.

Comment 18

[Alex Keybl [:akeybl]]

Given the risk here, and the fact that FF18 is affected by bug 682208, we'll wontfix for FF19 and take the fix for the first time in FF20. Please nominate for uplift to Aurora when you get the chance.

Comment 19

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/292f8e2dc39e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e72b799bfc1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bdd1b235c12

Comment 20

[(no longer active)]

Regression: Mozilla-Inbound - Tp5 No Network Row Major Responsiveness MozAfterPaint - MacOSX 10.8 - 9.68% increase
------------------------------------------------------------------------------------------------------------------
    Previous: avg 49.233 stddev 1.736 of 30 runs up to revision caf13c3365f3
    New     : avg 54.000 stddev 0.707 of 5 runs since revision 3bdd1b235c12
    Change  : +4.767 (9.68% / z=2.746)
    Graph   : http://mzl.la/Xm7qJb

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=caf13c3365f3&tochange=3bdd1b235c12

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/292f8e2dc39e
    : Mats Palmgren <[e-mail]> - Bug 813442 - Do the DestroyWidget() synchronously with a strong ref on the widget so that it's not deleted, drop the ref asynchronously later.  r=roc
    : http://bugzilla.mozilla.org/show_bug.cgi?id=813442

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/7e72b799bfc1
    : Mats Palmgren <[e-mail]> - Bug 813442 - CaptureRollupEvents(false) before calling anything that can destroy us.  r=roc
    : http://bugzilla.mozilla.org/show_bug.cgi?id=813442

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/3bdd1b235c12
    : Mats Palmgren <[e-mail]> - Bug 813442 - Remove useless do_QueryFrames and null checks.  r=roc
    : http://bugzilla.mozilla.org/show_bug.cgi?id=813442

Comment 21

[Mats Palmgren (inactive)]

The patches in this bug only touches nsComboboxControlFrame, nothing else.
It seems extremely unlikely that it could have caused the above regression.

Comment 22

[Mats Palmgren (inactive)]

Comment on attachment 708609 [details] [diff] [review]
Do the DestroyWidget() synchronously with a strong ref on the widget so that it's not deleted, drop the ref asynchronously later

As requested in comment 18.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bdd1b235c12
https://hg.mozilla.org/mozilla-central/rev/7e72b799bfc1
https://hg.mozilla.org/mozilla-central/rev/292f8e2dc39e

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 708609 [details] [diff] [review]
Do the DestroyWidget() synchronously with a strong ref on the widget so that it's not deleted, drop the ref asynchronously later

Please prepare a patch for ESR17 or nominate this one if it applies cleanly.

Comment 25

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5670705438d8
https://hg.mozilla.org/releases/mozilla-aurora/rev/95dc87a041cf
https://hg.mozilla.org/releases/mozilla-aurora/rev/d1ed6825646e

Comment 26

[Mats Palmgren (inactive)]

Attachment: Aurora - all patches

For the record, the patches applied cleanly to Aurora.  This is the resulting
combined changes...

Comment 27

[Mats Palmgren (inactive)]

Attachment: ESR17 - all patches

This is resulting combined changes for ESR17 after some minor tweaks like
s/nsView/nsIView/ etc. There's one hunk that did not apply on ESR17 (from part 3):

 nsIWidget*
 nsComboboxControlFrame::GetRollupWidget()
 {
-  nsIFrame* listFrame = do_QueryFrame(mListControlFrame);
-  if (!listFrame)
-    return nullptr;
-
-  nsView* view = listFrame->GetView();
+  nsView* view = mDropdownFrame->GetView();
   MOZ_ASSERT(view);
   return view->GetWidget();
 }

Compare the nsWindow::Destroy between Aurora / ESR17:
http://mxr.mozilla.org/mozilla-aurora/source/widget/gtk2/nsWindow.cpp#638
http://mxr.mozilla.org/mozilla-esr17/source/widget/gtk2/nsWindow.cpp#620

I think it's clear this hunk isn't needed -
nsComboboxControlFrame::GetRollupWidget() doesn't exist on ESR17 and instead
there is a "do_QueryReferent(gRollupWindow);" where gRollupWindow is a nsWeakPtr.

Mostly the same deal for other widget platforms:
http://mxr.mozilla.org/mozilla-esr17/source/widget/qt/nsWindow.cpp#387

Windows use nsIWidget* and manual addref/release though:
http://mxr.mozilla.org/mozilla-esr17/source/widget/windows/nsWindow.cpp#3048
http://mxr.mozilla.org/mozilla-esr17/source/widget/windows/nsWindow.cpp#7029

Same:
http://mxr.mozilla.org/mozilla-esr17/source/widget/cocoa/nsCocoaWindow.mm#1638
http://mxr.mozilla.org/mozilla-esr17/source/widget/cocoa/nsChildView.mm#2770
http://mxr.mozilla.org/mozilla-esr17/source/widget/cocoa/nsCocoaWindow.mm#93

Same:
http://mxr.mozilla.org/mozilla-esr17/source/widget/os2/nsWindow.cpp#1531
http://mxr.mozilla.org/mozilla-esr17/source/widget/os2/nsWindow.cpp#497

"CaptureRollupEvents(false)" in part 2 clears the gRollupWidget.
So landing this on ESR17 without the hunk above seems correct to me.

Comment 28

[Mats Palmgren (inactive)]

Comment on attachment 711021 [details] [diff] [review]
ESR17 - all patches

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: relatively common crash (bug 682208, maybe more)
Fix Landed on Version: on m-c since 2013-02-05, on aurora since 2013-02-06
Risk to taking this patch (and alternatives if risky):
medium risk because this code is notoriously error prone and our test
coverage is poor
String or UUID changes made by this patch: none

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.

Comment 30

[Alex Keybl [:akeybl]]

Comment on attachment 711021 [details] [diff] [review]
ESR17 - all patches

Common sec-high crash - let's try to fix this on ESR17 as well.

Comment 31

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-esr17/rev/4133e875bcb0
https://hg.mozilla.org/releases/mozilla-esr17/rev/2f9d50e0f5cf
https://hg.mozilla.org/releases/mozilla-esr17/rev/f55ce11de837

Comment 32

[Alex Keybl [:akeybl]]

(In reply to Mats Palmgren [:mats] from comment #28)
> Comment on attachment 711021 [details] [diff] [review]

B2G18 is affected, but I don't believe B2G itself is affected (no browser fullscreen mode). Can we mark this as unaffected for that branch?

Comment 33

[Mats Palmgren (inactive)]

There's another reason mobile / b2g aren't affected, they have
MOZ_USE_NATIVE_POPUP_WINDOWS = 1
http://dxr.mozilla.org/search?tree=mozilla-central&q=MOZ_USE_NATIVE_POPUP_WINDOWS

The (Gecko) dropdown menu isn't used in that case as can be seen here:
http://hg.mozilla.org/mozilla-central/annotate/0e7639e3bdfb/layout/forms/nsComboboxControlFrame.cpp#l1690
http://hg.mozilla.org/mozilla-central/annotate/0e7639e3bdfb/layout/forms/nsListControlFrame.cpp#l2255
http://hg.mozilla.org/mozilla-central/annotate/0e7639e3bdfb/layout/forms/nsListControlFrame.cpp#l2019

Comment 34

[Mats Palmgren (inactive)]

Can the attached test be modified to it can be run as a crashtest?
If not, can it be added to one of the other test suites?

Comment 35

[Jesse Ruderman]

As reported, the bug requires holding alt+down. If you can't make a testcase without that requirement, I guess you'd have to synthesize the keys. I've seen mochitests do that, but haven't written them myself.