Last Comment Bug 703133 - Firefox Crash @ nsImageLoadingContent::OnStartContainer
: Firefox Crash @ nsImageLoadingContent::OnStartContainer
Status: VERIFIED FIXED
[qa!]
: crash, regression, topcrash
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- critical with 3 votes (vote)
: mozilla12
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
: 715200 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-16 16:04 PST by Marcia Knous [:marcia - use ni]
Modified: 2015-10-16 11:38 PDT (History)
32 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified


Attachments
crash-reproduce-patch (DO NOT COMMIT TO TRUNK!) (20.76 KB, patch)
2011-11-29 14:46 PST, Scott Johnson (:jwir3)
no flags Details | Diff | Splinter Review
b703133-debug (1.59 KB, patch)
2011-12-12 09:12 PST, Scott Johnson (:jwir3)
no flags Details | Diff | Splinter Review
stacks showing the problem (12.01 KB, text/plain)
2011-12-15 16:16 PST, David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch)
no flags Details
remove the SetDocumentCharacterSet call (1.03 KB, patch)
2011-12-22 06:55 PST, David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch)
bzbarsky: review-
Details | Diff | Splinter Review
Do reflow off a runnable (3.53 KB, patch)
2012-01-11 06:48 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Marcia Knous [:marcia - use ni] 2011-11-16 16:04:21 PST
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
Comment 1 Scott Johnson (:jwir3) 2011-11-16 16:25:58 PST
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!
Comment 2 Scott Johnson (:jwir3) 2011-11-21 10:44:55 PST
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.
Comment 3 Scott Johnson (:jwir3) 2011-11-22 14:11:38 PST
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!
Comment 4 Marcia Knous [:marcia - use ni] 2011-11-22 15:55:56 PST
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!
Comment 5 Scott Johnson (:jwir3) 2011-11-22 23:34:18 PST
(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 Robert Kaiser 2011-11-23 09:22:05 PST
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
Comment 7 Scott Johnson (:jwir3) 2011-11-23 11:01:46 PST
Thanks KaiRo.
Comment 8 matthias koplenig 2011-11-24 12:33:36 PST
i might be reading the crash-stats wrong, but isn't this the second-most crasher on mac-nightlies?
Comment 9 Scott Johnson (:jwir3) 2011-11-24 14:37:46 PST
(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.
Comment 10 Joe Drew (not getting mail) 2011-11-24 20:31:24 PST
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.
Comment 12 Joe Drew (not getting mail) 2011-11-25 20:51:05 PST
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.
Comment 13 Jared Wein [:jaws] (please needinfo? me) 2011-11-25 22:56:35 PST
(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?
Comment 14 Joe Drew (not getting mail) 2011-11-26 10:29:19 PST
I didn't, I don't think. Certainly, I had only a couple of tabs open.
Comment 15 Benoit Girard (:BenWa) 2011-11-26 11:11:26 PST
I think in one instance I was running a canvas app, but not the other times:
http://alteredqualia.com/visualization/evolve/
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-26 11:24:14 PST
(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.
Comment 17 Benoit Girard (:BenWa) 2011-11-26 12:26:18 PST
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.
Comment 18 Scott Johnson (:jwir3) 2011-11-28 10:41:20 PST
Since it doesn't look like this is affected by bug 682077, I'm going to re-push it.
Comment 19 Scott Johnson (:jwir3) 2011-11-28 13:59:26 PST
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...
Comment 20 Timothy Nikkel (:tnikkel) 2011-11-28 14:02:00 PST
This sounds like the reproducible crash i was investigating in bug 376997. Specifically see comment 74 and 75.
Comment 21 Scott Johnson (:jwir3) 2011-11-29 14:46:53 PST
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!
Comment 22 Scott Johnson (:jwir3) 2011-11-30 13:26:38 PST
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.
Comment 23 Joe Drew (not getting mail) 2011-12-11 16:31:22 PST
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.
Comment 24 Scott Johnson (:jwir3) 2011-12-12 09:12:20 PST
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?
Comment 25 Joe Drew (not getting mail) 2011-12-13 12:55:25 PST
That patch doesn't fix the crash, no.
Comment 26 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2011-12-15 16:16:03 PST
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.
Comment 27 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2011-12-15 16:23:21 PST
MediaDocument::GetFileName really does seem like the wrong time to call SetDocumentCharacterSet.  Could we have done that earlier?
Comment 28 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2011-12-15 16:26:52 PST
This is probably a regression from bug 700854, but I really blame bug 199237.
Comment 29 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2011-12-15 16:35:13 PST
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 Boris Zbarsky [:bz] 2011-12-15 18:55:59 PST
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.
Comment 31 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-12-16 00:08:11 PST
(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 Sheila Mooney 2011-12-21 21:15:14 PST
This is our #2 crash on the trunk. I think it's worthy of the top crash keyword for tracking.
Comment 33 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2011-12-22 06:55:50 PST
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?
Comment 34 Boris Zbarsky [:bz] 2011-12-22 08:24:00 PST
> 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 Boris Zbarsky [:bz] 2011-12-22 08:37:53 PST
Comment on attachment 583784 [details] [diff] [review]
remove the SetDocumentCharacterSet call

I don't think this is right....
Comment 36 Boris Zbarsky [:bz] 2011-12-22 08:41:36 PST
But maybe it's ok if no one actually looks at the document character set?
Comment 37 Rob Campbell [:rc] (:robcee) 2011-12-23 07:15:19 PST
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.
Comment 38 Scott Johnson (:jwir3) 2011-12-24 15:33:02 PST
Reassigning to default as I'm not actively working on this. Feel free to take. :)
Comment 39 Jared Wein [:jaws] (please needinfo? me) 2011-12-24 16:07:29 PST
Is there a possibility that the patch in bug 708431 could fix this? The patch there also includes a fix for ImageDocument.
Comment 40 Sheila Mooney 2012-01-03 14:14:11 PST
Can we get this assigned to somebody else? It's a reproducible regression and top crash.
Comment 41 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2012-01-03 14:16:44 PST
(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.
Comment 42 Alex Keybl [:akeybl] 2012-01-03 14:35:47 PST
Johnny - could we find somebody to take a quick look at this in the 11 Aurora timeframe? Thanks!
Comment 43 Joe Drew (not getting mail) 2012-01-03 15:25:25 PST
I can't reproduce this crash using my steps in comment 23 any more, with or without the patch on bug 708431. :(
Comment 44 Joe Drew (not getting mail) 2012-01-03 15:25:46 PST
(Note: on Nightly.)
Comment 45 Johnny Stenback (:jst, jst@mozilla.com) 2012-01-03 17:24:32 PST
Kyle, could you have a look at this?
Comment 46 Boris Zbarsky [:bz] 2012-01-03 22:36:59 PST
(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 Octoploid 2012-01-04 04:56:34 PST
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
Comment 48 Paul Rouget [:paul] 2012-01-04 10:46:07 PST
*** Bug 715200 has been marked as a duplicate of this bug. ***
Comment 49 Scoobidiver (away) 2012-01-07 01:38:13 PST
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)!
Comment 50 Jared Wein [:jaws] (please needinfo? me) 2012-01-10 13:05:14 PST
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 Scoobidiver (away) 2012-01-10 14:34:55 PST
(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 Sheila Mooney 2012-01-10 14:38:16 PST
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
Comment 53 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-11 04:58:17 PST
(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.
Comment 54 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-11 06:48:27 PST
Created attachment 587683 [details] [diff] [review]
Do reflow off a runnable

Currently percolating through try.
Comment 55 Boris Zbarsky [:bz] 2012-01-11 07:09:58 PST
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.
Comment 56 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-11 07:24:12 PST
(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 57 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-11 12:27:06 PST
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.
Comment 58 Boris Zbarsky [:bz] 2012-01-11 12:38:37 PST
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
Comment 59 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-11 12:40:03 PST
(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 Boris Zbarsky [:bz] 2012-01-11 14:05:04 PST
> aCharSet should be |const nsCString|.

Or more precisely |const nsCString&|
Comment 61 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-11 14:11:46 PST
https://hg.mozilla.org/mozilla-central/rev/7c7d2a8db7ff

Marcia, et al.:  Can QA watch crashstats to see if this drops off?
Comment 62 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-13 03:04:42 PST
Not a single report on the 1/12 nightly so far.
Comment 63 Philipp Hartwig 2012-01-13 03:20:01 PST
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.
Comment 64 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-15 13:16:30 PST
(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.
Comment 65 Alex Keybl [:akeybl] 2012-01-17 10:12:47 PST
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.
Comment 66 Richard Newman [:rnewman] 2012-01-23 21:23:59 PST
Call me selfish (having just had three crashes from this), but any news on uplift? :D
Comment 67 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-24 01:02:47 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/bee0171b9597
Comment 68 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-05 15:57:33 PST
Marking verified for Firefox 11 since there have been no crash reports in Beta.
Comment 69 AlexV 2012-08-08 17:42:22 PDT
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.
Comment 70 Timothy Nikkel (:tnikkel) 2012-08-08 21:37:55 PDT
You should probably file a new bug with those steps to reproduce.
Comment 71 AlexV 2012-08-09 13:35:37 PDT
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"

Note You need to log in before you can comment on or make changes to this bug.