475128 - GC old rule trees rather than deleting them

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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).

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

...er, should have said we fix bug 462788 as well.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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/

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 359637 [details] [diff] [review]
patch

Doh.  Right.

Yay tests!  ;)

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/3b2f69cc7004

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ada235e2d225

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 359637 [details] [diff] [review]
patch

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Commited to CVS trunk (for 1.9.0.* releases), 2009-02-02 20:17 -0800.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.*.