423521 - improve XUL template CC traversal

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Robert Sayre]

there's a bunch of stuff in here we can add

Comment 1

[Robert Sayre]

Attachment: xul templates (wip)

Comment 2

[Robert Sayre]

Comment on attachment 310072 [details] [diff] [review]
xul templates (wip)

Nothing wrong with this code. We should take it.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Have you checked that the classes can deal with all those members being null. Often large parts of the code can get executed during teardown so it needs to be prepared for the various members having been unlinked.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 310072 [details] [diff] [review]
xul templates (wip)

So I've been running with this patch for a while without any crashes and it all looks ok to me.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Still want answer to comment 3 before sr'ing

Comment 6

[Peter Van der Beken [:peterv]]

I just took a quick look, it doesn't make sense to note objects as children if they don't participate in cycle collection (for example mEvaluator in nsXULTemplateQueryProcessorXML). You're also noting some objects that should probably be cycle collected, but I'd rather start noting them when we add traversal code to them (for example mBoxObject in nsXULTreeBuilder), but it'll fan out quickly then (plenty of other non-participating objects hold box objects for example).

And as Jonas said, more unlinking can be scary. It's ok if you've verified that all code can deal with those objects being null, the reason why our unlink code doesn't match the traverse code is exactly because we didn't do that (tedious) checking.

Comment 7

[Robert Sayre]

(In reply to comment #6)
> You're also noting some objects that should
> probably be cycle collected, but I'd rather start noting them when we add
> traversal code to them 

Definitely did that. Your approach makes sense.

> And as Jonas said, more unlinking can be scary. It's ok if you've verified that
> all code can deal with those objects being null

Evaluating the unlink methods required reading the teardown methods these objects have, so I did a decent job there, I think. Will take another pass at this.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 310072 [details] [diff] [review]
xul templates (wip)

Resetting request waiting for more information (comment 3)

Comment 9

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.