1155722 - Crash @ mozilla::image::imgFrame::ImageUpdated(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) on http://www.classicshell.net/ (even in safe mode)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Matthew]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150402191859

Steps to reproduce:

Visited http://www.classicshell.net/ for the first time on Firefox 37.0.1 on Windows with and without safe mode. Crash could allow denial of service of Firefox.

I have tried loading the webpages assets individually (thus warming up the cache), the page then loads.. I suspect it may have something to do with the favicon.

I found the about:crashes screen reporting "Firefox 37.0.1 Crash Report [@ mozilla::image::imgFrame::ImageUpdated(nsIntRect const&) ]" listing a crash reason of "EXCEPTION_ACCESS_VIOLATION_READ".

New to this so if I've done anything wrong please critique :-).


Actual results:

Crash. I have allowed the crash reporter to report it.


Expected results:

Page should have rendered. It renders in IE.

Comment 1

[:Gijs (out for now; he/him)]

Thanks for the report!

Can you go to about:crashes and copy the crash ID for the crash into a comment here?

Comment 2

[Matthew]

Report ID 	Date Submitted
bp-e0111f69-8fa4-4c1a-8ee8-6183d2150417
	17/04/2015	16:44
bp-1905ba45-e213-45d9-ba76-e32802150417
	17/04/2015	16:43
bp-836cb064-623b-47c9-b4a1-b59da2150417
	17/04/2015	16:26
bp-9d5c7399-2296-4b9c-90eb-043e42150417
	17/04/2015	16:20
bp-66df32d6-960d-4290-b098-4538d2150417
	17/04/2015	16:19

I submitted it a couple of times so you guys (and girls) know it's reproducible.

Comment 3

[:Gijs (out for now; he/him)]

Thanks!

Comment 4

[Matthew]

Gijs - No problem :-).

I have spent a bit more time trying to reproduce it and have learnt the following.

It appears to be some kind of race condition, I tried simulating the same content as that sent by the web server (I even went as far as using Burp to replace the response headers). I found on another computer (Windows 8.1) which has a slower wireless connection that I was unable to crash it (so it probably doesn't happen on every PC).

From looking more at the stack trace it's clearly dying in a thread dealing with the favicon and given the location on the tab of the favicon goes through a number of visual states once the page is loading I am inclined to believe it's linked to the tab redrawing while the favicon is also trying to load.

I have Firefox set to never remember my history, so it has to load the favicon on every site as it's not caching it between browser closes. Once the favicon has loaded then the issue doesn't appear again no matter how many times you view http://classicshell.net/downloads/ which further leads me to think it's a race condition.

Also browsing http://classicshell.net/favicon.ico before trying to load that page is good enough to prevent the crash from happening.

Let me know if I can help further but that's as much as I can determine now.

Comment 5

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

This feels like the kind of issue that might result from the bugs that we fixed in bug 1137002. That landed in Firefox 39. I can't reproduce the crash on Nightly, so it *might* be fixed now.

Matthew, can you reproduce this on Nightly?

Comment 6

[Matthew]

Reproduced with nightly :

Report ID
bp-35ba703c-e93b-47c6-8fe2-7e9782150420

Comment 7

[Matthew]

Can confirm bug present and reproducible in Firefox 38.

Report ID:
bp-684d3b19-6a78-43c9-bd9f-fa6132150512

Comment 8

[Matthew]

And nightly 41.0a1.

Report ID:
bp-3b1c1b6d-12e1-4fbf-8db1-23b642150512

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Needinfo ping.  Seth, there are a few more bugs hanging off 1137002, some in tests as intermittent failures, and there is a lot of crashes with this signature.  And it's still in 41.  Any ideas?

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Setting tracking flag for firefox40 and firefox41. It is hard to assess how often users will encounter this bug? Is it a crash on only this specific site or are there more? Depending on that we may need to track this for firefox39 too.

Comment 11

[Matthew]

Ritu, 

There are more sites when I was looking at other bug reports that others had made for the same crash details. The conditions for crash are a site where you need to download a favicon and the website becoming fully loaded as that favicon is attempting to display.

I just use that particular site as my test case for each of the new versions of Firefox so I can keep reporting to verify it's still present in each new version.

Hope this helps,
Matthew

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Mathew for the additional details. This definitely helps. Tracking it for firefox39 as well (not sure if it can still be done).

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

I'll put together a somewhat special build that could help us at least rule some potential problems out.  Stay tuned.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Matthew, when you have a chance, could you do a custom install of this more instrumented build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d2a01ed01b90/try-win32/firefox-41.0a1.en-US.win32.installer.exe
the custom install part is so that it doesn't overwrite your regular Firefox.  Then run your test and point us to a crash you get?
I do expect the crash to still be there, I'm just hoping it will be in a different place.

Comment 15

[Matthew]

Hah I don't normally run random executables that people share with me.. particularly those with fake signatures, but considering it's come from a Mozilla.com user :-)

bp-1f44b03b-651d-4851-b27a-50e462150519

Let me know if I can be of further assistance.

Thanks,
Matthew

Comment 16

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

I'm having a hard time reproducing this, but I notice that we fail to decode the favicon. Possibly a race condition that involves decode failure.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Matthew from comment #15)
> Hah I don't normally run random executables that people share with me..
> particularly those with fake signatures, but considering it's come from a
> Mozilla.com user :-)
> 
> bp-1f44b03b-651d-4851-b27a-50e462150519
> 
> Let me know if I can be of further assistance.
> 
> Thanks,
> Matthew

I messed up with that previous build, forgot to ask for the symbols.  Any chance you could run this one instead?  The installer is here:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0a514255ab63/try-win32/

from this try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a514255ab63

where you can see the code changes - I am just making some of the usually debug only asserts trigger in the release build.

Comment 18

[Matthew]

No problem, here you go :

bp-7a12f356-3012-4e5f-ba91-1ef4d2150520

Also for those trying to reproduce this, I run in-private browsing and my machine has an SSD which means that a) I have to load the favicon everytime for that site (I found if I load just the favicon for that site so that it is in my memory cache and then try and visit the site then the crash does not exist). and b) the SSD might cause the threads to finish at different times from perhaps some other machines... I was unable to reproduce the issue on my laptop for instance. Such is the pain with thread debugging :-(.

As an aside, "Mozilla Fake SPC" signs these builds, is that the expected behaviour?

Let me know if you need me to test any other builds :-).

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks.  Seth, fwiw, no assertions fire during the run, the crash is in the same place.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

It's getting late to take a fix for this in 39. We could still take a patch for 40.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Assigning since it's tracked.

Comment 22

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b54ef7bba2c

Comment 23

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

^ Since I cannot reproduce this crash, and it's not obvious to me from inspection of the code how we're getting a null mCurrentFrame when nsBMPDecoder calls PostInvalidation, I've pushed a build that fixes this in the dumbest possible way: by adding a runtime nullcheck to Decoder::PostInvalidation.

If this works, at least we will have confirmed the diagnosis.

Comment 24

[Matthew]

Hi Seth,

Sorry but I am not familiar with treeherder.. do you have a build you'd like me to try?

Thanks for your time and patience with this so far :-).

Thanks,
Matthew

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks Matthew - you'd download the ...installer.exe from http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f3ba3bb5108c/try-win32/

Comment 26

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

(In reply to Milan Sreckovic [:milan] from comment #25)
> Thanks Matthew - you'd download the ...installer.exe from
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mfowler@mozilla.
> com-f3ba3bb5108c/try-win32/

Thanks for pointing Matthew in the right direction, Milan! I meant to come back and post links to the builds, but I got distracted with other bugs.

(In reply to Matthew from comment #24)
> Thanks for your time and patience with this so far :-).

Happy to help! This has been a tricky one to figure out, but if the build I pushed fixes it, at least we'll know we're looking in the right place. Thanks for being willing to try it out!

Comment 27

[Matthew]

(In reply to Seth Fowler [:seth] from comment #26)
> Happy to help! This has been a tricky one to figure out, but if the build I
> pushed fixes it, at least we'll know we're looking in the right place.
> Thanks for being willing to try it out!

Thanks :-).

Unfortunately I bring bad news : 51f14d3f-ab3b-4bc4-8279-8fc922150715

Comment 28

[Matthew]

Oops missed the bp:

Report ID 	Date Submitted
bp-51f14d3f-ab3b-4bc4-8279-8fc922150715
	15/07/2015	23:55

Comment 29

[Lawrence Mandel [:lmandel] (use needinfo)]

The crash rate for 40 is in an acceptable range and we don't yet have a fix for this bug. Marking 40 as wontfix.

Comment 30

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

Kyle, does this reproduce on any of the machines in your lab?

Comment 31

[Kyle Fung]

I couldn't reproduce it on a machine using a similar graphics card.

Comment 32

[Matthew]

Anything I can do to assist?

I'm happy to have a TeamViewer / Cisco WebEx session.

Particularly it is the http://www.classicshell.net/downloads/ page when running "Always use private browsing mode".

Comment 33

[:Gijs (out for now; he/him)]

(In reply to Matthew from comment #32)
> Anything I can do to assist?
> 
> I'm happy to have a TeamViewer / Cisco WebEx session.
> 
> Particularly it is the http://www.classicshell.net/downloads/ page when
> running "Always use private browsing mode".

Comment 34

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

I really appreciate the offer to help, but unfortunately this issue is probably going to be difficult to diagnose via remote desktop.

I strongly suspect that this bug is related to the precise way in which packets are chunked when they're arriving over the network. There have been other similar bugs in the past. Recently, in bug 1196065, I've added new tests to try to detect bugs in the image decoders that happen only under these conditions.

Those new tests indeed uncovered a bug in the ICO decoder. I've filed bug 1196066 about fixing that. My plan is to fix that issue (which is easily reproducible with the test) and then check back with you and see if *this* bug is fixed. If not, we can try to find a new approach.

Comment 35

[Milan Sreckovic [:milan] (needinfo for best results)]

I don't see this getting fixed in 41.

Comment 36

[Matthew]

>Those new tests indeed uncovered a bug in the ICO decoder. I've filed bug 1196066 about fixing that.

Excellent! Looks like you're all over it :-). The moment you have a patch for 1196066 I'm happy to give it a shot as I'm sure it's exactly that (I noticed anything that plays with larger or lower MTUs or proxies seems to affect the reproducibility of the bug, smaller packets would therefore make perfect sense) :-).

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Too late for 41.

Comment 38

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

(In reply to Matthew from comment #36)
> >Those new tests indeed uncovered a bug in the ICO decoder. I've filed bug 1196066 about fixing that.
> 
> Excellent! Looks like you're all over it :-). The moment you have a patch
> for 1196066 I'm happy to give it a shot as I'm sure it's exactly that (I
> noticed anything that plays with larger or lower MTUs or proxies seems to
> affect the reproducibility of the bug, smaller packets would therefore make
> perfect sense) :-).

Thanks for being willing to try out the fix! I just pushed bug 1196066 to inbound, so with luck it should end up in tomorrow's Nightly. I'll be really interested to hear if the crash is still reproducible with that build.

Comment 39

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

Unfortunately the change got backed out. I've repushed it, but it won't be in tonight's Nightly. I'll post an update when it does make it into Nightly.

Comment 40

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

OK, the change landed again. It should be part of the next Nightly release, and it will also make it into Developer Edition.

Comment 41

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

This change is in Nightly now. It's not in the current Developer Edition, although it will arrive there within a few days.

Matthew, can you still reproduce this issue on Nightly?

Comment 42

[Matthew]

Report ID 	Date Submitted
bp-54b94fb8-7831-40a5-b13b-2ffd12150927
	27/09/2015	11:26

This what you required?

Comment 43

[Matthew]

Ignore my last comment, I hadn't closed all instances of Firefox before starting nightly.

I have downloaded and installed both x86 and x64 nightly Firefox and so far so good, I have been unable to reproduce :-)

Thank you so much :-).

Regards,
Matthew

Comment 44

[u279076]

FWIW, I'm not seeing *any* reports of this signature on any current Firefox branch in Socorro.

Comment 45

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

Excellent. Thanks for verifying the fix, Matthew and Anthony!

Comment 46

[Sylvestre Ledru [:Sylvestre]]

Seems that 42 is fixed too.

Comment 47

[Georg Koppen]

FWIW we still have a number of users that hit this one with esr38. Thanks to a Python script provided by a user (which is attached) I was able to reproduce the crash reliably and bisected a bit. The crash itself got fixed with bug 1117607 which left my browser frozen though. It gets un-frozen again with bug 1196066.

I am wondering whether

1) it got ruled out by anyone that this crash is exploitable?
2) there is an easy workaround we could apply to the last upcoming esr38 release? Looking at the patches involved alone I guess it is too much effort to backport them. But can we do something else instead? Like setting `browser.chrome.site_icons` to `false`?

Comment 48

[Georg Koppen]

Attachment: Python script for reproducing the crash

Comment 49

[Matthew]

Attachment: Bug Proof of Concept (for Python 2 and 3)

Updated and simplified Python PoC which should work in both Python 2 and 3.

Comment 50

[Matthew]

Attachment: Updated Bug Proof of Concept (for Python 2 and 3)

Further updated and simplified Python PoC which should work in both Python 2 and 3.

Comment 51

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

(In reply to Georg Koppen from comment #47)
> FWIW we still have a number of users that hit this one with esr38. Thanks to
> a Python script provided by a user (which is attached) I was able to
> reproduce the crash reliably and bisected a bit. The crash itself got fixed
> with bug 1117607 which left my browser frozen though. It gets un-frozen
> again with bug 1196066.

I'm sorry, it may be because I'm sick and on cold medicine, but I'm not totally sure I understand what you're saying here, and I just want to double check my understanding. You are saying that:

(1) Between when bug 1117607 landed and when bug 1196066 landed, Firefox stopped crashing, but the browser froze when we hit this bug.

(2) After bug 1196066 landed, there is no problem at all and everything is resolved.

Is this right?

Please let me know and then set the needinfo flag again.

Comment 52

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

To be clear, my understanding is that (2) is definitely the case, but (1) is a bit surprising to me as it's not immediately clear to me how that patch converted this bug from a crash to a freeze.

Comment 53

[Georg Koppen]

(In reply to Seth Fowler [:seth] [:s2h] from comment #51)
> (In reply to Georg Koppen from comment #47)
> > FWIW we still have a number of users that hit this one with esr38. Thanks to
> > a Python script provided by a user (which is attached) I was able to
> > reproduce the crash reliably and bisected a bit. The crash itself got fixed
> > with bug 1117607 which left my browser frozen though. It gets un-frozen
> > again with bug 1196066.
> 
> I'm sorry, it may be because I'm sick and on cold medicine, but I'm not
> totally sure I understand what you're saying here, and I just want to double
> check my understanding. You are saying that:
> 
> (1) Between when bug 1117607 landed and when bug 1196066 landed, Firefox
> stopped crashing, but the browser froze when we hit this bug.
> 
> (2) After bug 1196066 landed, there is no problem at all and everything is
> resolved.
> 
> Is this right?

Yes.

Comment 54

[Georg Koppen]

(In reply to Georg Koppen from comment #53)
> (In reply to Seth Fowler [:seth] [:s2h] from comment #51)
> > I'm sorry, it may be because I'm sick and on cold medicine, but I'm not
> > totally sure I understand what you're saying here, and I just want to double
> > check my understanding. You are saying that:
> > 
> > (1) Between when bug 1117607 landed and when bug 1196066 landed, Firefox
> > stopped crashing, but the browser froze when we hit this bug.
> > 
> > (2) After bug 1196066 landed, there is no problem at all and everything is
> > resolved.
> > 
> > Is this right?
> 
> Yes.

To be a bit more verbose: If I use the script I attached and test with opening 127.0.0.1:8080/favicon.ico in the browser, I can see a "This tab has crashed" notice before the fix for bug 1117607 landed. But I still can open a new tab and browse the web or open look at my file system with file:///. With bug 1117607 landed I don't get such a notice anymore. Rather I seem to see part of a rendered icon and my browser freezes which means I can still open new tabs but they are not usable. I can't surf the web nor can't I look at my filesystem with file:/// anymore. Those tabs just show me a spinning icon as if they were trying to connect. Going back to the tab with the partly rendered icon shows me now a tab that is totally blank. Hope this helps.