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

RESOLVED FIXED in Firefox 20

Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: jruderman, Assigned: mats)

Tracking

(4 keywords)

Trunk
mozilla21
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox18 wontfix, firefox19+ wontfix, firefox20+ fixed, firefox21+ fixed, firefox-esr1720+ fixed, b2g18 unaffected, b2g18-v1.0.0 unaffected, b2g18-v1.0.1 unaffected)

Details

(Whiteboard: also fixes topcrash 682208 [adv-main20+][adv-esr1705+], crash signature)

Attachments

(11 attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
ASan isn't required for reproducing this bug, but it does produce a useful set of stack traces.
(Reporter)

Comment 2

7 years ago
Guessing sec-high based on UAF, though it is a read and not a write.
Keywords: csec-uaf, sec-high
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 6

6 years ago
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
(Assignee)

Comment 7

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

Updated

6 years ago
Assignee: nobody → matspal
OS: Mac OS X → All
Hardware: x86_64 → All
(Assignee)

Comment 9

6 years ago
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.
Attachment #708609 - Flags: review?(roc)
(Assignee)

Comment 10

6 years ago
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].
Attachment #708612 - Flags: review?(roc)
(Assignee)

Comment 12

6 years ago
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.
(Assignee)

Comment 13

6 years ago
Steven, maybe this will help with bug 836236 / bug 682208 ?
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.
(Assignee)

Comment 15

6 years ago
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
Attachment #708609 - Flags: sec-approval?
(Assignee)

Comment 16

6 years ago
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)
Whiteboard: also fixes topcrash 682208
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.
Attachment #708609 - Flags: sec-approval? → sec-approval+
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.
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 <matspal@gmail.com> - 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 <matspal@gmail.com> - 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 <matspal@gmail.com> - Bug 813442 - Remove useless do_QueryFrames and null checks.  r=roc
    : http://bugzilla.mozilla.org/show_bug.cgi?id=813442
(Assignee)

Comment 21

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

Comment 22

6 years ago
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.
Attachment #708609 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3bdd1b235c12
https://hg.mozilla.org/mozilla-central/rev/7e72b799bfc1
https://hg.mozilla.org/mozilla-central/rev/292f8e2dc39e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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.
Attachment #708609 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 26

6 years ago
For the record, the patches applied cleanly to Aurora.  This is the resulting
combined changes...
(Assignee)

Comment 27

6 years ago
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.
Attachment #711021 - Flags: feedback?(roc)
(Assignee)

Comment 28

6 years ago
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
Attachment #711021 - Flags: approval-mozilla-esr17?
(Assignee)

Updated

6 years ago
Crash Signature: [@ nsViewManager::Refresh] [@ nsIViewManager::GetDisplayRootFor | nsViewManager::Refresh] → [@ nsViewManager::Refresh] [@ nsIViewManager::GetDisplayRootFor | nsViewManager::Refresh] [@ nsComboboxControlFrame::GetRollupWidget() ]
Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.
Comment on attachment 711021 [details] [diff] [review]
ESR17 - all patches

Common sec-high crash - let's try to fix this on ESR17 as well.
Attachment #711021 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
(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?
Whiteboard: also fixes topcrash 682208 → also fixes topcrash 682208 [adv-main20+][adv-esr1705+]
Group: core-security
(Assignee)

Comment 34

5 years ago
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?
Flags: needinfo?(jruderman)
(Reporter)

Comment 35

5 years ago
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.
Flags: needinfo?(jruderman)
You need to log in before you can comment on or make changes to this bug.