1326167 - stylo: dom/html/crashtests/795221-1.html leaks

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

One of the crashtest leaks is from dom/html/crashtests/795221-1.html (and its friends), which creates a cycle by setting an expando property on a ServoStyleRule's declaration.

Xidorn, did we get something wrong with the cycle collector stuff there?

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

I think the issue here is actually that I didn't hook ServoCSSRuleList into cycle collection. Gecko's CSSRuleList doesn't need special handling because it doesn't own the rules, but ServoCSSRuleList owns the rules, so it does need to trace them.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1326167 - Hook ServoStyleSheet and ServoCSSRuleList into cycle collection.

Review commit: https://reviewboard.mozilla.org/r/101444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101444/

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8822582 [details]
Bug 1326167 - Hook ServoStyleSheet and ServoCSSRuleList into cycle collection.

Looks simple enough, but bz was recently poking around at the object model stuff, so he should probably take a look.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8822582 [details]
Bug 1326167 - Hook ServoStyleSheet and ServoCSSRuleList into cycle collection.

https://reviewboard.mozilla.org/r/101444/#review103218

r=me

::: layout/style/ServoCSSRuleList.cpp:40
(Diff revision 1)
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ServoCSSRuleList)
> +  for (uintptr_t& rule : tmp->mRules) {
> +    if (rule > kMaxRuleType) {
> +      CastToPtr(rule)->Release();
> +      // XXX Do we need to restore the type value here? Is it possible

Probably safest to set the type to 0, yes.  Someone else could poke at it during their own unlinking process...

::: layout/style/ServoCSSRuleList.cpp:48
(Diff revision 1)
> +    }
> +  }
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END_INHERITED(dom::CSSRuleList)
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(ServoCSSRuleList,
> +                                                  dom::CSSRuleList)
> +  tmp->EnumerateInstantiatedRules([&](css::Rule* aRule) {

Does this compile down to something reasonable that doesn't involve lots of function calls in practice?

::: layout/style/ServoCSSRuleList.cpp:49
(Diff revision 1)
> +  }
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END_INHERITED(dom::CSSRuleList)
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(ServoCSSRuleList,
> +                                                  dom::CSSRuleList)
> +  tmp->EnumerateInstantiatedRules([&](css::Rule* aRule) {
> +    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "rule");

I think "mRules[i]" would be a closer match for how we normally represent arrays as CC edge names.

::: layout/style/ServoCSSRuleList.cpp:50
(Diff revision 1)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END_INHERITED(dom::CSSRuleList)
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(ServoCSSRuleList,
> +                                                  dom::CSSRuleList)
> +  tmp->EnumerateInstantiatedRules([&](css::Rule* aRule) {
> +    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "rule");
> +    cb.NoteXPCOMChild(aRule->GetExistingDOMRule());

This is going to conflict with the patches in bug 851892 (which remove GetExistingDOMRule), but merging should be easy enough.  Just check that it hasn't landed before you land this.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8822582 [details]
Bug 1326167 - Hook ServoStyleSheet and ServoCSSRuleList into cycle collection.

https://reviewboard.mozilla.org/r/101444/#review103218

> Does this compile down to something reasonable that doesn't involve lots of function calls in practice?

I... don't know. I simply trust that the compiler would do the right thing... and I'm not sure what is the easist way to check that :/

Comment 6

[Boris Zbarsky [:bzbarsky]]

Step through it and see what the assembly actually looks like?  :(

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

So for VS2015, the compiler inlines the lambda into the instantiated template, but the template function isn't inlined into the traverse function. I guess that is fine as far as we don't do a function call for each rule for traversing.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8822582 [details]
Bug 1326167 - Hook ServoStyleSheet and ServoCSSRuleList into cycle collection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101444/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8f9f36068ef
Hook ServoStyleSheet and ServoCSSRuleList into cycle collection. r=bz

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I guess that is fine as far as we don't do a function call for each rule

Yes, that's the key part.  Thank you for checking!

Comment 11

[Iris Hsiao [:ihsiao]]

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