Closed Bug 682595 Opened 8 years ago Closed Last year

OOM crash in the CSS parser

Categories

(Core :: Layout, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED WONTFIX

People

(Reporter: bjacob, Unassigned)

References

Details

(Keywords: crash)

Crash Data

I was just looking at the list of top Linux / Firefox9.0a1 crashers. This one is ranking 23th... with just 2 crashes last week ;-)

https://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A9.0a1&platform=linux&query_search=signature&query_type=contains&reason_type=contains&date=&range_value=1&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozalloc_abort%20%7C%20mozalloc_handle_oom%20%7C%20moz_xmalloc

Here's one of the crash reports:

https://crash-stats.mozilla.com/report/index/006c35e4-b3f0-47ab-a10e-57a872110827

The stack says that it ran into OOM in the CSS parser, inside of here:

http://hg.mozilla.org/mozilla-central/annotate/e87454393401/layout/style/nsCSSParser.cpp#l3600

If this code can specifically use a lot of memory, maybe it should use a fallible memory allocator? Not sure what our "policy" toward OOM and allocation failure really is.
Severity: normal → critical
Keywords: crash
This is a dupe of your own bug 682615. ;-)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 682615
Why do you think it's a duplicate of bug 682615?

Reopening until I understand that, to make sure that we don't forget about it.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(In reply to Benoit Jacob [:bjacob] from comment #2)
> Why do you think it's a duplicate of bug 682615?

Because it's the same signature. If you are saying that all frames of this signature are useless, then we probably need to add moz_xmalloc to the skiplist as well.
Uh ... yeah ... crashes in moz_xmalloc are useless.
Hey! First time I disagree with 2 comments in a row.

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #3)
> (In reply to Benoit Jacob [:bjacob] from comment #2)
> > Why do you think it's a duplicate of bug 682615?
> 
> Because it's the same signature.

A [@ mozalloc_abort | mozalloc_handle_oom | moz_xmalloc ] just means that it crashed as it ran out of memory.

So different, unrelated bugs can easily have this same signature.

The way to tell them apart is to look at the stack to see who did the memory allocation that failed.

> If you are saying that all frames of this
> signature are useless, then we probably need to add moz_xmalloc to the
> skiplist as well.

Don't do that! See below:

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4)
> Uh ... yeah ... crashes in moz_xmalloc are useless.

Not all of them. Of course if the stack points to something really innocent e.g. "copy a short string in a place where no long strings/buffers are involved" then indeed there's nothing to do about it. We actually ran out of memory, so bad stuff is inevitable.

But if the stack points to code that potentially does really large memory allocations, that are not vital to all of Firefox, then it's very possible that this code needs to be edited to use fallible allocations and check for error to gracefully recover. This is what happened in bug 682615.

I don't know into what category the present bug falls. We need a CSS parser developer to look into this.
(In reply to Benoit Jacob [:bjacob] from comment #5)
> Hey! First time I disagree with 2 comments in a row.
> 
> (In reply to Robert Kaiser (:kairo@mozilla.com) from comment #3)
> > (In reply to Benoit Jacob [:bjacob] from comment #2)
> > > Why do you think it's a duplicate of bug 682615?
> > 
> > Because it's the same signature.
> 
> A [@ mozalloc_abort | mozalloc_handle_oom | moz_xmalloc ] just means that it
> crashed as it ran out of memory.
> 
> So different, unrelated bugs can easily have this same signature.
> 
> The way to tell them apart is to look at the stack to see who did the memory
> allocation that failed.

Right.

> > If you are saying that all frames of this
> > signature are useless, then we probably need to add moz_xmalloc to the
> > skiplist as well.
> 
> Don't do that! See below:

No, that's exactly what we want to do.  Adding it to the skiplist would change the signature to [@ mozalloc_abort | mozalloc_handle_oom | moz_xmalloc ]

> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4)
> > Uh ... yeah ... crashes in moz_xmalloc are useless.
> 
> Not all of them. Of course if the stack points to something really innocent
> e.g. "copy a short string in a place where no long strings/buffers are
> involved" then indeed there's nothing to do about it. We actually ran out of
> memory, so bad stuff is inevitable.
> 
> But if the stack points to code that potentially does really large memory
> allocations, that are not vital to all of Firefox, then it's very possible
> that this code needs to be edited to use fallible allocations and check for
> error to gracefully recover. This is what happened in bug 682615.

I should have been more clear.  What I meant is that a crash signature that stops of moz_xmalloc doesn't tell us anything useful.  I agree with everything you've said in these two paragraphs.
(In reply to Benoit Jacob [:bjacob] from comment #0)
> Here's one of the crash reports:
> 
> https://crash-stats.mozilla.com/report/index/006c35e4-b3f0-47ab-a10e-57a872110827

This is exactly the sort of allocation that (as I understand it) we're supposed to use infallible malloc for:  it's one of a set of tiny allocations we make as we're building up a parse tree; handling errors at each point is a lot of code complexity.  If we shouldn't be using infallible malloc here, then we shouldn't have infallible malloc in the codebase, and we should just handle all malloc errors in code (like we used to).
A drop more detail on comment 7, since I suspect the crash report will go away after a bit.  The caller of malloc that's failing is:
http://hg.mozilla.org/mozilla-central/annotate/e87454393401/layout/style/StyleRule.cpp#l834
   834  nsCSSSelector* newSel = new nsCSSSelector();
which is in turn called by:
http://hg.mozilla.org/mozilla-central/annotate/e87454393401/layout/style/nsCSSParser.cpp#l3600
  3600  nsCSSSelector* selector = aList->AddSelector(aPrevCombinator);
At one point, there was talk of having a fallback (e.g. were we'd set an OOM flag and then check it more rarely; in this case maybe every rule or something)....
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6)
> No, that's exactly what we want to do.  Adding it to the skiplist would
> change the signature to [@ mozalloc_abort | mozalloc_handle_oom |
> moz_xmalloc ]

That's the current one. Adding moz_xmalloc to the skiplist would make it:
[@ mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | nsCSSSelectorList::AddSelector ]

If we want that, please file a bug in Webtools::Socorro for adding moz_xmalloc to the skiplist and we'll get it into the next update.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #10)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6)
> > No, that's exactly what we want to do.  Adding it to the skiplist would
> > change the signature to [@ mozalloc_abort | mozalloc_handle_oom |
> > moz_xmalloc ]
> 
> That's the current one. Adding moz_xmalloc to the skiplist would make it:
> [@ mozalloc_abort | mozalloc_handle_oom | moz_xmalloc |
> nsCSSSelectorList::AddSelector ]

Bah, I could have sworn I had that right.

> If we want that, please file a bug in Webtools::Socorro for adding
> moz_xmalloc to the skiplist and we'll get it into the next update.

/me does that.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11)
> > If we want that, please file a bug in Webtools::Socorro for adding
> > moz_xmalloc to the skiplist and we'll get it into the next update.
> 
> /me does that.

Bug 682960.
Depends on: 636113
Crash Signature: [@ mozalloc_abort | mozalloc_handle_oom | moz_xmalloc ] → [@ mozalloc_abort | mozalloc_handle_oom | moz_xmalloc ] [@ mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | nsCSSSelectorList::AddSelector] [@ mozalloc_abort(char const* const) | mozalloc_handle_oom() | moz_xmalloc | nsCSSSelectorList::AddSelector(w…
OS: Linux → All
Hardware: x86_64 → All
Closing because no crash reported since 12 weeks.
Status: REOPENED → RESOLVED
Closed: 8 years agoLast year
Resolution: --- → WONTFIX
Closing because no crash reported since 12 weeks.
You need to log in before you can comment on or make changes to this bug.