967985 - Firefox 27 crashes with image discarding and decode-on-draw disabled

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[her34her34]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

Firefox 27
new profile

user_pref("image.mem.decodeondraw", false);
user_pref("image.mem.discardable", false);



Actual results:

Firefox crashes while scrolling down website with images


Expected results:

Not crash

Comment 1

[Manuela Muntean [Away]]

WFM with latest Nightly on Win 7 64-bit.

Which website are you scrolling?

Comment 2

[[:Aleksej]]

WFM with 2014-02-10-03-02-01-mozilla-central-firefox-30.0a1.en-US.linux-x86_64 (no hw acceleration).

Comment 3

[her34her34]

http://gigaom.com/
https://crash-stats.mozilla.com/report/index/1cb4294b-ba32-443d-b99f-a2eb02140210
https://crash-stats.mozilla.com/report/index/afcb6c1f-0a0b-49ae-94cb-0a99b2140210

http://www.aol.com/
https://crash-stats.mozilla.com/report/index/c944140d-d25f-4e3e-87a2-84c252140210

https://www.yahoo.com/
https://crash-stats.mozilla.com/report/index/d489dfeb-c747-4f2a-a04a-81ba22140210

Comment 4

[Robert Lickenbrock [:rclick]]

It looks to me like this was probably caused by bug 867755. I get the following assertion failure in my m-c debug builds:

Assertion failure: NS_IsMainThread() || aStrategy == DECODE_ASYNC, at c:\mozilla-central\image\src\Decoder.cpp:91

00 xul!mozilla::image::Decoder::Write+0x3e [c:\mozilla-central\image\src\decoder.cpp @ 91]
01 xul!mozilla::image::RasterImage::WriteToDecoder+0x88 [c:\mozilla-central\image\src\rasterimage.cpp @ 2149]
02 xul!mozilla::image::RasterImage::AddSourceData+0x22b [c:\mozilla-central\image\src\rasterimage.cpp @ 1586]
03 xul!mozilla::image::RasterImage::WriteToRasterImage+0x1c [c:\mozilla-central\image\src\rasterimage.cpp @ 2903]
04 xul!nsInputStreamTee::WriteSegmentFun+0x2f [c:\mozilla-central\xpcom\io\nsinputstreamtee.cpp @ 200]
05 xul!nsPipeInputStream::ReadSegments+0x14d [c:\mozilla-central\xpcom\io\nspipe3.cpp @ 780]
06 xul!nsInputStreamTee::ReadSegments+0x88 [c:\mozilla-central\xpcom\io\nsinputstreamtee.cpp @ 258]
07 xul!mozilla::image::RasterImage::OnImageDataAvailable+0x28 [c:\mozilla-central\image\src\rasterimage.cpp @ 1749]
08 xul!imgRequest::OnDataAvailable+0x5c9 [c:\mozilla-central\image\src\imgrequest.cpp @ 842]
09 xul!ProxyListener::OnDataAvailable+0x4e [c:\mozilla-central\image\src\imgloader.cpp @ 2114]
0a xul!nsStreamListenerTee::OnDataAvailable+0x2a6 [c:\mozilla-central\netwerk\base\src\nsstreamlistenertee.cpp @ 93]
0b xul!mozilla::net::nsHttpChannel::OnDataAvailable+0x528 [c:\mozilla-central\netwerk\protocol\http\nshttpchannel.cpp @ 5328]
0c xul!nsInputStreamPump::OnStateTransfer+0x2b4 [c:\mozilla-central\netwerk\base\src\nsinputstreampump.cpp @ 593]
0d xul!nsInputStreamPump::OnInputStreamReady+0x118 [c:\mozilla-central\netwerk\base\src\nsinputstreampump.cpp @ 434]
0e xul!nsInputStreamReadyEvent::Run+0x4a [c:\mozilla-central\xpcom\io\nsstreamutils.cpp @ 86]
0f xul!nsThreadPool::Run+0x2c2 [c:\mozilla-central\xpcom\threads\nsthreadpool.cpp @ 210]
10 xul!nsThread::ProcessNextEvent+0x3ac [c:\mozilla-central\xpcom\threads\nsthread.cpp @ 643]
11 xul!NS_ProcessNextEvent+0x62 [c:\mozilla-central\xpcom\glue\nsthreadutils.cpp @ 263]
12 xul!mozilla::ipc::MessagePumpForNonMainThreads::Run+0x1e9 [c:\mozilla-central\ipc\glue\messagepump.cpp @ 303]
13 xul!MessageLoop::RunInternal+0x4e [c:\mozilla-central\ipc\chromium\src\base\message_loop.cc @ 227]

Comment 5

[u279076]

Nominating for tracking since this is currently the #12 topcrash in Firefox 28.

Comment 6

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #5)
> Nominating for tracking since this is currently the #12 topcrash in Firefox
> 28.

Correction, this is actually #11.

Comment 7

[Robert Lickenbrock [:rclick]]

So, disabling both discarding and decode-on-draw has the side effect that we no longer store the source data and instead synchronously decode it in the OnDataAvailable callbacks. However, we don't really have a good way of doing that off the main thread since we can only synchronously allocate frames on the main thread.

A possible solution is to always store the source data, complete the decode asynchronously, then throw away the source data if the image is non-discardable. Getting that right in the multipart/x-mixed-replace case could be a bit involved though, and I don't think we'd want to land such a change on branches in the latter half of a cycle.

For the time being, I think the safest thing to do is disable retargeting of OnDataAvailable for images until a proper fix can be implemented.

Comment 8

[Robert Lickenbrock [:rclick]]

Attachment: patch

This is effectively a partial backout of bug 867755. It seems to fix the crashes for me locally. I'll work on fixing this properly and re-enabling off the main thread delivery in followup bugs.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

We're going to build our last beta 28 today, if this is a safe landing please let me know (ping in IRC) if we can get this reviewed and nominated in the next few hours.  Otherwise we will have to let this ride the trains from 29.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Confirmed in IRC with rclick that this only crashes in a configuration we don't ship so this isn't a huge concern for FF28, wontfixing for that version and we'll get this work into FF29 as soon as it's reviewed/landed/verified on trunk.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Seth, could you review Robert's patch? We would like to have it in FF 29. (FYI, 29 beta 3 go to build is today).
Thanks

Comment 12

[Seth Fowler [:seth] [:s2h]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #6)
> (In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #5)
> > Nominating for tracking since this is currently the #12 topcrash in Firefox
> > 28.
> 
> Correction, this is actually #11.

How can a crash caused by a configuration we don't ship be the #11 topcrash? This is really puzzling to me.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8385824 [details] [diff] [review]
patch

Review of attachment 8385824 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks very much for your work in triaging this, Robert!

If your diagnosis is correct, then I'd prefer something much more targeted than this. Rather than describe a much different patch and ask you to code it, I'll just post one of my own shortly. Consider it an attempt to make up for my slow speed in reviewing this patch.

Comment 14

[Seth Fowler [:seth] [:s2h]]

Attachment: Don't retarget OnDataAvailable when not storing source data.

Here's my proposed patch. Rather than totally disable ODA retargeting, this patch makes the check in imgRequest more strict so that we only retarget in situations where it should definitely be safe.

I've confirmed that I can reproduce the crash on gigaom.com without this patch, and that I can't reproduce it after the patch is applied.

Comment 15

[Robert Lickenbrock [:rclick]]

Comment on attachment 8398837 [details] [diff] [review]
Don't retarget OnDataAvailable when not storing source data.

Review of attachment 8398837 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/ImageFactory.cpp
@@ +100,5 @@
> +  if (aIsMultiPart) {
> +    return false;
> +  }
> +
> +  uint32_t imageFlags = ComputeImageFlags(aURI, aIsMultiPart);

In theory, the image flags could change if the preferences are flipped in the time between this check and image's creation. However, I seriously doubt this is a common scenario. It's probably not worth worrying about.

Comment 16

[Seth Fowler [:seth] [:s2h]]

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/efdf983fadd7

Comment 17

[Seth Fowler [:seth] [:s2h]]

(In reply to Robert Lickenbrock [:rclick] from comment #15)
> In theory, the image flags could change if the preferences are flipped in
> the time between this check and image's creation. However, I seriously doubt
> this is a common scenario. It's probably not worth worrying about.

That's a good point. I agree that it's probably now worth worrying about for now, but I'll consider whether there's a clean way to avoid that race.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/efdf983fadd7

Comment 19

[u279076]

This is now down to #48 in the Firefox 31.0a1 7-day topcrash report. However I'm still seeing crashes with this signature on the latest Nightly. It appears to be a different stack though so maybe it's a different bug.

Example:
https://crash-stats.mozilla.com/report/index/d489dfeb-c747-4f2a-a04a-81ba22140210

Seth, can you have a look and see if this needs a follow-up bug?

her34her34, please confirm if you can still reproduce this crash in the latest Firefox Nightly.

Comment 20

[Seth Fowler [:seth] [:s2h]]

We indeed have new STR in bug 989677. Please consider that the followup bug. I'm investigating it now.

Comment 21

[u279076]

I'm going to consider this verified fixed based on crashstats and considering follow-up work is happening in bug 989677.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Is this a significant crasher on 29/30?  We could consider a low risk uplift to our final builds if so, otherwise wontfix for 29 and uplift just to 30.

Comment 23

[Robert Kaiser]

https://crash-stats.mozilla.com/topcrasher_ranks_bybug/?bug_number=967985 says it's #22-#26 in recent 29 betas, and also in a similar range, #20, on 30.0a2. It's also #16 on 28.

Given that we are so near to shipping and it not being a regression in 29, I'm leaning towards only taking it for 30.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Let's get an uplift nomination for 30 then (currently Aurora) so we get this into the early Beta cycle.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Can we get a beta uplift nomination please?  This is the last week to take more speculative or slightly higher risk patches on Beta while we have time to evaluate and try to catch regressions before shipping.

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Too late now for FF30, and this is in the lower end of the crash concerns at #20, wontfixing.

Comment 27

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #19)
> her34her34, please confirm if you can still reproduce this crash in the
> latest Firefox Nightly.

Clearing out this overdue request.