Closed
Bug 1342289
Opened 7 years ago
Closed 7 years ago
stylo: memory leak when creating sheets via script, dom/html/crashtests/795221-3.html
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bradwerth, Assigned: bradwerth)
References
Details
Attachments
(2 files)
When caching is enabled in bug 1290218, Stylo leaks memory in crashtest dom/html/crashtests/795221-3.html.
Assignee | ||
Comment 1•7 years ago
|
||
Patch for Bug 1342869 appears to resolve this.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
(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.
Updated•7 years ago
|
Priority: -- → P2
Updated•7 years ago
|
Priority: P2 → P3
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8844556 -
Flags: review?(cam)
Attachment #8864345 -
Flags: review?(cam)
Comment 9•7 years ago
|
||
mozreview-review |
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.
Attachment #8844556 -
Flags: review?(cam) → review+
Comment 10•7 years ago
|
||
mozreview-review |
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
Attachment #8864345 -
Flags: review?(cam) → review+
Comment 11•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
Pushed by bwerth@mozilla.com: 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
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
Flags: needinfo?(bwerth)
Comment 17•7 years ago
|
||
Backout by kwierso@gmail.com: https://hg.mozilla.org/mozilla-central/rev/9064201fe655 Backed out 2 changesets for crashtest leaks a=backout
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
Pushed by bwerth@mozilla.com: 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/061521e9f929 https://hg.mozilla.org/mozilla-central/rev/2d18b28d3ec0
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bwerth)
You need to log in
before you can comment on or make changes to this bug.
Description
•