Closed
Bug 475128
Opened 16 years ago
Closed 16 years ago
GC old rule trees rather than deleting them
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b3
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Keywords: fixed1.9.0.7, fixed1.9.1)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
dveditz
:
approval1.9.0.7+
|
Details | Diff | Splinter Review |
I think we should garbage-collect old rule trees rather than just deleting them. We've seen crashes related to accessing the deleted rule tree because of other bugs show up a bit lately (a fixed example is bug 468645).
Flags: blocking1.9.1?
Assignee | ||
Comment 1•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Attachment #358540 -
Flags: superreview?(bzbarsky)
Attachment #358540 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•16 years ago
|
||
Er, I should also delete them in Shutdown, which means we fix bug 462392 as well (and thus I can put in the assertion in FreeToShell that I've had in my tree for ages).
Attachment #358540 -
Attachment is obsolete: true
Attachment #358543 -
Flags: superreview?(bzbarsky)
Attachment #358543 -
Flags: review?(bzbarsky)
Attachment #358540 -
Flags: superreview?(bzbarsky)
Attachment #358540 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 3•16 years ago
|
||
...er, should have said we fix bug 462788 as well.
Updated•16 years ago
|
Attachment #358543 -
Flags: superreview?(bzbarsky)
Attachment #358543 -
Flags: superreview+
Attachment #358543 -
Flags: review?(bzbarsky)
Attachment #358543 -
Flags: review+
Comment 4•16 years ago
|
||
Comment on attachment 358543 [details] [diff] [review]
patch
>+++ b/layout/style/nsStyleContext.cpp
>- // We don't need to clear out mRoots; NotifyStyleContextDestroyed
>- // will, and they're useful in EndReconstruct if they don't get
>- // completely cleared out.
Why remove that comment? Seems like it might still be good to make it clear that clearing mRoots here would be a _bad_ idea. Even worse than before this patch, no?
> nsStyleSet::EndReconstruct()
Do we want to assert that mOldRootTrees has length 1 here? Or are we figuring any case when it doesn't is caught by the other asserts?
>+nsStyleSet::GCRuleTree()
>+ // Mark the style context tree by marking all all style contexts which
s/all all/all/
>+++ b/layout/style/nsStyleSet.h
>+ void GCRuleTree();
Document? And maybe call it GCRuleTrees?
>+ // Old rule tree, which should only be non-empty between
s/tree/trees/ ?
cause
>+ // style contexts to exist to long, may last longer.
s/to long/too long/
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Whiteboard: [needs landing]
Assignee | ||
Comment 5•16 years ago
|
||
I landed this earlier today at http://hg.mozilla.org/mozilla-central/rev/24917a339f2e but then realized that it was causing a lot of assertions (I'd noticed earlier, and then forgotten). The assertions were pointing out a real bug; I'd effectively regressed the purpose of the GC. So I backed it out:
http://hg.mozilla.org/mozilla-central/rev/0cbd3800749f
http://hg.mozilla.org/mozilla-central/rev/f6b5ed8b3824
Here's a fixed patch. The fix is the additional change to nsRuleNode::Sweep. There's also a new nsStyleSet::GetRuleTree method to help it.
Attachment #358543 -
Attachment is obsolete: true
Attachment #359637 -
Flags: superreview?(bzbarsky)
Attachment #359637 -
Flags: review?(bzbarsky)
Updated•16 years ago
|
Attachment #359637 -
Flags: superreview?(bzbarsky)
Attachment #359637 -
Flags: superreview+
Attachment #359637 -
Flags: review?(bzbarsky)
Attachment #359637 -
Flags: review+
Comment 6•16 years ago
|
||
Comment on attachment 359637 [details] [diff] [review]
patch
Doh. Right.
Yay tests! ;)
Assignee | ||
Comment 7•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Comment 8•16 years ago
|
||
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
Assignee | ||
Updated•16 years ago
|
Attachment #359637 -
Flags: approval1.9.0.7?
Updated•16 years ago
|
Attachment #359637 -
Flags: approval1.9.0.7? → approval1.9.0.7+
Comment 9•16 years ago
|
||
Comment on attachment 359637 [details] [diff] [review]
patch
Approved for 1.9.0.7, a=dveditz for release-drivers.
Assignee | ||
Comment 10•16 years ago
|
||
Commited to CVS trunk (for 1.9.0.* releases), 2009-02-02 20:17 -0800.
Keywords: fixed1.9.0.7
Assignee | ||
Comment 11•16 years ago
|
||
NOTE to anybody doing 1.8.* backports: this patch does not apply to 1.8.* and is not relevant there since we did not do rule tree reconstructs back then; they were new in 1.9.*.
Flags: wanted1.8.1.x-
You need to log in
before you can comment on or make changes to this bug.
Description
•