Startup crash in v41 nsStyleSet::GatherRuleProcessors(nsStyleSet::sheetType) possibly related to Yandex toolbar *and* Adblock Plus
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
People
(Reporter: away, Assigned: heycam)
References
Details
(Keywords: crash)
Crash Data
Attachments
(1 file)
|
1.12 KB,
patch
|
dbaron
:
review+
ritu
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
ritu
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
|
||
Comment 4•9 years ago
|
||
|
|
||
Comment 6•9 years ago
|
||
|
Assignee | |
Comment 8•9 years ago
|
||
|
|
Assignee | |
Comment 9•9 years ago
|
||
|
|
Assignee | |
Comment 10•9 years ago
|
||
|
|
Assignee | |
Comment 11•9 years ago
|
||
|
|
Assignee | |
Comment 13•9 years ago
|
||
|
|
Assignee | |
Comment 14•9 years ago
|
||
|
|
Assignee | |
Comment 15•9 years ago
|
||
|
||
Comment 16•9 years ago
|
||
|
||
Comment 17•9 years ago
|
||
|
|
Reporter | |
Comment 18•9 years ago
|
||
|
||
Comment 25•9 years ago
|
||
|
|
||
Comment 29•9 years ago
|
||
|
||
Comment 30•9 years ago
|
||
|
||
Comment 31•9 years ago
|
||
|
|
||
Comment 32•9 years ago
|
||
|
|
||
Comment 33•9 years ago
|
||
|
||
Comment 34•6 years ago
|
||
I've spent a few hours chasing this bug down and unfortunately the classifications here in the bug tracker made it an extremely difficult ordeal.
- This bug is (only?) triggered via recursive observer notifications pertaining to document stylesheets, which can occur when a stylesheet is loaded in an idiosyncratic fashion, almost certainly only if the observation is triggered by mutation of a
<style>...</style>or<link rel="stylesheet" ... />element by an asynchronously executed script, - The crash isn't the actual bug here, it's brought on by an infinite loop resulting in 100% CPU and continuous memory ballooning, probably leading to an allocation failure that ultimately results in
mRuleTreebeingnull, - This bug may be surfaced in the Yandex Toolbar or the AdBlock Plus code but has nothing to do with them (a class of AdBlock rules work by injecting style rules into the document to hide ad elements, triggered on page request (based off the URL) independent of the content (i.e. asynchronously prior to the load event),
- This bug is not fixed in Firefox 41. The partial patch that was backported to Firefox 41 was insufficient to address the issue, and this bug reproduces in Firefox 41.0.2,
- This bug was not introduced in Firefox 41; I can reproduce it going back to at least Firefox 25 (although perhaps the symptoms caught and reported by the telemetrics differ based off of what happens after the memory explodes),
- For the record, this bug last reproduces in Firefox 42.0 beta 2 and is resolved in Firefox 42.0 beta 3 and above.
The status of this bug being incorrectly marked as fixed in Firefox 41 meant that it did not appear in any bugzilla queries for bugs affecting Firefox 41 and fixed in Firefox 42, so I needed to manually bisect archived release builds then track down the relevant changesets (only(GECKO420b3_2015100117_RELBRANCH, GECKO420b2_2015092813_RELBRANCH), for the record).
The bug is easily reproducible with the following code:
function loadCss(url: string) {
return new Promise((resolve, reject) => {
const link = document.createElement("link");
link.rel = "stylesheet";
link.type = "text/css";
link.href = url;
// We know that this document contains no styles currently in use,
// so allow this to be loaded asynchronously without delaying page
// load by giving it a `media` other than `screen`:
link.media = "notscreen";
link.addEventListener("load", function () {
// Trigger actually loading the stylesheet:
link.media = "screen";
resolve();
});
link.addEventListener("error", reject);
head.appendChild(link);
});
}
loadCss("path/to/file.css");
started asynchronously on document request (transpiled as needed depending on the version of Firefox you intend to target).
In case it's not obvious what causes the recursive observer notification, it's the link.media = "screen"; line being unconditionally executed. The change of link.media from screen to screen (on second+ invocation) is (falsely) detected as a change and results in this crash. The following code is not affected by this issue:
function loadCss(url: string) {
return new Promise((resolve, reject) => {
const link = document.createElement("link");
link.rel = "stylesheet";
link.type = "text/css";
link.href = url;
// We know that this document contains no styles currently in use,
// so allow this to be loaded asynchronously without delaying page
// load by giving it a `media` other than `screen`:
link.media = "notscreen";
link.addEventListener("load", function () {
// Trigger actually loading the stylesheet:
if (link.media !== "screen") {
link.media = "screen";
}
resolve();
});
link.addEventListener("error", reject);
head.appendChild(link);
});
}
loadCss("path/to/file.css");
To the best of my knowledge all other contemporaries of the affected Firefox versions handle this correctly (including versions of IE and Chrome from the same era), and I'm unsure whether or the issue with recursive invocations of the notification observer still persists in current Firefox (with the fallout handled per the changesets targeting this bug).
(Of course all this could be a separate bug, but the stack trace in the error reports indicates it's unlikely to be the case.)
|
|
||
Comment 35•6 years ago
|
||
Comment 34 seems to be describing bug 1203973.
That is not the same as this bug.
probably leading to an allocation failure that ultimately results in mRuleTree being null
Allocation failure for mRuleTree resulted in an immediate process abort going back to at least Firefox 18 (see bug 809533). So no, this is not what happened.
But most importantly, this bug is about a crash during startup. The code in comment 34 is not terribly likely to be running at startup...
|
|
||
Comment 36•6 years ago
|
||
Bug 1203973 is marked as being fixed in Firefox 18, so either it was reintroduced between then and Firefox 25 or it's not the same issue.
I'm not sure why the code in comment 34 can't have been used by the Yandex toolbar and the AdBlock Plus plugin at startup (although I confess not knowing at which point in startup this crash was happening, which does make a difference). If it's at startup prior to requesting any pages (even a local start tab or start page); as I understand it, XUL can use stylesheets for skinning, and who knows what the Yandex plugin loaded (in the background?) at startup.
I was not implying that the allocation of mRuleTree itself was erroring, but rather that an unhandled (or incorrectly handled) allocation failure in the code leading to its initialization/assignment perhaps resulted in that state. But I am nowhere near being familiar enough with the codebase to definitively state that's the case.
|
|
||
Comment 37•6 years ago
|
||
btw, the code in comment 34 can likely (untested) be replicated via simply the following:
<link rel="stylesheet" type="text/css" media="notscreen" href="path/to/file.css" onload="this.media='screen'" />
which is a lot less arcane and webapp-y.
|
|
||
Comment 38•6 years ago
|
||
Bug 1203973 is marked as being fixed in Firefox 18
No, it's marked fixed in Firefox 42, which matches your conclusions about when your testcase was fixed. Did you accidentally look at the resolution information for bug 809533 instead?
I'm not sure why the code in comment 34 can't have been used by the Yandex toolbar and the AdBlock Plus plugin at startup
It could have, but then the stack would show the recursion, and it's not showing that.
but rather that an unhandled (or incorrectly handled) allocation failure in the code
It's possible in theory, but for what it's worth allocation failure is generally immediately fatal in Gecko and has been for a long time (dating back to before Firefox 20).
|
|
||
Comment 39•6 years ago
|
||
No, it's marked fixed in Firefox 42, which matches your conclusions about when your testcase was fixed. Did you accidentally look at the resolution information for bug 809533 instead?
You're right, that's exactly what happened.
Mea culpa, I stand corrected.
Description
•