1171970 - crash in OOM | large | NS_ABORT_OOM(unsigned int) | nsAString_internal::Assign(nsAString_internal const&) | mozilla::css::ErrorReporter::ReportUnexpected(char const*, nsString const&)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[(Away)]

This bug was filed from the Socorro interface and is 
report bp-77d163b1-b223-448a-b560-925652150530.
=============================================================

This is one of the top OOMs on 39 beta. The allocations tend to be 1-3 MB at the point of failure.

xul!NS_ABORT_OOM
xul!nsAString_internal::Assign
xul!mozilla::css::ErrorReporter::ReportUnexpected
xul!`anonymous namespace'::CSSParserImpl::ParseDeclaration
xul!`anonymous namespace'::CSSParserImpl::ParseDeclarationBlock
xul!`anonymous namespace'::CSSParserImpl::ParseSheet
xul!mozilla::css::Loader::ParseSheet
xul!mozilla::css::SheetLoadData::OnStreamComplete
xul!nsUnicharStreamLoader::OnStopRequest

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is happening on REPORT_UNEXPECTED_P(PEUnknownProperty, propertyName) in that crash-stats incident.

That lands us in the nsString overload of ErrorReporter::ReportUnexpected where we AddToError (this is where the crash happens).

I would _guess_ that the actual crash is on this bit:

  mErrorLine = mScanner->GetCurrentLine();

if we have a large stylesheet all on one line or something.  Hard to tell, since it appears AddToError got inlined.

Comment 2

[(Away)]

I want to say it's one of these:

ErrorReporter::AddToError(const nsString &aErrorText)
...
  } else {
    mError.AppendLiteral("  ");
    mError.Append(aErrorText);
  }

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, looks like some other incidents like https://crash-stats.mozilla.com/report/index/c220eabf-2355-4fa7-ac3d-ecb432150530 don't optimize out AddToError.  Or https://crash-stats.mozilla.com/report/index/4518fc51-5638-4ab9-a149-7b2252150529 for a more recent Firefox version (37).  Both are on that mErrorLine line.

I guess we could make that a fallible append, right?

Comment 4

[Boris Zbarsky [:bzbarsky]]

I don't really see a way that mError could get up into the multi-megabyte range.  We control that string, afaik, and we never put that much stuff in there.

Comment 5

[(Away)]

Oh, nope, it is indeed GetCurrentLine, at least in bp-5d5771a1-07db-495c-b9ef-884bc2150603. Funny enough it's the Assign for the operator=. Wonder if we could just take over the data (though GetCurrentLine could fail just as well).

Comment 6

[(Away)]

5d18ea0a e8ffa3beff      call    xul!nsCSSScanner::GetCurrentLine (5cd78e0e)
5d18ea0f 8d8e94000000    lea     ecx,[esi+94h]
5d18ea15 8bd0            mov     edx,eax
5d18ea17 e87b7cbeff      call    xul!nsAString_internal::Assign (5cd76697)
5d18ea1c 8b55fc          mov     edx,dword ptr [ebp-4]  <-- return address

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Handle super-long lines in CSS files a bit more gracefully if they cause OOM when creating CSS error messages

Comment 8

[Boris Zbarsky [:bzbarsky]]

> though GetCurrentLine could fail just as well

GetCurrentLine doesn't allocate. It just returns a dependent string pointing into an existing buffer.  So we're good there.

Comment 9

[(Away)]

Comment on attachment 8616000 [details] [diff] [review]
Handle super-long lines in CSS files a bit more gracefully if they cause OOM when creating CSS error messages

As of bug 1126593 you can just pass "fallible" if you prefer.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Handle super-long lines in CSS files a bit more gracefully if they cause OOM when creating CSS error messages

Ah, can and should.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a757393241c

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a757393241c

Comment 13

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Can this be uplifted to Firefox 40 (Beta now)?
It affects Firefox 39 user per bug # 1185289
crashlog report - https://crash-stats.mozilla.com/report/index/8df03784-f211-46c0-b916-3e4952150714
but probably it's not so critical to uplift it to stable.

Comment 14

[Cameron McCormack (:heycam)]

Yes, this is safe to uplift.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8616035 [details] [diff] [review]
Handle super-long lines in CSS files a bit more gracefully if they cause OOM when creating CSS error messages

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: OOM crashes with certain style sheet content
[Describe test coverage new/current, TreeHerder]: fix has been in 41 for about 5 weeks with no regression
[Risks and why]: quite low; changing a single string assignment to be fallible
[String/UUID change made/needed]: N/A

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8616035 [details] [diff] [review]
Handle super-long lines in CSS files a bit more gracefully if they cause OOM when creating CSS error messages

As stated in the approval request, this fix has been on 41 for 5 weeks. Let's get this into 40 beta6. Beta+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7a60da087850