Crash @ nsCSSFrameConstructor::ProcessPendingRestyles

VERIFIED FIXED in Firefox 12

Status

()

Core
Layout
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: mats)

Tracking

(Blocks: 2 bugs, {crash, testcase})

Trunk
mozilla14
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox12+ verified, firefox13+ verified, firefox14 verified, firefox-esr1012+ verified, status1.9.2 unaffected)

Details

(Whiteboard: [sg:critical][qa!], crash signature)

Attachments

(6 attachments)

(Reporter)

Description

5 years ago
Created attachment 606033 [details]
testcase (crashes Firefox when loaded)

About 50% of the time, this testcase causes a crash in nsCSSFrameConstructor::ProcessPendingRestyles.
(Reporter)

Comment 1

5 years ago
Created attachment 606034 [details]
stack trace with popups allowed
(Reporter)

Comment 2

5 years ago
Created attachment 606035 [details]
stack trace with popups blocked
(Assignee)

Updated

5 years ago
Assignee: nobody → matspal
Component: DOM: Core & HTML → Layout
OS: Mac OS X → All
QA Contact: general → layout
Hardware: x86_64 → All
(Assignee)

Comment 3

5 years ago
Created attachment 606052 [details]
stack
(Assignee)

Comment 4

5 years ago
Created attachment 606076 [details] [diff] [review]
fix+test

FYI, the document has a strong pointer to the nsSMILAnimationController
object, which keeps a raw pointer to it (mDocument).

Try results pending:
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=0281d2f4f580

Filed bug 735966 on the assertion when running the crash test.
Attachment #606076 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

5 years ago
Again, with just this change this time (there's a jsreftest crash in the one above):
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=8316f137ca46
(Reporter)

Comment 6

5 years ago
My testcase (reduced from what the fuzzer created) is an insane tangle of setTimeouts. It might be possible to create a more robust regression test ;)

Comment 7

5 years ago
I hit bp-d0df45dc-63db-44d8-a6d8-f76722120315 (popups blocked) and bp-3d521fe3-cc02-44fc-8058-d85972120315 (popups allowed).
Crash Signature: [@ nsCSSFrameConstructor::ProcessPendingRestyles()] [@ nsCSSFrameConstructor::ProcessPendingRestyles] [@ PresShell::FlushPendingNotifications(mozFlushType)]
Summary: Crash [@ nsCSSFrameConstructor::ProcessPendingRestyles] → Crash @ nsCSSFrameConstructor::ProcessPendingRestyles
(Assignee)

Comment 8

5 years ago
Hmm, there's a couple of "ASSERTION: This is unsafe! Fix the caller!: 'Error', content/events/src/nsEventDispatcher.cpp, line 558" on the new crash test.
https://tbpl.mozilla.org/php/getParsedLog.php?id=10090225&tree=Try&full=1#error0
Comment on attachment 606076 [details] [diff] [review]
fix+test

Document (probably in the header and at the presshell check for mIsDestroying) that FlushResampleRequests can trigger style flushes, and r=me
Attachment #606076 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/75f461e05221
Flags: in-testsuite+
(Assignee)

Comment 11

5 years ago
This crash fix seems worth taking on branches after baking it for a while...
tracking-firefox12: --- → ?
tracking-firefox13: --- → ?
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/75f461e05221
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(In reply to Mats Palmgren [:mats] from comment #11)
> This crash fix seems worth taking on branches after baking it for a while...

Do we know what the regressing bug is or whether this is a pain point for users? If not, this can ride the trains.
(Assignee)

Comment 14

5 years ago
I would guess the addition of the FlushResampleRequests() call in
http://hg.mozilla.org/mozilla-central/rev/d6dbb4231c11
is what caused the regression (bug 547333).

The reason I recommend merging this fix to branches is that without it we
run script from inside a destroyed pres shell, which I think is unsafe.
The fix is very low-risk.
(Assignee)

Comment 15

5 years ago
Comment on attachment 606076 [details] [diff] [review]
fix+test

See comment 14.
Attachment #606076 - Flags: approval-mozilla-beta?
Attachment #606076 - Flags: approval-mozilla-aurora?
Group: core-security
status1.9.2: --- → unaffected
status-firefox-esr10: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → fixed
tracking-firefox-esr10: --- → ?
Whiteboard: [sg:critical]

Updated

5 years ago
tracking-firefox-esr10: ? → 12+
tracking-firefox12: ? → +
tracking-firefox13: ? → +
Comment on attachment 606076 [details] [diff] [review]
fix+test

[Triage Comment]
Low risk fix, and both Mats and the security team agree that this is something we should fix all the way up to FF12. Please land asap, as we'll go to build with Beta 4 tomorrow (4/3) at 5PM PT.

Please make sure to prepare a patch for the ESR branch as well.
Attachment #606076 - Flags: approval-mozilla-beta?
Attachment #606076 - Flags: approval-mozilla-beta+
Attachment #606076 - Flags: approval-mozilla-aurora?
Attachment #606076 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 18

5 years ago
Created attachment 611648 [details] [diff] [review]
for esr10

The fix applies also to esr10, but without the test since the pref()
reftest feature doesn't exist there, afaict.
Attachment #611648 - Flags: approval-mozilla-esr10?
Verified fixed in nightly Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20120402 Firefox/14.0a1. The testcase pretty reliably crashes my Aurora build.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/18753a4ee7cd
https://hg.mozilla.org/releases/mozilla-beta/rev/a958bd24a213
status-firefox12: affected → fixed
status-firefox13: affected → fixed
Comment on attachment 611648 [details] [diff] [review]
for esr10

Thanks, please go ahead and land as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Attachment #611648 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/releases/mozilla-esr10/rev/f66c6b1c5ee6
status-firefox-esr10: affected → fixed
Whiteboard: [sg:critical] → [sg:critical][qa+]

Updated

5 years ago
status-firefox12: fixed → verified

Updated

5 years ago
status-firefox13: fixed → verified

Updated

5 years ago
status-firefox-esr10: fixed → verified

Updated

5 years ago
status-firefox14: fixed → verified
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Group: core-security
You need to log in before you can comment on or make changes to this bug.