682595 - OOM crash in the CSS parser

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 1

[Robert Kaiser]

This is a dupe of your own bug 682615. ;-)

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 3

[Robert Kaiser]

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

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Uh ... yeah ... crashes in moz_xmalloc are useless.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Hey! First time I disagree with 2 comments in a row.

(In reply to Robert Kaiser (:[e-mail]) 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] ([e-mail]) 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.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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 (:[e-mail]) 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] ([e-mail]) 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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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);

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Robert Kaiser]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Robert Kaiser (:[e-mail]) from comment #10)
> (In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.