Closed Bug 1171970 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

39 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 --- wontfix
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: away, Assigned: bzbarsky)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
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.
Component: Layout → CSS Parsing and Computation
I want to say it's one of these:

ErrorReporter::AddToError(const nsString &aErrorText)
...
  } else {
    mError.AppendLiteral("  ");
    mError.Append(aErrorText);
  }
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?
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.
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).
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
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
> 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 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.
Attachment #8616000 - Attachment is obsolete: true
Attachment #8616000 - Flags: review?(cam)
Attachment #8616035 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/9a757393241c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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.
Flags: needinfo?(cam)
Flags: needinfo?(bzbarsky)
Yes, this is safe to uplift.
Flags: needinfo?(cam)
Flags: needinfo?(bzbarsky)
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
Attachment #8616035 - Flags: approval-mozilla-beta?
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+
Attachment #8616035 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.