Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Firefox Crash @ nsImageLoadingContent::OnStartContainer

VERIFIED FIXED in Firefox 11

Status

()

Core
DOM
--
critical
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: marcia, Assigned: khuey)

Tracking

({crash, regression, topcrash})

Trunk
mozilla12
crash, regression, topcrash
Points:
---

Firefox Tracking Flags

(firefox11+ verified)

Details

(Whiteboard: [qa!], crash signature)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Seen while looking at new trunk crashes. Signature appears in low volume in Windows and Mac. Trunk crashes started showing up using 20111116030946 build.

One comment from Windows crasher:

I installed an update, let it autorestart, focused taskbar, opened Skype, focused Firefox and it crashed.

Possible pushlog regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9ae1d4f44b8b&tochange=086dea3f0bad

Not sure if Bug 682077 is involved? Adding Scott in case.

https://crash-stats.mozilla.com/report/index/579b3561-a834-42b1-ad6b-b3cf12111116

Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	nsImageLoadingContent::OnStartContainer 	content/base/src/nsImageLoadingContent.cpp:213
1 	xul.dll 	mozilla::TimeStamp::Now 	xpcom/ds/TimeStamp.cpp:127
2 	xul.dll 	imgRequestProxy::OnStartContainer 	image/src/imgRequestProxy.cpp:641
3 	xul.dll 	imgStatusTracker::SyncNotify 	image/src/imgStatusTracker.cpp:230
4 	nspr4.dll 	MD_CURRENT_THREAD 	nsprpub/pr/src/md/windows/w95thred.c:308
5 	mozutils.dll 	arena_dalloc 	memory/jemalloc/jemalloc.c:4536
6 	xul.dll 	nsEventQueue::GetEvent 	xpcom/threads/nsEventQueue.cpp:80
7 	nspr4.dll 	MD_CURRENT_THREAD 	nsprpub/pr/src/md/windows/w95thred.c:308
8 	nspr4.dll 	PR_Unlock 	nsprpub/pr/src/threads/combined/prulock.c:347
9 	xul.dll 	nsBaseAppShell::OnProcessNextEvent 	widget/src/xpwidgets/nsBaseAppShell.cpp:340
10 	xul.dll 	imgRequestNotifyRunnable::Run 	image/src/imgStatusTracker.cpp:117
11 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:631
12 	xul.dll 	TimerThread::RemoveTimer 	xpcom/threads/TimerThread.cpp:420
13 	xul.dll 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:245
14 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
15 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:201
16 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:175
17 	msvcr90.dll 	getenv_helper_nolock 	f:\\dd\\vctools\\crt_bld\\self_64_amd64\\crt\\src\\getenv.c:154
18 	xul.dll 	nsThreadManager::GetCurrentThread 	xpcom/threads/nsThreadManager.cpp:218
19 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
20 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:228
21 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3551
This is likely a regression from bug 666446. I'll look at it and see what I can do to fix it. Thanks for reporting!

Updated

6 years ago
Depends on: 666446
Blocks: 666446
No longer depends on: 666446

Updated

6 years ago
Assignee: nobody → sjohnson
So, actually, I think I was incorrect in comment 1. bug 666446 doesn't appear to be involved. I will still see if I can track down the push that caused this, though, and hopefully find a fix for it.
No longer blocks: 666446
Hi Marcia -

Could you provide some URLs where this is happening? I'm trying to reproduce and am having difficulty. I'd like to see if it's something that specific to the website, and if so, if I can reliably reproduce it for debugging.

Thanks!
Keywords: needURLs
(Reporter)

Comment 4

6 years ago
Scott: I asked KaiRo to help out with this request since I am working remotely. 

(In reply to Scott Johnson (:jwir3) from comment #3)
> Hi Marcia -
> 
> Could you provide some URLs where this is happening? I'm trying to reproduce
> and am having difficulty. I'd like to see if it's something that specific to
> the website, and if so, if I can reliably reproduce it for debugging.
> 
> Thanks!
(In reply to Marcia Knous [:marcia] from comment #4)
> Scott: I asked KaiRo to help out with this request since I am working
> remotely. 

Ok, thanks Marcia!

Comment 6

6 years ago
URLs from 2011-11-22:

      7 \N
      2 about:blank
      1 http://www.vindictuswiki.com/w/images/5/5d/Queen%27s_Light_Glory_Set_%28View_1%29.png
      1 http://www.teekult.ch/components/com_virtuemart/shop_image/product/1304.jpg
      1 http://www.tdfast.com/wallpapers_res1/450_10760_4.jpg
      1 http://www.hofwat.com/worf.jpg
      1 http://www.foxyboxmods.com/pb/wp_b7a43857/images/img157954de0bd3d13734.jpg
      1 http://www.foxyboxmods.com/pb/wp_b7a43857/images/img149734de09b53e08f7.JPG
      1 http://www.femjoyangels.com/pictures/rachel/warm-up/10.jpg?0
      1 http://www.facebook.com/ajax/pagelet/generic.php/PhotoViewerPagelet?...
      1 http://www.cameras.co.uk/cameraimages/Olympus-vr-310-outdoors-1-large.jpg
      1 http://www.blacksexyshemales.com/wp-content/uploads/2010/06/shemale-01.jpg
      1 http://www.asiafuns.com/uploadedimages/new_260154.jpeg
      1 http://www.akasa.co.uk/img/product/common/gallery/00/AK-ICR-14_g03.jpg
      1 http://thechive.files.wordpress.com/2011/11/diora-baird-5.jpg?w=500
      1 https://twimg0-a.akamaihd.net/profile_images/259358523/IMG_3106_2.jpg
      1 https://lh4.googleusercontent.com/-C8id55qpfOg/TrrRVf_Ax0I/AAAAAAAAByc/LJPqJdBx2qU/h301/daddy.jpg
      1 http://sagakikeita.com/works/56.jpg
      1 http://s1-04.twitpicproxy.com/photos/large/453029261.jpg
      1 http://pic4all.eu/images/TrickyOldTeacher.11.11.21.Tiffany.XXX.1080p.WMV-SEXORS_1.jpg
      1 http://livedoor.blogimg.jp/umashika1209/imgs/b/0/b00888ee.jpg
      1 http://livedoor.blogimg.jp/umashika1209/imgs/2/4/2431906c.jpg
      1 http://linux-sitebuilder.cp.hostnet.nl/sites/cc/cc6f4f59833b47dc2c3bf17c828439ca/__preview/images/logo_bg.gif?template=av-077&colorScheme=blue&header=headers2&button=buttons3
      1 http://indiangirlsclub.com/wp-content/uploads/2011/06/21/1-pakistani-girls-nude-pics.jpg
      1 http://img200.imageshack.us/img200/4792/79553666.gif
      1 http://images8.simpatie.ro/simpatie/SWEET_ROXXY_7_2135761483.jpg
      1 http://images8.simpatie.ro/simpatie/good_girl_kyty_8_1588498259.jpg
      1 http://images8.simpatie.ro/simpatie/alexandrytza_1_1835659309.jpg
      1 http://images.4chan.org/b/src/1322002079961.png
      1 http://images.4chan.org/a/src/1322026697939.png
      1 http://images.4chan.org/a/src/1321988744503.jpg
      1 http://i.imgur.com/xHIar.jpg
      1 http://i.imgur.com/VBsyN.png
      1 http://i.imgur.com/oz7aS.png
      1 http://i.imgur.com/MSpZm.png
      1 http://i.imgur.com/Jy242.png
      1 http://i.imgur.com/DVBF9.jpg
      1 http://i.imgur.com/dR9DP.jpg
      1 http://i.imgur.com/bQi00b.jpg
      1 http://i.imgur.com/AkxD9.jpg
      1 http://gallys.gayrevenge.com/gr/30/th3.jpg
      1 http://forums.vr-zone.com/photopost/data/500/medium/DSC00827.JPG
      1 http://forsan.fsky.info/bitrix/templates/1/banner.jpg
      1 http://donation.24htremblant.com/Pictures/908Team%2024H.jpg
      1 http://cdn-ak.f.st-hatena.com/images/fotolife/F/Factory-T/20080615/20080615172129.jpg
      1 http://bit.ly/tGqWOi
      1 http://3.bp.blogspot.com/-Vd-hVpvOLYQ/ToGSyRuWlTI/AAAAAAAAKhU/0bXIZUn_boI/s1600/2sdfhdghj.jpg
      1 http://3.bp.blogspot.com/-LFPzzgnBrvc/TcmCpOaaklI/AAAAAAAAAUM/1sxMa9n9HH8/s1600/tumblr_l1ls6z5ehc1qzdoopz.jpg
Keywords: needURLs
Thanks KaiRo.

Comment 8

6 years ago
i might be reading the crash-stats wrong, but isn't this the second-most crasher on mac-nightlies?
(In reply to matthias koplenig from comment #8)
> i might be reading the crash-stats wrong, but isn't this the second-most
> crasher on mac-nightlies?

Hm, yes it does look that way.
I just ran into this, but it's not reproducible. http://hg.mozilla.org/mozilla-central/annotate/3c8147998124/content/base/src/nsImageLoadingContent.cpp#l213 is where it crashes; a null dereference there doesn't make a _ton_ of sense, but I don't know what the lifetime of nsImageLoadingContent listeners is.

Adding Boris, who might be able to shed some light.
Got it on http://imgur.com/YsKly:
http://crash-stats.mozilla.com/report/index/bp-d0b3703a-a814-4b01-b563-117382111125
This seems to only be occurring on image documents from the URLs in comment 6. Jared changed image documents in bug 700854 and bug 700856, which are included in the possible regression range; I'm adding him to see if he has any ideas.
(In reply to Joe Drew (:JOEDREW!) from comment #12)
> This seems to only be occurring on image documents from the URLs in comment
> 6. Jared changed image documents in bug 700854 and bug 700856, which are
> included in the possible regression range; I'm adding him to see if he has
> any ideas.

I tried visiting the URLs above but I can't reproduce these crashes. Joe & Benoit: did you have a lot of memory pressure at the time?
I didn't, I don't think. Certainly, I had only a couple of tabs open.
I think in one instance I was running a canvas app, but not the other times:
http://alteredqualia.com/visualization/evolve/
(In reply to Joe Drew (:JOEDREW!) from comment #10)
> I just ran into this, but it's not reproducible.
> http://hg.mozilla.org/mozilla-central/annotate/3c8147998124/content/base/src/
> nsImageLoadingContent.cpp#l213 is where it crashes; a null dereference there
> doesn't make a _ton_ of sense, but I don't know what the lifetime of
> nsImageLoadingContent listeners is.
> 
> Adding Boris, who might be able to shed some light.

hg@1
144// Macro to call some func on each observer. This handles observers
hg@1
145// removing themselves.
hg@1
146#define LOOP_OVER_OBSERVERS(func_) \
hg@1
147 PR_BEGIN_MACRO \
hg@1
148 for (ImageObserver* observer = &mObserverList, *next; observer; \
hg@1
149 observer = next) { \
hg@1
150 next = observer->mNext; \
hg@1
151 if (observer->mObserver) { \
hg@1
152 observer->mObserver->func_; \
hg@1
153 } \
hg@1
154 } \
hg@1
155 PR_END_MACRO 

There's nothing in there that can be dereferenced without being null checked ...  a null dereference there doesn't make any sense.
I took a quick look myself and I have no idea either. Can we get the raw report from socorro and look at the PC/IP address? With the disassemble we should be able to tell what we were meant to look up.
Since it doesn't look like this is affected by bug 682077, I'm going to re-push it.
One thing we might be able to do to determine what is causing this is to temporarily move the implementation of the LOOP_OVER_OBSERVERS macro into the function in question, in order to see which line exactly (and perhaps which variable) is being dereferenced incorrectly.

My thinking is that this isn't a NULL crash - that there must be some garbage in one of these pointers (similar to how we do frame poisoning, perhaps?) and it's passing the null check, but it's actually an invalid pointer. 

The question is why it's getting set to this invalid value in the first place...
This sounds like the reproducible crash i was investigating in bug 376997. Specifically see comment 74 and 75.
Created attachment 577756 [details] [diff] [review]
crash-reproduce-patch (DO NOT COMMIT TO TRUNK!)

(In reply to Timothy Nikkel (:tn) from comment #20)
> This sounds like the reproducible crash i was investigating in bug 376997.
> Specifically see comment 74 and 75.

I've confirmed this is the same crash. To reproduce: 

1) Pull code from rev 1363c893427c from Oct. 18.
2) Push the patch in this attachment on top of this rev.
3) Compile.
4) Run this mochitest: dom/tests/mochitest/dom-level2-html/test_HTMLFrameElement01.html.

It should exhibit this crash. Now, I'll work on tracing this back and figuring out where the pointer is getting set to this invalid value.

Thanks to Timothy for walking me through how to reproduce this!
Hm, so I think I found what is going on here. Somehow, ImageObserver* next and ImageObserver* observer are set to the same address:

214	  for (ImageObserver* observer = &mObserverList, *next = nsnull;
(gdb) n
216	    next = observer->mNext;
(gdb) p next
$1 = (nsImageLoadingContent::ImageObserver *) 0x7f07e1614c10
(gdb) p observer
$2 = (nsImageLoadingContent::ImageObserver *) 0x7f07e1614c10

I'm not quite sure how this is happening, but when the line next = observer->mNext is hit, somehow, it sets both pointers to an invalid value, since observer->mNext is invalid.

If I make the change so that the code looks like this:

  ImageObserver* observer = &mObserverList;
  ImageObserver* next = nsnull;
  while (observer) {
    next = observer->mNext;

    if (observer->mObserver) {
      observer->mObserver->OnStartContainer(aRequest, aContainer);
    }

    observer = next;
  }

instead of this:

  for (ImageObserver* observer = &mObserverList, *next;
         observer; observer = next) {
    next = observer->mNext;
    if (observer->mObserver) {
      observer->mObserver->OnStartContainer(aRequest, aContainer);
    }
  }

Then, the crash doesn't happen, even though the code should be the same. I think there is something subtle going on here in how these two (seemingly identical) code blocks are compiled.
We should probably track this for Fx11, as it's pretty bad, and a regression.

I can reproduce it consistently by installing Reddit Enhancement Suite <http://reddit.honestbleeps.com/>, then clicking "expand image" on a Reddit comment such as <http://www.reddit.com/r/gaming/comments/n8nsf/image_from_current_thread_on_v_thought_it_might/c375r44>, then closing that image and clicking on the image link itself (which loads it in a tab). Immediate crash.
tracking-firefox11: --- → ?
Created attachment 580942 [details] [diff] [review]
b703133-debug

(In reply to Joe Drew (:JOEDREW!) from comment #23)
> We should probably track this for Fx11, as it's pretty bad, and a regression.
> 
> I can reproduce it consistently by installing Reddit Enhancement Suite
> <http://reddit.honestbleeps.com/>, then clicking "expand image" on a Reddit
> comment such as
> <http://www.reddit.com/r/gaming/comments/n8nsf/
> image_from_current_thread_on_v_thought_it_might/c375r44>, then closing that
> image and clicking on the image link itself (which loads it in a tab).
> Immediate crash.

Joe, can you see if this patch alleviates the crash for you?
(Reporter)

Updated

6 years ago
Keywords: reproducible
That patch doesn't fix the crash, no.
Created attachment 582138 [details]
stacks showing the problem

Since I've been seeing this crash a lot viewing standalone images, I added the following assertion patch to me tree:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/83e81a185e6f/img-notify-assert
since this looked like a crash-on-unwind.

In other words, the current notification code is set up to tolerate the removal of the observer being notified, but it's not ok with *other* observers being removed during the notification, and adding observers during a notification isn't great either.

In any case, just now I hit the crash, and got these stacks.  They've got some funny frames in them, but I think they show the point of what's going on.
MediaDocument::GetFileName really does seem like the wrong time to call SetDocumentCharacterSet.  Could we have done that earlier?
This is probably a regression from bug 700854, but I really blame bug 199237.
I suppose one possibility might be to just remove the SetDocumentCharacterSet(docCharset) call in MediaDocument::GetFileName and leave everything else as-is.  This code seems a bit wacky to me, but perhaps it makes more sense to somebody else.

Comment 30

6 years ago
Hrm.  There's other wackiness here too:

1)  Why does SetDocumentCharacterSet do a _sync_ style reconstruct?
2)  nsImageLoadingContent should use nsTObserverArray, I think.  It'd work much better.
(In reply to Boris Zbarsky (:bz) from comment #30)
> Hrm.  There's other wackiness here too:
> 
> 1)  Why does SetDocumentCharacterSet do a _sync_ style reconstruct?

The only reason to anything with style at all is performing language-sensitive font fallback with the root language inferred from the encoding, right? I don't see why that needs to be sync.

Comment 32

6 years ago
This is our #2 crash on the trunk. I think it's worthy of the top crash keyword for tracking.
Keywords: topcrash
Created attachment 583784 [details] [diff] [review]
remove the SetDocumentCharacterSet call

I was noticing this crash about once a day before I put this patch in my tree; I haven't seen it since.

I don't really see why this is here -- if we don't SetDocumentCharacterSet presumably we'd just go through the same code path the next time around.  But I'm not really an expert on this code, either.

Thoughts?
Attachment #583784 - Flags: review?(bzbarsky)

Comment 34

6 years ago
> I don't really see why this is here

The CVS blame says:

1.7 <jshin@mailaps.org> 2003-07-25 22:17
bug 199237 : fix the rendering of the non-ASCII title of a non-text doc(image, media) opened in a new tab/window (r=bryner, sr=bz)

So we really do want the charset set at some point.  The question is whether we can make it async.

Comment 35

6 years ago
Comment on attachment 583784 [details] [diff] [review]
remove the SetDocumentCharacterSet call

I don't think this is right....
Attachment #583784 - Flags: review?(bzbarsky) → review-

Comment 36

6 years ago
But maybe it's ok if no one actually looks at the document character set?
I'm getting this on any image, regardless of url if I right click the image and select "View Image" from the context menu on a webpage.
Reassigning to default as I'm not actively working on this. Feel free to take. :)
Assignee: sjohnson → nobody
Is there a possibility that the patch in bug 708431 could fix this? The patch there also includes a fix for ImageDocument.

Updated

6 years ago
tracking-firefox11: ? → +

Comment 40

6 years ago
Can we get this assigned to somebody else? It's a reproducible regression and top crash.
(In reply to Boris Zbarsky (:bz) from comment #36)
> But maybe it's ok if no one actually looks at the document character set?

I don't see why anything would care what the document character set for an image is other than the code that gets the filename to display... i.e., this code.
Johnny - could we find somebody to take a quick look at this in the 11 Aurora timeframe? Thanks!
I can't reproduce this crash using my steps in comment 23 any more, with or without the patch on bug 708431. :(
(Note: on Nightly.)
Kyle, could you have a look at this?
Assignee: nobody → khuey

Comment 46

6 years ago
(In reply to David Baron [:dbaron] from comment #41)
> I don't see why anything would care what the document character set for an
> image is other than the code that gets the filename to display... i.e., this
> code.

Save as code might, also for filenames....

Kyle, you may want to see comment 30.

Comment 47

6 years ago
Still crashes for me. Clicking on images on wikipedia triggers the bug quickly.

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff5e38413 in nsImageLoadingContent::OnStartContainer(imgIRequest*, imgIContainer*) (this=0x7fffdaecf430, aRequest=0x7fffd2ee3e40, 
    aContainer=0x7fffd2e24e30) at /var/tmp/mozilla-central/content/base/src/nsImageLoadingContent.cpp:213
213       LOOP_OVER_OBSERVERS(OnStartContainer(aRequest, aContainer));
(gdb) bt
#0  0x00007ffff5e38413 in nsImageLoadingContent::OnStartContainer(imgIRequest*, imgIContainer*) (this=0x7fffdaecf430, aRequest=0x7fffd2ee3e40, 
    aContainer=0x7fffd2e24e30) at /var/tmp/mozilla-central/content/base/src/nsImageLoadingContent.cpp:213
#1  0x00007ffff5d8061a in imgRequestProxy::OnStartContainer (this=0x7fffd2ee3e40, image=0x7fffd2e24e30)
    at /var/tmp/mozilla-central/image/src/imgRequestProxy.cpp:641
#2  0x00007ffff5fabf02 in imgStatusTracker::SyncNotify (this=0x7fffd44a4da0, proxy=0x7fffd2ee3e40)
    at /var/tmp/mozilla-central/image/src/imgStatusTracker.cpp:230
#3  0x00007ffff5fac095 in imgRequestNotifyRunnable::Run (this=0x7fffd10ff0a0) at /var/tmp/mozilla-central/image/src/imgStatusTracker.cpp:117
#4  0x00007ffff61de972 in nsThread::ProcessNextEvent (this=0x7ffff71257c0, mayWait=<optimized out>, result=0x7fffffffa00f)
    at /var/tmp/mozilla-central/xpcom/threads/nsThread.cpp:660
#5  0x00007ffff611e237 in NS_ProcessNextEvent_P (thread=<optimized out>, mayWait=false)
    at /var/tmp/mozilla-central/moz-build-dir/xpcom/build/nsThreadUtils.cpp:245
#6  0x00007ffff6309a93 in mozilla::ipc::MessagePump::Run (this=0x7ffff71b4700, aDelegate=0x7ffff71e40b0)
    at /var/tmp/mozilla-central/ipc/glue/MessagePump.cpp:110
#7  0x00007ffff5d1708f in RunInternal (this=<optimized out>) at /var/tmp/mozilla-central/ipc/chromium/src/base/message_loop.cc:208
#8  MessageLoop::Run (this=<optimized out>) at /var/tmp/mozilla-central/ipc/chromium/src/base/message_loop.cc:201
#9  0x00007ffff6315a63 in nsBaseAppShell::Run (this=0x7fffef5053c0) at /var/tmp/mozilla-central/widget/src/xpwidgets/nsBaseAppShell.cpp:189
#10 0x00007ffff5dd0647 in nsAppStartup::Run (this=0x7fffef578bf0) at /var/tmp/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:220
#11 0x00007ffff669ad6a in XRE_main (argc=<optimized out>, argv=<optimized out>, aAppData=<optimized out>)
    at /var/tmp/mozilla-central/toolkit/xre/nsAppRunner.cpp:3523
#12 0x00000000004066cb in do_main (argv=0x7fffffffe5d8, argc=1, exePath=0x7fffffffa4c0 "/var/tmp/mozilla-central/moz-build-dir/dist/bin/")
    at /var/tmp/mozilla-central/browser/app/nsBrowserApp.cpp:201
#13 main (argc=<optimized out>, argv=<optimized out>) at /var/tmp/mozilla-central/browser/app/nsBrowserApp.cpp:287

Updated

6 years ago
Duplicate of this bug: 715200

Comment 49

6 years ago
In 12.0a1, it's #4 top crasher on Windows or Linux and #1 on Mac OS X.
But in 11.0a2 on Windows, it's only #144 top crasher (#1 on Mac OS X and #6 on Linux)!
Summary: Firefox Crash @ nsImageLoadingContent → Firefox Crash @ nsImageLoadingContent::OnStartContainer
Whiteboard: [STR in comment 23]
Have these crashes in Nightly gone down since the patch for bug 708431 landed (Jan 8, 2012)? It contained a fix for ImageDocument.cpp and VideoDocument.cpp.

Comment 51

6 years ago
(In reply to Jared Wein [:jwein and :jaws] from comment #50)
> Have these crashes in Nightly gone down since the patch for bug 708431
> landed (Jan 8, 2012)?
Those crashes fluctuate across builds so it's too soon to conclude but I'd say it has no impact: 23 crashes in 12.0a1/20120109 against about 30 crashes on average per build.
See the graph tab in:
https://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A12.0a1&query_search=signature&query_type=contains&query=nsImageLoadingContent%3A%3AOnStartContainer%28imgIRequest*%2C%20imgIContainer*%29&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=nsImageLoadingContent%3A%3AOnStartContainer%28imgIRequest*%2C%20imgIContainer*%29

Comment 52

6 years ago
Thanks scoobidiver. I added few numbers from chofmann's daily reports....

Jan 09, 2012 - 37 crashes
Jan 08, 2012 - 56 crashes
Jan 07, 2012 - 33 crashes
Jan 06, 2012 - (couldn't get data)
Jan 05, 2012 - 59 crashes
Jan 04, 2012 - 36 crashes
Keywords: reproducible → testcase-wanted
Whiteboard: [STR in comment 23]
Attachment #580942 - Attachment is obsolete: true
Attachment #577756 - Attachment is obsolete: true
(In reply to Boris Zbarsky (:bz) from comment #30)
> Hrm.  There's other wackiness here too:
> 
> 1)  Why does SetDocumentCharacterSet do a _sync_ style reconstruct?

Because the prescontext observers for character set changes and does a sync style reconstruct:

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#1110

This was added in Bug 103282.  This seems like the kind of thing we could do off a runnable.

> 2)  nsImageLoadingContent should use nsTObserverArray, I think.  It'd work
> much better.

Yeah, it should.  I'm not sure that that is sufficient for nsImageLoadingContent to be safe for reentrancy though.
Created attachment 587683 [details] [diff] [review]
Do reflow off a runnable

Currently percolating through try.
Attachment #583784 - Attachment is obsolete: true

Comment 55

6 years ago
Comment on attachment 587683 [details] [diff] [review]
Do reflow off a runnable

One question I had was whether we need the sync style data rebuild at all (even off the event) or whether we can just use the existing PostRebuildAllStyleDataEvent mechanism.
(In reply to Boris Zbarsky (:bz) from comment #55)
> Comment on attachment 587683 [details] [diff] [review]
> Do reflow off a runnable
> 
> One question I had was whether we need the sync style data rebuild at all
> (even off the event) or whether we can just use the existing
> PostRebuildAllStyleDataEvent mechanism.

I have no idea.  Doing the UpdateCharset call at the same time as the style data rebuild seems less likely to break things than doing the UpdateCharset call immediately and doing the style data rebuild later.
Comment on attachment 587683 [details] [diff] [review]
Do reflow off a runnable

This passed try on the platforms it built on (still need to figure out some gcc weirdness) but the idea is evident enough.
Attachment #587683 - Flags: review?(bzbarsky)

Comment 58

6 years ago
Comment on attachment 587683 [details] [diff] [review]
Do reflow off a runnable

>+  CharSetChangingRunnable(nsPresContext* aPresContext,
>+                          nsCString& aCharSet)

aCharSet should be |const nsCString|.  Surprised that compiled.

r=me modulo that
Attachment #587683 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #58)
> Comment on attachment 587683 [details] [diff] [review]
> Do reflow off a runnable
> 
> >+  CharSetChangingRunnable(nsPresContext* aPresContext,
> >+                          nsCString& aCharSet)
> 
> aCharSet should be |const nsCString|.  Surprised that compiled.
> 
> r=me modulo that

It doesn't on gcc, and that might be why!

/me mumbles about traveling and not having his normal machines.

Comment 60

6 years ago
> aCharSet should be |const nsCString|.

Or more precisely |const nsCString&|
https://hg.mozilla.org/mozilla-central/rev/7c7d2a8db7ff

Marcia, et al.:  Can QA watch crashstats to see if this drops off?
Not a single report on the 1/12 nightly so far.

Comment 63

6 years ago
I was able to reproduce this crash extremely reliably until 2012-1-11. With 2012-1-12 it indeed seems to be gone. Great, thank you very much.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #62)
> Not a single report on the 1/12 nightly so far.

And not one on any later nightlies.  I think we can call this fixed.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Attachment #587683 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Status: RESOLVED → VERIFIED
Comment on attachment 587683 [details] [diff] [review]
Do reflow off a runnable

[Triage Comment]
Top crash, and had some time to bake on m-c. Let's uplift to Aurora.
Attachment #587683 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Call me selfish (having just had three crashes from this), but any news on uplift? :D
status-firefox11: --- → affected
https://hg.mozilla.org/releases/mozilla-aurora/rev/bee0171b9597
status-firefox11: affected → fixed
Marking verified for Firefox 11 since there have been no crash reports in Beta.
status-firefox11: fixed → verified
Whiteboard: [qa!]

Comment 69

5 years ago
This behavior was still present in v13 and is now present in v14 (just updated today, 14.0.1).

Pasting an image URL into the address bar and pressing Enter, clicking "View Image" in a context menu, or just clicking a direct link to a JPG (cf. Google Image Search results) will result in a crash.

This never happens immediately after the browser is launched, but after trying to directly view an image a few times it will inevitably recur.

I've submitted Crash Reports numerous times in the past few weeks.
You should probably file a new bug with those steps to reproduce.

Comment 71

5 years ago
Sure, new bug report created:
https://bugzilla.mozilla.org/show_bug.cgi?id=781566

Ironically all my crash reports point to this bug as "Related"
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.