1368690 - Crash in mozilla::CSSStyleSheet::ClearRuleCascades

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Marcia Knous [:marcia]]

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

Comment 1

[Marcia Knous [:marcia]]

Marking security sensitive due to the fact that some of the signatures look possibly exploitable.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Marcia, could you provide some details for the crash? Or do you know how can I reproduce it?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 5

[Calixte Denizet (:calixte)]

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

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I'm looking, thanks.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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...

Comment 8

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 9

[Calixte Denizet (:calixte)]

In the report [1], the user (under OS X 10.12) said that it was trying to print a pdf [2].

[1] https://crash-stats.mozilla.com/report/index/217f5278-0474-4679-b228-7dad00170531
[2] http://pmt.physicsandmathstutor.com/download/Physics/A-level/Topic-Qs/AQA/03-Waves/Set-A/Diffraction.pdf

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: 0001-Bug-1368690-Move-EnsureUniqueInner-call-after-all-th.patch

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

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".

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comments addressed, and...:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b0b1edd128f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b0b1edd128f