Closed Bug 403454 Opened 17 years ago Closed 17 years ago

[FIX]Crash [@ nsRuleNode::Mark] with {ib}, :before, zooming

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9beta2

People

(Reporter: jruderman, Assigned: bzbarsky)

References

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical?])

Crash Data

Attachments

(2 files)

testcase (see steps in comment 0)
498 bytes, text/html
Details
Fix
1.31 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review 
Steps to reproduce:
1. In mozilla/layout/style/nsStyleSet.cpp, change kGCInterval from 1000 to 1.
2. Load the testcase.
3. Immediately zoom in or out using Cmd++ or Cmd+- (before it goes gray).

Result: Crash [@ nsRuleNode::Mark] dereferencing 0xddddddf5.
Flags: blocking1.9?
Whiteboard: [sg:critical?]
Hrm, I thought Eli's patch suppressed rule tree GC during reconstruct.
Component: Layout → Style System (CSS)
QA Contact: layout → style-system
(Though perhaps that's not the issue.)
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Somehow during frame construction we end up with an mRoots[2]->mParent that's not null in the style set.  I don't see how that could happen, exactly, short of memory corruption: as far as I can tell mRoots is only written to clear it or remove things or add things with no parent, and nsStyleContext's mParent is immutable...
Attached patch Fix — — Splinter Review 
The problem was that we were actually destroying a root style context while we had an mOldRuleTree.  To be more precise, we had a pending restyle for the float change.  So when we reresolved style on zoom, we got a reconstruct hint for that node.  Due to {ib} splits we actually reframe the <body>.  So now the sequence of events is:

 * start rule tree reconstruct
 * reresolve style
 * reframe the <body>
 * this last calls into PropagateScrollToViewport
 * PropagateScrollToViewport resolves style for the root node
 * Since we don't do sharing of root style contexts, this creates a new root
   context and adds it to mRoots
 * The style context dies on return from PropagateScrollToViewport but gets
   left in mRoots.

This last is the bug.  After that we'll crash the next time we try to mark.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #288613 - Flags: superreview?(dbaron)
Attachment #288613 - Flags: review?(dbaron)
OS: Mac OS X → All
Hardware: PC → All
Summary: Crash [@ nsRuleNode::Mark] with {ib}, :before, zooming → [FIX]Crash [@ nsRuleNode::Mark] with {ib}, :before, zooming
Target Milestone: --- → mozilla1.9 M10
Comment on attachment 288613 [details] [diff] [review]
Fix

r+sr=dbaron
Attachment #288613 - Flags: superreview?(dbaron)
Attachment #288613 - Flags: superreview+
Attachment #288613 - Flags: review?(dbaron)
Attachment #288613 - Flags: review+
Checked in the fix.  Still need to check in tests...
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Can this kind of thing be tested without changing kGCInterval?
I think so.  You just neeed to trigger kGCInterval style changes after the zoom.  A loop that sets style and flushes layout kGCInterval times would do it, I think.
Crash Signature: [@ nsRuleNode::Mark]
Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70de54da937c
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: