Closed
Bug 559491
Opened 14 years ago
Closed 14 years ago
Crash [@ nsStyleContext::Destroy]
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: jruderman, Assigned: tnikkel)
References
(Blocks 1 open bug)
Details
(Keywords: crash, testcase, Whiteboard: [sg:dos (critical w/out frame-poisoning)])
Crash Data
Attachments
(3 files)
No description provided.
Reporter | ||
Comment 1•14 years ago
|
||
Reporter | ||
Comment 2•14 years ago
|
||
bp-5fabdd31-84b7-4fc9-b8af-dbc0e2100414 seems to be missing some symbols compared to the Mac OS X crash report for a debug Firefox.
Comment 3•14 years ago
|
||
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
Comment 4•14 years ago
|
||
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
Comment 5•14 years ago
|
||
Worksforme, using: Mozilla/5.0 (Windows; U; Windows NT 6.1; nl; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Assignee | ||
Comment 6•14 years ago
|
||
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.
Assignee | ||
Comment 7•14 years ago
|
||
Assignee: nobody → tnikkel
Assignee | ||
Updated•14 years ago
|
Attachment #439714 -
Flags: review?(dbaron)
Comment 8•14 years ago
|
||
(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?
Assignee | ||
Comment 9•14 years ago
|
||
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 10•14 years ago
|
||
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+
Comment 11•14 years ago
|
||
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.
Assignee | ||
Comment 12•14 years ago
|
||
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?
Comment 13•14 years ago
|
||
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.
Assignee | ||
Comment 14•14 years ago
|
||
Ok, yeah, that makes sense.
Assignee | ||
Comment 16•14 years ago
|
||
I have not. With the current bandwidth situation I have been waiting to accumulate a few more patches to push at once.
Assignee | ||
Comment 17•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/bda1a3aff0d8
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
blocking2.0: ? → final+
Priority: -- → P1
Updated•13 years ago
|
Crash Signature: [@ nsStyleContext::Destroy]
Comment 18•10 years ago
|
||
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.
Description
•