Closed Bug 475128 Opened 13 years ago Closed 13 years ago

GC old rule trees rather than deleting them

Categories

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

defect

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)

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?
Attached patch patch (obsolete) — Splinter Review
Attachment #358540 - Flags: superreview?(bzbarsky)
Attachment #358540 - Flags: review?(bzbarsky)
Attached patch patch (obsolete) — Splinter Review
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)
...er, should have said we fix bug 462788 as well.
Blocks: 462788
Attachment #358543 - Flags: superreview?(bzbarsky)
Attachment #358543 - Flags: superreview+
Attachment #358543 - Flags: review?(bzbarsky)
Attachment #358543 - Flags: review+
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
Blocks: 469432
Attached patch patchSplinter Review
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)
Attachment #359637 - Flags: superreview?(bzbarsky)
Attachment #359637 - Flags: superreview+
Attachment #359637 - Flags: review?(bzbarsky)
Attachment #359637 - Flags: review+
Comment on attachment 359637 [details] [diff] [review]
patch

Doh.  Right.

Yay tests!  ;)
http://hg.mozilla.org/mozilla-central/rev/3b2f69cc7004
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Depends on: 473871
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ada235e2d225
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
Attachment #359637 - Flags: approval1.9.0.7?
Attachment #359637 - Flags: approval1.9.0.7? → approval1.9.0.7+
Comment on attachment 359637 [details] [diff] [review]
patch

Approved for 1.9.0.7, a=dveditz for release-drivers.
Commited to CVS trunk (for 1.9.0.* releases), 2009-02-02 20:17 -0800.
Keywords: fixed1.9.0.7
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-
Blocks: 454276
You need to log in before you can comment on or make changes to this bug.