Closed Bug 1368690 Opened 3 years ago Closed 3 years ago

Crash in mozilla::CSSStyleSheet::ClearRuleCascades

Categories

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

55 Branch
Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: marcia, Assigned: emilio)

References

Details

(4 keywords)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-1ce2ac49-2114-4409-b203-54eb50170530.
=============================================================

Seen while looking at nightly crash stats - crashes started using 20170530030204.

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cce4d83d2b99ffedbd67a2f40ce26e53e9ae27ab&tochange=286f71223256cbb3a769432fd860f563c4886e81

Bug 1352968 appears to be in the range. ni :xidorn
Flags: needinfo?(xidorn+moz)
Marking security sensitive due to the fact that some of the signatures look possibly exploitable.
Group: core-security
Group: core-security → layout-core-security
It is not clear to me why bug 1352968 can cause this issue. It seems printing involves the crash, and I cannot reproduce this issue with some random test with printing.

I don't have access to details of the crash report. It would be great if someone can provide some urls related to those crash reports.
Marcia, could you provide some details for the crash? Or do you know how can I reproduce it?
Flags: needinfo?(xidorn+moz) → needinfo?(mozillamarcia.knous)
Also cc emilio and heycam, since I would suspect more on bug 1357583 rather than bug 1352968, because the former directly touches the function CSSStyleSheet::ClearRuleCascades.
Patch [1] seems to have introduced this regression. This signature is ranked #5 in content top crashes.
:emilio, could you investigate ?

[1] https://hg.mozilla.org/mozilla-central/rev?node=3deb86de58ce1103d3ec566bcbeee886aafc9cc0
Blocks: 1357583
Flags: needinfo?(emilio+bugs)
Keywords: regression, topcrash
I'm looking, thanks.
Flags: needinfo?(emilio+bugs)
I don't see any way my patch could introduce this crash, I doubt it's the one that caused the regression... But I'm still investigating regardless.

A way to repro it would make it easier though...
Ok, I think I know what's going on. This was introduced in bug 1339629, and it's probably a dupe of bug 1368958.

In particular, we're calling EnsureUniqueInner in the copy-constructor of the _base_ class of the stylesheet, and that calls ClearRuleCascades, which was a virtual function (which now is a call to the parent class function).

This is totally unsafe, because since we're running on the base class constructor, the members of the super-class aren't initialized yet. I'm not sure how ASAN or something like that hasn't caught it on CI... I guess the tests we run under ASAN may not exercise this code?

Anyway, will post a patch soon.
Blocks: 1339629
No longer blocks: 1357583
Comment on attachment 8872947 [details] [diff] [review]
0001-Bug-1368690-Move-EnsureUniqueInner-call-after-all-th.patch

Review of attachment 8872947 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/CSSStyleSheet.cpp
@@ +365,5 @@
> +  if (mDirty) { // CSSOM's been there, force full copy now
> +    NS_ASSERTION(mInner->mComplete, "Why have rules been accessed on an incomplete sheet?");
> +    // FIXME: handle failure?
> +    //
> +    // NOTE: It's important to call this from the super-class, since this calls

from *subclass* I think. "super-class" is usually the synonym of base class.

::: layout/style/ServoStyleSheet.cpp
@@ +92,5 @@
> +    NS_ASSERTION(mInner->mComplete, "Why have rules been accessed on an incomplete sheet?");
> +    // FIXME: handle failure?
> +    //
> +    // NOTE: It's important to call this from the super-class, since this calls
> +    // ClearRuleCascades(), which would access uninitialized members otherwise.

EnsureUniqueInner doesn't call ClearRuleCascades for ServoStyleSheet, does it? Maybe it can be a more general statement like, "since this may access uninitialized members of subclass otherwise".
Attachment #8872947 - Flags: review?(xidorn+moz) → review+
Comments addressed, and...:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b0b1edd128f
Assignee: nobody → emilio+bugs
Flags: needinfo?(mozillamarcia.knous)
https://hg.mozilla.org/mozilla-central/rev/8b0b1edd128f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Duplicate of this bug: 1369813
Group: layout-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.