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

VERIFIED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P1
critical
VERIFIED FIXED
11 months ago
10 months ago

People

(Reporter: Ekanan Ketunuti, Assigned: jdm)

Tracking

(Blocks: 1 bug, {crash, regression})

56 Branch
mozilla56
Unspecified
Windows 10
crash, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 verified)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 months ago
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
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.
Blocks: 1293767
No longer blocks: 1375906
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.
(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
Oh, okay, I somehow overlooked that line...
That being said, I think we can make the dependencies code quite better memory-wise... Let me take a stab at it.
Assignee: nobody → emilio+bugs
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.
Flags: needinfo?(ananuti)
(Reporter)

Comment 7

11 months ago
(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.
(Reporter)

Comment 8

11 months ago
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. (-‸ლ)
(Reporter)

Updated

11 months ago
Blocks: 1352669
Flags: needinfo?(ananuti)
Keywords: regression
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)
Flags: needinfo?(josh)
(Reporter)

Comment 10

11 months ago
(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 :(
(Assignee)

Comment 11

11 months ago
Created attachment 8886288 [details] [diff] [review]
Share CSS source line between all same-line errors
Attachment #8886288 - Flags: review?(cam)
(Assignee)

Updated

11 months ago
Assignee: emilio+bugs → josh
Status: NEW → ASSIGNED
(Assignee)

Comment 12

11 months ago
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.
Flags: needinfo?(josh)
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.
Attachment #8886288 - Flags: review?(cam) → review+
(Assignee)

Comment 14

11 months ago
(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.
(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.)
Priority: -- → P1
(Assignee)

Comment 16

10 months ago
Created attachment 8886601 [details] [diff] [review]
Share CSS source line between all same-line errors
(Assignee)

Updated

10 months ago
Attachment #8886288 - Attachment is obsolete: true

Comment 17

10 months ago
Pushed by josh@joshmatthews.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03e414f78aef
Share CSS source line between all same-line errors. r=heycam

Comment 18

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/03e414f78aef
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Reporter)

Updated

10 months ago
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
status-firefox54: --- → unaffected
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.