1373725 - stylo: Rule tree never gets GCed until teardown

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Bobby Holley (:bholley)]

Brian Lewis discovered this in [1]. Thanks Brian!

Working up a patch now.

[1] https://github.com/servo/servo/issues/17280

Comment 1

[Bobby Holley (:bholley)]

Oh, and on top of that, it looks like we never actually call maybe_gc(). Adding a patch for that as well...

Comment 2

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83d058d26ec45d43916e8cd91c8c55fc78ce40f4

Comment 3

[Bobby Holley (:bholley)]

And here's a try run with a bonus fix to assert against permanently leaking rule nodes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd0ca5e6e654f638f3bceafb56ee6e9d566e7006

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Actually increment the counter when adding rule nodes to the free list. v1

MozReview-Commit-ID: 1uipYlIF8fv

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Reuse the prev_sibling slot for free_count to save a word. v1

MozReview-Commit-ID: 9jVkDM4P8mC

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Trigger a rule tree gc at the end of DoProcessPendingRestyles. v1

MozReview-Commit-ID: W2lkQohudA

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Assert against permanently-leaked rule nodes. v1

MozReview-Commit-ID: CK5iEWWHFSr

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8878636 [details] [diff] [review]
Part 1 - Actually increment the counter when adding rule nodes to the free list. v1

Review of attachment 8878636 [details] [diff] [review]:
-----------------------------------------------------------------

Pfft... good catch :)

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8878638 [details] [diff] [review]
Part 3 - Trigger a rule tree gc at the end of DoProcessPendingRestyles. v1

Review of attachment 8878638 [details] [diff] [review]:
-----------------------------------------------------------------

I'm pretty sure we called this when it was added, I wonder when was it removed...

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8878639 [details] [diff] [review]
Part 4 - Assert against permanently-leaked rule nodes. v1

Review of attachment 8878639 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Ick, of course I was the one that removed it... https://github.com/servo/servo/commit/e67ea42c3fa03264b8c897430f7c17a7c153e5bd

Comment 12

[Bobby Holley (:bholley)]

https://github.com/servo/servo/pull/17368

Comment 13

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/autoland/rev/1f216a97ac0633e2ffd5f9af2893e4610f7206f7

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a21be24aa822
Trigger a rule tree gc at the end of DoProcessPendingRestyles. r=emilio

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a21be24aa822