1342289 - stylo: memory leak when creating sheets via script, dom/html/crashtests/795221-3.html

Status: RESOLVED FIXED

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

Description

[Brad Werth [:bradwerth]]

When caching is enabled in bug 1290218, Stylo leaks memory in crashtest dom/html/crashtests/795221-3.html.

Comment 1

[Brad Werth [:bradwerth]]

Patch for Bug 1342869 appears to resolve this.

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1342289 Part 1: Generalize Loader to cycle collect all StyleSheets, not just CSSStyleSheets.

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

Comment 3

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #1)
> Patch for Bug 1342869 appears to resolve this.

Not, in fact, resolved by bug 1342869. The pathology of this leak is that the testcase triggers creation of a nsGlobalWindow at https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2956. In this testcase, both the parent and the new inner window are not cycle collected and therefore the nsLayoutStatics are not Released enough times and all the statics leak.

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 1342289 Part 2: Re-enable crashtest dom/html/crashtests/795221-3.html.

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

Comment 5

[Brad Werth [:bradwerth]]

Comment on attachment 8844556 [details]
Bug 1342289 Part 1: Generalize Loader to cycle collect all StyleSheets, not just CSSStyleSheets.

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

Comment 6

[Brad Werth [:bradwerth]]

Comment on attachment 8844556 [details]
Bug 1342289 Part 1: Generalize Loader to cycle collect all StyleSheets, not just CSSStyleSheets.

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

Comment 7

[Brad Werth [:bradwerth]]

Comment on attachment 8864345 [details]
Bug 1342289 Part 2: Re-enable crashtest dom/html/crashtests/795221-3.html.

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

Comment 8

[Brad Werth [:bradwerth]]

Comment on attachment 8864345 [details]
Bug 1342289 Part 2: Re-enable crashtest dom/html/crashtests/795221-3.html.

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

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8844556 [details]
Bug 1342289 Part 1: Generalize Loader to cycle collect all StyleSheets, not just CSSStyleSheets.

https://reviewboard.mozilla.org/r/117938/#review145030

::: dom/html/crashtests/795221-3.html:11
(Diff revision 3)
>    // cycle through, thus accidentally avoiding the cycle
>    var link = document.createElement("link");
>    link.setAttribute("rel", "stylesheet");
>    link.setAttribute("href", "data:text/css,div { }");
>    link.onload = function () {
> -    document.styleSheets[0].cssRules[0].style.foo = document;
> +    //document.styleSheets[0].cssRules[0].style.foo = document;

I guess you don't want to check this change in?

::: layout/style/ServoStyleSheet.cpp:76
(Diff revision 3)
>    DropRuleList();
>  }
>  
>  // QueryInterface implementation for ServoStyleSheet
>  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(ServoStyleSheet)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMCSSStyleSheet)

It's this line that fixes the bug, right?  I don't see any calls like

  RefPtr<ServoStyleSheet> servoSheet = do_QueryObject(sheet);

which is what the CID stuff enables.

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8864345 [details]
Bug 1342289 Part 2: Re-enable crashtest dom/html/crashtests/795221-3.html.

https://reviewboard.mozilla.org/r/135992/#review145034

Comment 11

[Cameron McCormack (:heycam)]

One of the uses of this dynamic casting to a concrete sheet classes is in inDOMUtils::ParseStyleSheet.  Tommy is working on that in bug 1358993, so he might use this.

Comment 12

[Brad Werth [:bradwerth]]

Comment on attachment 8844556 [details]
Bug 1342289 Part 1: Generalize Loader to cycle collect all StyleSheets, not just CSSStyleSheets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117938/diff/3-4/

Comment 13

[Brad Werth [:bradwerth]]

Comment on attachment 8864345 [details]
Bug 1342289 Part 2: Re-enable crashtest dom/html/crashtests/795221-3.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135992/diff/3-4/

Comment 14

[Brad Werth [:bradwerth]]

Comment on attachment 8844556 [details]
Bug 1342289 Part 1: Generalize Loader to cycle collect all StyleSheets, not just CSSStyleSheets.

https://reviewboard.mozilla.org/r/117938/#review145030

> It's this line that fixes the bug, right?  I don't see any calls like
> 
>   RefPtr<ServoStyleSheet> servoSheet = do_QueryObject(sheet);
> 
> which is what the CID stuff enables.

Yeah, my confusion was that since this was a crashtest, I assumed that the cause of the crash was the feature actually probed by the test. But, when ServoStyleSheet became a subclass of StyleSheet, the "document.styleSheets" property triggered a new crash in ServoStyleSheet::QueryInterface. So the crash and leak was triggered by the property reference in JS.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0a029765f69
Part 1: Define UUID for ServoStyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3744676f7923
Part 2: Re-enable crashtest dom/html/crashtests/795221-3.html. r=heycam

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/4d13294ef068adfab6e36d969113f74b8aa7dbc3 for stylo crashtest leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=101000698&repo=autoland

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/9064201fe655
Backed out 2 changesets for crashtest leaks a=backout

Comment 18

[Brad Werth [:bradwerth]]

Comment on attachment 8844556 [details]
Bug 1342289 Part 1: Generalize Loader to cycle collect all StyleSheets, not just CSSStyleSheets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117938/diff/4-5/

Comment 19

[Brad Werth [:bradwerth]]

Comment on attachment 8864345 [details]
Bug 1342289 Part 2: Re-enable crashtest dom/html/crashtests/795221-3.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135992/diff/4-5/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/061521e9f929
Part 1: Generalize Loader to cycle collect all StyleSheets, not just CSSStyleSheets. r=heycam
https://hg.mozilla.org/integration/autoland/rev/2d18b28d3ec0
Part 2: Re-enable crashtest dom/html/crashtests/795221-3.html. r=heycam

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/061521e9f929
https://hg.mozilla.org/mozilla-central/rev/2d18b28d3ec0