1380488 - Stylo: Gmail crash in alloc::oom::default_oom_handler | alloc::oom::oom

Status: VERIFIED FIXED

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

Description

[Ekanan Ketunuti]

This bug was filed from the Socorro interface and is 
report bp-bf883567-53df-417c-a3a0-dd09f0170712.
=============================================================

STR: Open https://mail.google.com/mail/u/0/#inbox

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This is an OOM in Win 32 bit AFAICT... I guess it depends on the system, and what was the user doing at that time...

But yeah, we should measure memory usage and ensure we don't regress it a lot.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

It seems to me that we never remove items from Stylist.selectors_for_cache_revalidation. Maybe it is intentional. Not sure whether this could lead to any problem. Probably not a bit deal in general... unless a page endlessly loads different stylesheets with different content inside.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
> It seems to me that we never remove items from
> Stylist.selectors_for_cache_revalidation.

We do right here, no? http://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/servo/components/style/stylist.rs#331

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Oh, okay, I somehow overlooked that line...

Comment 5

[Emilio Cobos Álvarez (:emilio)]

That being said, I think we can make the dependencies code quite better memory-wise... Let me take a stab at it.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

https://github.com/servo/servo/pull/17707 should improve this a bit. If the OOM was consistent, could you try again with the next nightly? Otherwise, I don't think we can do much more in this bug than tracking other general memory usage issues like bug 1293767, so we should probably close it.

Comment 7

[Ekanan Ketunuti]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> https://github.com/servo/servo/pull/17707 should improve this a bit. If the
> OOM was consistent, could you try again with the next nightly?

will do when rev 6177c907f345 merged into nightly. leaving ni as reminder for now.

Comment 8

[Ekanan Ketunuti]

fyi, this oom was first seen in 20170712 nightly.

Bisect to:

last good: autoland changeset d047630ee1ff
first bad: autoland changeset 81adbdebbfe3
log: https://hg.mozilla.org/integration/autoland/log?rev=d047630ee1ff%3A%3A81adbdebbfe3%20and%20!d047630ee1ff

jdm made this crash. (－‸ლ)

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Hmm... Thanks for the bisection!

Josh, could you take a look at this? Maybe we're leaving CSS errors stored in the reporter for too long, or seeing how could we mitigate this somehow?

(Still I think my patch above should help a fair bit)

Comment 10

[Ekanan Ketunuti]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> https://github.com/servo/servo/pull/17707 should improve this a bit. If the
> OOM was consistent, could you try again with the next nightly? Otherwise, I
> don't think we can do much more in this bug than tracking other general
> memory usage issues like bug 1293767, so we should probably close it.

Still crash with this build. bp-ab8637de-d018-45a0-8795-6500b0170713 :(

Comment 11

[Josh Matthews [:jdm]]

Attachment: Share CSS source line between all same-line errors

Comment 12

[Josh Matthews [:jdm]]

I have not been able to reproduce the problem, but it seems clear to me that the previous behaviour of creating a new UTF-16 version of the current CSS line for each error reported is not a good idea, especially when minimized CSS is so common.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8886288 [details] [diff] [review]
Share CSS source line between all same-line errors

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

::: layout/style/ErrorReporter.cpp
@@ +267,5 @@
> +    if (!AppendUTF8toUTF16(aSourceLine, mErrorLine, fallible)) {
> +      mErrorLine.Truncate();
> +    }
> +
> +    mPrevErrorLineNumber = aLineNumber;

(Since we seem to output a single error at a time in Gecko_ReportUnexpectedCSSError, regardless of whether a sequence of errors happen on the same line number, we don't really need to be updating/checking mPrevErrorLineNumber for an ErrorReporter for Stylo.  Although maybe we should be.  Still, it doesn't hurt.)

@@ +288,5 @@
>    if (mError.IsEmpty()) {
>      mError = aErrorText;
> +    // If this error reporter is being used from Stylo, the equivalent operation occurs
> +    // in the OutputError variant that provides source information.
> +    if (mScanner) {

Can you add a helper method IsServo() that just returns whether mScanner is null?  I think that might make it clearer that that's what we're switching on here.

Could you also make the ErrorReporter::ErrorReporter constructor that takes a sheet argument take it as a ServoStyleSheet, since we don't seem to call it from elsewhere, and would help us rely on mScanner being null meaning we're an ErrorReporter for Stylo.

Comment 14

[Josh Matthews [:jdm]]

(In reply to Cameron McCormack (:heycam) from comment #13)
> Comment on attachment 8886288 [details] [diff] [review]
> Share CSS source line between all same-line errors
> 
> Review of attachment 8886288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/ErrorReporter.cpp
> @@ +267,5 @@
> > +    if (!AppendUTF8toUTF16(aSourceLine, mErrorLine, fallible)) {
> > +      mErrorLine.Truncate();
> > +    }
> > +
> > +    mPrevErrorLineNumber = aLineNumber;
> 
> (Since we seem to output a single error at a time in
> Gecko_ReportUnexpectedCSSError, regardless of whether a sequence of errors
> happen on the same line number, we don't really need to be updating/checking
> mPrevErrorLineNumber for an ErrorReporter for Stylo.  Although maybe we
> should be.  Still, it doesn't hurt.)

I'm not sure what you mean by this - we can report multiple errors that use the same source line (such as from minimized CSS), so we need to keep track of when we should update the current source line.

Comment 15

[Cameron McCormack (:heycam)]

(In reply to Josh Matthews [:jdm] from comment #14)
> I'm not sure what you mean by this - we can report multiple errors that use
> the same source line (such as from minimized CSS), so we need to keep track
> of when we should update the current source line.

Sorry, yes you're right that we need to keep that updated so that we don't create distinct strings for the same source line.  (I was confusing myself with ErrorReporter's behaviour of building up multiple errors per line in mError until an OutputError call, and how nsCSSParser (for example) won't call OutputError after each ReportXXX call, but at some logical points such as after parsing one declaration.)

Comment 16

[Josh Matthews [:jdm]]

Attachment: Share CSS source line between all same-line errors

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03e414f78aef
Share CSS source line between all same-line errors. r=heycam

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/03e414f78aef