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.

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.

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.

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

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. (－‸ლ)

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)

(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 :(

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

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

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