Closed Bug 559491 Opened 10 years ago Closed 10 years ago

Crash [@ nsStyleContext::Destroy]

Categories

(Core :: CSS Parsing and Computation, defect, P1, critical)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: tnikkel)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, testcase, Whiteboard: [sg:dos (critical w/out frame-poisoning)])

Crash Data

Attachments

(3 files)

No description provided.
Attached file stack trace
bp-5fabdd31-84b7-4fc9-b8af-dbc0e2100414 seems to be missing some symbols compared to the Mac OS X crash report for a debug Firefox.
3.5.x might be safe. no crash for me on a vm with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.7) Gecko/20091221 Firefox/3.5.7
and no crash on another combination with 3.0.x and vista 
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.19) Gecko/2010031422 Firefox/3.0.19
Worksforme, using:
Mozilla/5.0 (Windows; U; Windows NT 6.1; nl; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
So the problem here is that we GC after destroying a root style context for an <a> element, but the corresponding visited style context doesn't get marked because it is not reachable from any root. So its rulenode gets collected and we crash. So I think we need to also add root visited style contexts to the list of roots.
Attached patch patchSplinter Review
Assignee: nobody → tnikkel
Blocks: 147777
Attachment #439714 - Flags: review?(dbaron)
(In reply to comment #6)
> So the problem here is that we GC after destroying a root style context for an
> <a> element, but the corresponding visited style context doesn't get marked
> because it is not reachable from any root. So its rulenode gets collected and
> we crash. So I think we need to also add root visited style contexts to the
> list of roots.

What's holding a pointer to the visited style context other than its own unvisited style context?  In other words, why does it need to get marked?
The unvisited style context is the one holding a reference to the visited style context but it doesn't drop the reference (and hence cause destruction) until after the body of ~nsStyleContext finishes executing and calls the destructor on the nsRefPtr<nsStyleContext> mStyleIfVisited member. But we GC in the middle of ~nsStyleContext for the unvisited style context when it calls nsStyleSet::NotifyStyleContextDestroyed.

If we wanted to cause destruction of the visited style context before the unvisited one by setting mStyleIfVisited to null in ~nsStyleContext we would have to guarantee no one else is holding a reference to the visited style context.
Comment on attachment 439714 [details] [diff] [review]
patch

ok, I guess this is ok, so r=dbaron.  It took me a little time to convince myself that it was sufficient, though.
Attachment #439714 - Flags: review?(dbaron) → review+
Actually, though, in addition to this patch, please *remove* the:

 if (mStyleIfVisited)
    mStyleIfVisited->Mark();

at the beginning of nsStyleContext::Mark.  It's no longer needed once you have this patch.
I don't understand why marking the visited style context is no longer needed. Wouldn't we still need to mark the visited style context for any non-root style context that has one?
No.

All style contexts own their parents.  Style contexts own their visited style contexts as well.  Any style context without a parent is a GC root.

So any visited style context has a parent:  that parent may or may not be a visited style context.  But either way, if you follow the parent chain up to the top, you hit something in mRoots.  So the mark phase works its way down the same way, and always finds the visited style context through its parent chain.
Ok, yeah, that makes sense.
Please don't forget to land this.
blocking2.0: --- → ?
I have not. With the current bandwidth situation I have been waiting to accumulate a few more patches to push at once.
http://hg.mozilla.org/mozilla-central/rev/bda1a3aff0d8
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
blocking2.0: ? → final+
Priority: -- → P1
Crash Signature: [@ nsStyleContext::Destroy]
Landed a crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/800da25d1788
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.