1388301 - Stylo: Crash in alloc::oom::oom

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-3bad05f2-a019-49f8-a091-685fe0170808.
=============================================================

There are 11 crashes in nightly 57 (5 with buildid 20170803134456 and 6 with buildid 20170807113452).

Comment 1

[Calixte Denizet (:calixte)]

There are 40 crashes from 37 installations in beta 56.0b1.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

FWIW most of the crashes I see with this signature are from the mp4 demuxer:

  alloc::raw_vec::RawVec<mp4parse_capi::mp4parse_indice>::double<mp4parse_capi::mp4parse_indice>

I'm still taking a look to see whether there's something actionable we could do here for the stylo-related ones.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So, some stylo related ones:

  https://crash-stats.mozilla.com/report/index/9f04511b-1244-49d7-b787-e37840170811

Seems like something animating the transform property, and it recurses a bunch on Option::map. Not sure yet if it's a bug or just an unlucky OOM.

  https://crash-stats.mozilla.com/report/index/d6a826c5-2502-43f5-a61f-694120170811

This one is cascading the border property, copy-constructing a style struct. Not sure there's anything actionable there except trying to share more structs.

  https://crash-stats.mozilla.com/report/index/e7ece8ca-fafb-4277-b31d-9e27d0170811

This one is building the rule cascades. We're looking into making these fallible in bug 1389009, not sure if there's a good path for it though.

Comment 4

[Calixte Denizet (:calixte)]

My bad, you're right, there is only 1 crash in beta which doesn't contain mp4.
By the way, I'll report a bug for mp4 demuxer and add alloc::oom::oom to the Socorro skiplist to have a better signature.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Err, sorry, pressed "Save Changes" too early.

  https://crash-stats.mozilla.com/report/index/af3bb3ed-3200-4b30-b798-c5f410170810
  https://crash-stats.mozilla.com/report/index/3dd311e8-a527-4177-a19e-4378a0170808
  https://crash-stats.mozilla.com/report/index/d566efad-48d3-4bec-8ff3-5e0c40170808

This is during stylesheet parsing. I suspect we could improve a few bits here, including not converting the whole stylesheet to utf-8.

  https://crash-stats.mozilla.com/report/index/033e3734-814c-4e65-995a-d16270170810

This one doesn't make any sense. It's during the MP4 demuxer it seems, but it claims to be in a vec of MediaQueries, which would be stylo-related.

  https://crash-stats.mozilla.com/report/index/4cd17929-522e-4c5f-8f04-d33fd0170810
  https://crash-stats.mozilla.com/report/index/d65aa19a-f38a-4ffd-8e88-97ddf0170810
  https://crash-stats.mozilla.com/report/index/37336393-ce10-4073-a2c9-df4af0170810
  https://crash-stats.mozilla.com/report/index/fff9a456-4b4c-45b7-8ae3-e6c5c0170810

These two are also rebuilding the rule cascades... We should really try for it to be fallible.

  https://crash-stats.mozilla.com/report/index/4f31c140-7e57-4047-bc42-05e070170808

There is one or two with this signature, which is allocating a style context, and I'm not sure it's really actionable.

  https://crash-stats.mozilla.com/report/index/a7ca5cc7-7821-4562-a2a4-9d5710170806

Then there's this one, which is failing to allocate the "StyleThread#n" string... sigh

There are a few ones webrender-related, fwiw:

  https://crash-stats.mozilla.com/report/index/dc1539b0-4228-4657-ba3b-aecf30170809
  https://crash-stats.mozilla.com/report/index/3e554306-a218-44dd-8dcf-59c730170806

So, the TL;DR is:

  Most of the crashes with this signature are related to the mp4 demuxer. We can, though, look into the ones that are stylo-related, and look what's actionable.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Calixte Denizet (:calixte) from comment #4)
> My bad, you're right, there is only 1 crash in beta which doesn't contain
> mp4.
> By the way, I'll report a bug for mp4 demuxer and add alloc::oom::oom to the
> Socorro skiplist to have a better signature.

That sounds great, thank you! I'll comment in the bugs where we're investigating memory-usage to see what is actionable with those I mentioned above.

Comment 7

[Chris Peterson [:cpeterson]]

Emilio, should we file bugs for your suggested fixes from comment 5? This generic crash signature covers a lot of different issues. Is there anything specific we should do for Stylo in this bug report?

These two suggested fixes were the only ones that sounded actionable:

> This is during stylesheet parsing. I suspect we could improve a few bits
> here, including not converting the whole stylesheet to utf-8.

> These two are also rebuilding the rule cascades... We should really try for
> it to be fallible.

I am marking this bug as status-firefox56=fix-optional for Stylo because, of the 462 crashes with this signature from Nightly 57 and Beta 56 over the last week, only one actually had layout.css.servo.enabled=true, AFAICT. That lone crash report happened seven days ago in 56.0b1:

bp-d6a826c5-2502-43f5-a61f-694120170811

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Chris Peterson [:cpeterson] from comment #7)
> Emilio, should we file bugs for your suggested fixes from comment 5? This
> generic crash signature covers a lot of different issues. Is there anything
> specific we should do for Stylo in this bug report?
> 
> These two suggested fixes were the only ones that sounded actionable:
> 
> > This is during stylesheet parsing. I suspect we could improve a few bits
> > here, including not converting the whole stylesheet to utf-8.
> 
> > These two are also rebuilding the rule cascades... We should really try for
> > it to be fallible.

All these potential improvements fall under the umbrella of bug 1389009.

> I am marking this bug as status-firefox56=fix-optional for Stylo because, of
> the 462 crashes with this signature from Nightly 57 and Beta 56 over the
> last week, only one actually had layout.css.servo.enabled=true, AFAICT. That
> lone crash report happened seven days ago in 56.0b1:

Yeah, that makes sense, because most of them are from the mp4 demuxer, not from stylo itself.

Comment 9

[Bobby Holley (:bholley)]

This should be covered by the OOM handling in bug 1389009 and bug 1393656.

Comment 10

[Bobby Holley (:bholley)]

No crashes in the last 7 days, and the dependent bugs landed.