528493 - Crash [@ nsIFrame::GetView() ] Data from Faulting Address may be used as a return value starting at gklayout!nsRuleNode::GetPresContext+0x000000000000000a (Hash=0x39561 c53.0x607d3622)

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Carsten Book [:Tomcat]]

Steps to reproduce - (reproduced with 1.9.2 debug build and 1.9.1.5 Opt build)

-> Loading http://www.under-my-skin.fr/indexx.php 
->> Crashes on this Site 

found during the testrun for Bug 526587

(51c.748): Access violation - code c0000005 (!!! second chance !!!)
eax=0000001c ebx=7ffd6000 ecx=0000001c edx=0629a4b0 esi=0012f3e8 edi=0012f41c
eip=02e0a6ba esp=0012f2dc ebp=0012f2e0 iopl=0         nv up ei pl nz na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000206

gklayout!nsRuleNode::GetPresContext+0xa:
02e0a6ba 8b00            mov     eax,dword ptr [eax]  ds:0023:0000001c=????????
0:000> cdb: Reading initial command '!load winext\msec.dll;.logappend;!exploitab
le;k;q'
Opened log file 'dbgeng.log'

Exploitability Classification: UNKNOWN
Recommended Bug Title: Data from Faulting Address may be used as a return value
starting at gklayout!nsRuleNode::GetPresContext+0x000000000000000a (Hash=0x39561
c53.0x607d3622)

The data from the faulting address may later be used as a return value from this
 function.
ChildEBP RetAddr
0012f2e0 02e0a69d gklayout!nsRuleNode::GetPresContext+0xa
0012f2ec 02eabe15 gklayout!nsIFrame::PresContext+0x1d
0012f308 02ea5be2 gklayout!nsIFrame::GetProperty+0x25
0012f32c 02e71864 gklayout!nsIFrame::GetView+0x32
0012f344 02ea5e63 gklayout!nsLayoutUtils::GetCrossDocParentFrame+0x24
0012f37c 02e6eb80 gklayout!nsIFrame::GetOffsetTo+0x53
0012f3c0 02e70ad5 gklayout!PluginBoundsEnumerator+0x30
0012f3d0 00286503 gklayout!nsTHashtable<nsPtrHashKey<nsObjectFrame> >::s_EnumStu
b+0x15
0012f41c 02e6f5d4 xpcom_core!PL_DHashTableEnumerate+0x113
0012f43c 02e6e947 gklayout!nsTHashtable<nsPtrHashKey<nsObjectFrame> >::Enumerate
Entries+0x54
0012f784 02e6edd1 gklayout!nsRootPresContext::GetPluginGeometryUpdates+0xe7
0012f7a4 02e1354f gklayout!nsRootPresContext::UpdatePluginGeometry+0x21
0012f940 02e13b14 gklayout!PresShell::DoReflow+0x68f
0012f97c 02e0b542 gklayout!PresShell::ProcessReflowCommands+0xf4
0012f9ac 02f0d1a7 gklayout!PresShell::FlushPendingNotifications+0x212
0012f9c4 0030c82a gklayout!nsGfxScrollFrameInner::AsyncScrollPortEvent::Run+0x37

0012fa00 00297b43 xpcom_core!nsThread::ProcessNextEvent+0x1fa
0012fa1c 01e4cead xpcom_core!NS_ProcessNextEvent_P+0x53
0012fa30 020a42db gkwidget!nsBaseAppShell::Run+0x5d
0012fa44 1000c302 tkitcmps!nsAppStartup::Run+0x6b
quit:

Comment 1

[Carsten Book [:Tomcat]]

also crash report id : http://crash-stats.mozilla.com/report/index/41c453a9-241e-4228-b8ab-540a12091113

Note: for the steps to reproduce it might be that the page does not crash in the first seconds. For the crash report above on mac i loaded http://www.under-my-skin.fr/indexx.php left the tab open and checked my mail for 30 seconds...and boom -> Crash :)

Comment 2

[Carsten Book [:Tomcat]]

On Windows XP Firefox 3.6 Beta 2 crashed here on load http://www.under-my-skin.fr/indexx.php

http://crash-stats.mozilla.com/report/index/bp-9e62d401-9d45-40d5-a0da-26b992091113 
Firefox 3.6b2 Crash Report [@nsIFrame::GetView() ]

for some reasons on a second try with the same build i got:

http://crash-stats.mozilla.com/report/index/541e5f2e-ad45-43ae-95a1-ab4402091113

Firefox 3.6b2 Crash Report [@nsObjectFrame::ComputeWidgetGeometry(nsRegion const&, nsPoint const&, nsTArray<nsIWidget::Configuration>*) ]

Comment 3

[chris hofmann]

so I guess one question is weather these two crashes we see on the test url by a random user are the same crashes that tomcat has reproduced? and the stacks don't look quite the same.


20091109-crashdata.csv:nsIFrame::GetView()	
http://www.under-my-skin.fr/indexx.php	
http://crash-stats.mozilla.com/report/index/a514eb04-8e43-42fc-b28d-102472091109
200911090246	200911090246	94694	
Firefox	3.6b1	20091029171059	1.9.2	
Windows NT	5.1.2600 Service Pack 3	x86	
0xfffffffff0dea7ff		\N

20091109-crashdata.csv:nsIFrame::GetView()	
http://www.under-my-skin.fr/indexx.php	
http://crash-stats.mozilla.com/report/index/af304ed7-6d74-4a94-a3f2-e18132091109
200911090248	200911090249	177	
Firefox	3.6b1	20091029171059	1.9.2	
Windows NT	5.1.2600 Service Pack 3	x86	
0xfffffffff0dea7ff		\N

Comment 4

[chris hofmann]

the crashes time proximity of the two crash reports also makes it appear that it might be the same user, or content that two users hit at about the same time.

Comment 5

[chris hofmann]

the first crash report in comment 2 matches random-user's crash, but 

the crash in comment 0 shows tomcat got a bit further past GetView()

ChildEBP RetAddr
0012f2e0 02e0a69d gklayout!nsRuleNode::GetPresContext+0xa
0012f2ec 02eabe15 gklayout!nsIFrame::PresContext+0x1d
0012f308 02ea5be2 gklayout!nsIFrame::GetProperty+0x25
0012f32c 02e71864 gklayout!nsIFrame::GetView+0x32

Comment 6

[chris hofmann]

at any rate this one should really be blocking.

Comment 7

[Bob Clary [:bc] (inactive)]

Tomcat has hardware dep enabled. Would that be responsible for the difference?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Zack, can you debug this?

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

--> Layout:Frames

Roc: make a call on blocking? Feels like it probably should.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Given we can reproduce, yes.

Comment 11

[Daniel Veditz [:dveditz]]

On a 3.5.6pre windows debug build I get a completely different stack (long
chain of various frame ::Destroy()s) ultimately crashing in
nsSupportsArray::Clear() trying to release mArray[0] which is 0xdddddddd

We need to capture the page and try to create a reduced testcase before it changes.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I crash instantly on Mac trunk.

Comment 13

[Carsten Book [:Tomcat]]

(In reply to comment #11)

> We need to capture the page and try to create a reduced testcase before it
> changes.

i have captured the page but not able to crash on the local copy so far 

(In reply to comment #12)
> I crash instantly on Mac trunk.

confirmed, it seems from the testing i did so far, as much as we go to a newer branch (like 1.9.2/1.9.3) we crash sooner ? like 3.5.5 took a while here for the crash.

Also submitting the 2 crash reports i got during this test 

(http://crash-stats.mozilla.com/report/index/1d920d18-b771-4c67-9d77-51beb2091113 and http://crash-stats.mozilla.com/report/index/bp-5ae29339-b487-4faf-b0a0-8c2292091113) - breakpad display for the latter one bug 471493 which was fixed in April 2009.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, here's the problem stack:

#0  DocumentViewerImpl::InitPresentationStuff (this=0xe225a40, aDoInitialReflow=0, aReenableRefresh=0) at /Users/roc/mozilla-checkin/layout/base/nsDocumentViewer.cpp:713
#1  0x02c958f0 in DocumentViewerImpl::Show (this=0xe225a40) at /Users/roc/mozilla-checkin/layout/base/nsDocumentViewer.cpp:2002
#2  0x047f8ff6 in nsDocShell::SetVisibility (this=0xf71ea60, aVisibility=1) at /Users/roc/mozilla-checkin/docshell/base/nsDocShell.cpp:4552
#3  0x02fb096b in nsFrameLoader::Show (this=0xf71e650, marginWidth=1, marginHeight=1, scrollbarPrefX=2, scrollbarPrefY=2, frame=0x967e288) at /Users/roc/mozilla-checkin/content/base/src/nsFrameLoader.cpp:530
#4  0x02d30d41 in nsSubDocumentFrame::ShowViewer (this=0x967e258) at /Users/roc/mozilla-checkin/layout/generic/nsFrameFrame.cpp:324
#5  0x02d30feb in nsSubDocumentFrame::Init (this=0x967e258, aContent=0xf71e720, aParent=0x967e1d0, aPrevInFlow=0x0) at /Users/roc/mozilla-checkin/layout/generic/nsFrameFrame.cpp:289
#6  0x02c48ec3 in nsCSSFrameConstructor::InitAndRestoreFrame (this=0xf3d62e0, aState=@0xbfffba14, aContent=0xf71e720, aParentFrame=0x967e1d0, aPrevInFlow=0x0, aNewFrame=0x967e258, aAllowCounters=1) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:4695
#7  0x02c511bd in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0xf3d62e0, aItem=@0xf73d340, aState=@0xbfffba14, aParentFrame=0x967e1d0, aFrameItems=@0xbfff9d74) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:3906
#8  0x02c51969 in nsCSSFrameConstructor::ConstructFramesFromItem (this=0xf3d62e0, aState=@0xbfffba14, aIter=@0xbfff9cb4, aParentFrame=0x967e1d0, aFrameItems=@0xbfff9d74) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:5600
#9  0x02c51afb in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0xf3d62e0, aState=@0xbfffba14, aItems=@0xf73d27c, aParentFrame=0x967e1d0, aFrameItems=@0xbfff9d74) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:9575
...
#42 0x02c526c2 in nsCSSFrameConstructor::ProcessChildren (this=0xf3d62e0, aState=@0xbfffba14, aContent=0xf717330, aStyleContext=0x965dc88, aFrame=0x965dcf0, aCanHaveGeneratedContent=1, aFrameItems=@0xbfffb91c, aAllowBlockStyles=1, aPendingBinding=0x0) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:9683
#43 0x02c53345 in nsCSSFrameConstructor::ConstructBlock (this=0xf3d62e0, aState=@0xbfffba14, aDisplay=0x160fca0, aContent=0xf717330, aParentFrame=0x160ffe0, aContentParentFrame=0x160ffe0, aStyleContext=0x965dc88, aNewFrame=0xbfffbabc, aFrameItems=@0xbfffba94, aAbsPosContainer=0, aPendingBinding=0x0) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:10726
#44 0x02c57ed3 in nsCSSFrameConstructor::ConstructDocElementFrame (this=0xf3d62e0, aDocElement=0xf717330, aFrameState=0x0, aNewFrame=0xbfffbbe0) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:2627
#45 0x02c58391 in nsCSSFrameConstructor::ContentInserted (this=0xf3d62e0, aContainer=0x0, aChild=0xf717330, aIndexInContainer=0, aFrameState=0x0) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:6640
#46 0x02cd32d2 in PresShell::InitialReflow (this=0xf3d5f80, aWidth=63300, aHeight=31260) at /Users/roc/mozilla-checkin/layout/base/nsPresShell.cpp:2621
#47 0x02f414c9 in nsContentSink::StartLayout (this=0x212c1a00, aIgnorePendingSheets=1) at /Users/roc/mozilla-checkin/content/base/src/nsContentSink.cpp:1326
#48 0x0311f266 in HTMLContentSink::StartLayout (this=0x212c1a00, aIgnorePendingSheets=1) at /Users/roc/mozilla-checkin/content/html/document/src/nsHTMLContentSink.cpp:2621
#49 0x031200bb in HTMLContentSink::FlushPendingNotifications (this=0x212c1a00, aType=Flush_Layout) at /Users/roc/mozilla-checkin/content/html/document/src/nsHTMLContentSink.cpp:3029
#50 0x02f92303 in nsDocument::FlushPendingNotifications (this=0x215d6400, aType=Flush_Layout) at /Users/roc/mozilla-checkin/content/base/src/nsDocument.cpp:6327
#51 0x02f92387 in nsDocument::FlushPendingNotifications (this=0x133f400, aType=Flush_Style) at /Users/roc/mozilla-checkin/content/base/src/nsDocument.cpp:6350
#52 0x02fe32bd in nsObjectLoadingContent::NotifyStateChanged (this=0xf735f60, aOldType=nsObjectLoadingContent::eType_Loading, aOldState=2097152, aSync=1) at /Users/roc/mozilla-checkin/content/base/src/nsObjectLoadingContent.cpp:1528
#53 0x02fe853c in AutoNotifier::Notify (this=0xbfffc194) at /Users/roc/mozilla-checkin/content/base/src/nsObjectLoadingContent.cpp:234
#54 0x02fe4c96 in nsObjectLoadingContent::LoadObject (this=0xf735f60, aURI=0xf736230, aNotify=1, aTypeHint=@0xbfffc36c, aForceLoad=0) at /Users/roc/mozilla-checkin/content/base/src/nsObjectLoadingContent.cpp:1174
#55 0x02fe5a0e in nsObjectLoadingContent::LoadObject (this=0xf735f60, aURI=@0xbfffc2d8, aNotify=1, aTypeHint=@0xbfffc36c, aForceLoad=0) at /Users/roc/mozilla-checkin/content/base/src/nsObjectLoadingContent.cpp:991
#56 0x030e6afd in nsHTMLSharedObjectElement::StartObjectLoad (this=0xf735f40, aNotify=1) at /Users/roc/mozilla-checkin/content/html/content/src/nsHTMLSharedObjectElement.cpp:432
#57 0x030e860d in nsHTMLSharedObjectElement::StartObjectLoad (this=0xf735f40) at /Users/roc/mozilla-checkin/content/html/content/src/nsHTMLSharedObjectElement.cpp:133
#58 0x030e87be in nsRunnableMethod<nsHTMLSharedObjectElement, void>::Run (this=0xf736080) at nsThreadUtils.h:282
#59 0x02f53f4a in nsContentUtils::RemoveScriptBlocker () at /Users/roc/mozilla-checkin/content/base/src/nsContentUtils.cpp:4485
#60 0x02d3a6a1 in nsContentUtils::RemoveRemovableScriptBlocker () at nsContentUtils.h:1403
#61 0x02d3acdf in mozAutoDocUpdate::~mozAutoDocUpdate (this=0xbfffc58c) at mozAutoDocUpdate.h:69
#62 0x02d3ad05 in mozAutoDocUpdate::~mozAutoDocUpdate (this=0xbfffc58c) at mozAutoDocUpdate.h:74
#63 0x02fc7472 in nsGenericElement::doInsertChildAt (aKid=0xf735f40, aIndex=0, aNotify=0, aParent=0xf735e10, aDocument=0x133f400, aChildArray=@0xf735e2c) at /Users/roc/mozilla-checkin/content/base/src/nsGenericElement.cpp:3313
#64 0x02fc74ff in nsGenericElement::InsertChildAt (this=0xf735e10, aKid=0xf735f40, aIndex=0, aNotify=0) at /Users/roc/mozilla-checkin/content/base/src/nsGenericElement.cpp:3227
#65 0x02c40a51 in nsINode::AppendChildTo (this=0xf735e10, aKid=0xf735f40, aNotify=0) at nsINode.h:428
#66 0x031461f4 in nsPluginDocument::CreateSyntheticPluginDocument (this=0x133f400) at /Users/roc/mozilla-checkin/content/html/document/src/nsPluginDocument.cpp:320
#67 0x0314628d in nsPluginDocument::SetScriptGlobalObject (this=0x133f400, aScriptGlobalObject=0xf72fdd0) at /Users/roc/mozilla-checkin/content/html/document/src/nsPluginDocument.cpp:205
#68 0x03281d40 in nsGlobalWindow::SetNewDocument (this=0xf71ef30, aDocument=0x133f400, aState=0x0, aClearScopeHint=1, aIsInternalCall=0) at /Users/roc/mozilla-checkin/dom/base/nsGlobalWindow.cpp:1986
#69 0x03282486 in nsGlobalWindow::SetNewDocument (this=0xf71ef30, aDocument=0x133f400, aState=0x0, aClearScopeHint=1) at /Users/roc/mozilla-checkin/dom/base/nsGlobalWindow.cpp:1550
#70 0x02c965a0 in DocumentViewerImpl::InitInternal (this=0xe225a40, aParentWidget=0x0, aState=0x0, aBounds=@0xbfffcdbc, aDoCreation=1, aInPrintPreview=0, aNeedMakeCX=1) at /Users/roc/mozilla-checkin/layout/base/nsDocumentViewer.cpp:952
#71 0x02c96d4f in DocumentViewerImpl::Init (this=0xe225a40, aParentWidget=0x0, aBounds=@0xbfffcdbc) at /Users/roc/mozilla-checkin/layout/base/nsDocumentViewer.cpp:697
#72 0x0480d7cf in nsDocShell::SetupNewViewer (this=0xf71ea60, aNewViewer=0xe225a40) at /Users/roc/mozilla-checkin/docshell/base/nsDocShell.cpp:7289
#73 0x04801e42 in nsDocShell::Embed (this=0xf71ea60, aContentViewer=0xe225a40, aCommand=0x488c14c "", aExtraInfo=0x0) at /Users/roc/mozilla-checkin/docshell/base/nsDocShell.cpp:5478
#74 0x048158e5 in nsDocShell::CreateContentViewer (this=0xf71ea60, aContentType=0xf702cc8 "application/x-shockwave-flash", request=0xf7205f0, aContentHandler=0xf7204e0) at /Users/roc/mozilla-checkin/docshell/base/nsDocShell.cpp:7074
#75 0x0483dbce in nsDSURIContentListener::DoContent (this=0xf71e790, aContentType=0xf702cc8 "application/x-shockwave-flash", aIsContentPreferred=0, request=0xf7205f0, aContentHandler=0xf7204e0, aAbortProcess=0xbfffd074) at /Users/roc/mozilla-checkin/docshell/base/nsDSURIContentListener.cpp:138
#76 0x0484762e in nsDocumentOpenInfo::TryContentListener (this=0xf7204d0, aListener=0xf71e790, aChannel=0xf7205f0) at /Users/roc/mozilla-checkin/uriloader/base/nsURILoader.cpp:736
#77 0x04847e44 in nsDocumentOpenInfo::DispatchContent (this=0xf7204d0, request=0xf7205f0, aCtxt=0x0) at /Users/roc/mozilla-checkin/uriloader/base/nsURILoader.cpp:434
#78 0x04848b48 in nsDocumentOpenInfo::OnStartRequest (this=0xf7204d0, request=0xf7205f0, aCtxt=0x0) at /Users/roc/mozilla-checkin/uriloader/base/nsURILoader.cpp:280
#79 0x01d4e3fa in nsHttpChannel::CallOnStartRequest (this=0xf7205c0) at /Users/roc/mozilla-checkin/netwerk/protocol/http/src/nsHttpChannel.cpp:839

The basic problem with this stack is that we're reentering nsDocumentViewer::InitPresentationStuff on the same document viewer!

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes the crash, and the site seems to work too.

I'm not sure about a testcase, though.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Like Tomcat, saving the page and then loading it locally doesn't crash for me.

I also tried creating a synthetic testcase that loads the Flash object into an <iframe>, can't get that to crash either.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> I'm not sure about a testcase, though.

Looking at the stack, something like this might work:

  <head>
    <link rel="stylesheet" href="something that will load slowly">
  </head>
  <body>
    <iframe src="something that loads faster and has a plug-in type; maybe just
                 'data:plugin/type,'"></iframe>
  </body>

The key is to guarantee getting OnStartRequest for the iframe before the stylesheet load finishes and calls StartLayout.  The other option that might trigger this sort of thing is trying to make sure a reframe style change is in flight for the <iframe> when the OnStartRequest happens, but that's hard.

Comment 18

[Boris Zbarsky [:bzbarsky]]

And in fact I just confirmed that this testcase:

<!DOCTYPE html>
<html>
  <head>
    <link rel="stylesheet" type="text/css"
          href="http://www.hixie.ch/tests/adhoc/html/parsing/script-style-blocking/slow-style.css">
  </head>
  <body>
    <iframe src="data:application/x-test," id="x"></iframe>
  </body>
</html>

ends up with exactly the reentry stack in this bug and various asserts in a debug build (but not a reliable crash for me).

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/93a0acf68dd6

Still need to write the test and land it.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backed out on suspicion of regressing content/html/document/test/test_bug369370.html

Comment 21

[Boris Zbarsky [:bzbarsky]]

Hmm.  Are we being hit by imagelib firing notifications sync again?  :(

Maybe we need to put the CheckOverflowing call in nsImageDocument::OnStartContainer off on a script runner (so it'll happen sync if we're not in a batch, but once the batch ends and it's ok to do the flushing CheckOverflowing wants to do otherwise)?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, this patch definitely caused the regression.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Should I implement the suggestion in comment #21?

Comment 24

[Boris Zbarsky [:bzbarsky]]

If it fixes the regression, I think so.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It doesn't.

I haven't yet figured out what caused this regression. It seems PresShell::FlushPendingNotifications doesn't get called when it's unsafe to run scripts.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks like the test script childLoaded runs when the image frame is still set to the alt text.

Comment 27

[Boris Zbarsky [:bzbarsky]]

When it's unsafe to run scripts the IsSafeToFlush() call at the top of PresShell::FlushPendingNotifications returns false, so the function becomes a no-op.

Comment 21 was just based on looking for flushes that might be thus suppressed by the script blocker.  Maybe there are more?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #27)
> When it's unsafe to run scripts the IsSafeToFlush() call at the top of
> PresShell::FlushPendingNotifications returns false, so the function becomes a
> no-op.

Right, I tried setting a conditional breakpoint to catch that, but it didn't seem to happen.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It looks like nsImageFrame::OnStartContainer never fires to switch us out of displaying the alt text.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, that's not a problem, it seems.

The problem is that with this patch, nsImageLoadingContent::OnStartContainer fires *after* we've already finished the test. Seems like onload is firing in the popup window before nsImageLoadingContent::OnStartContainer. That is definitely wrong.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

hum, this is suspicious --- here is what triggers the load event

#17 0x04752650 in nsDocShell::EndPageLoad (this=0xb4b0c0, aProgress=0xb4b0d4, aChannel=0xf7684b0, aStatus=2153578529) at /Users/roc/mozilla-checkin/docshell/base/nsDocShell.cpp:5730
#18 0x04746ecb in nsDocShell::OnStateChange (this=0xb4b0c0, aProgress=0xb4b0d4, aRequest=0xf7684b0, aStateFlags=131088, aStatus=2153578529) at /Users/roc/mozilla-checkin/docshell/base/nsDocShell.cpp:5608
#19 0x0478c901 in nsDocLoader::FireOnStateChange (this=0xb4b0c0, aProgress=0xb4b0d4, aRequest=0xf7684b0, aStateFlags=131088, aStatus=2153578529) at /Users/roc/mozilla-checkin/uriloader/base/nsDocLoader.cpp:1314
#20 0x0478cb39 in nsDocLoader::doStopDocumentLoad (this=0xb4b0c0, request=0xf7684b0, aStatus=2153578529) at /Users/roc/mozilla-checkin/uriloader/base/nsDocLoader.cpp:926
#21 0x0478ce5a in nsDocLoader::DocLoaderIsEmpty (this=0xb4b0c0, aFlushLayout=1) at /Users/roc/mozilla-checkin/uriloader/base/nsDocLoader.cpp:802
#22 0x0478d9e2 in nsDocLoader::OnStopRequest (this=0xb4b0c0, aRequest=0x89d43b0, aCtxt=0x0, aStatus=0) at /Users/roc/mozilla-checkin/uriloader/base/nsDocLoader.cpp:697
#23 0x01e1dd93 in nsLoadGroup::RemoveRequest (this=0xb4b2b0, request=0x89d43b0, ctxt=0x0, aStatus=0) at /Users/roc/mozilla-checkin/netwerk/base/src/nsLoadGroup.cpp:680
#24 0x027f3ecc in imgRequestProxy::RemoveFromLoadGroup (this=0x89d43b0, releaseLoadGroup=1) at /Users/roc/mozilla-checkin/modules/libpr0n/src/imgRequestProxy.cpp:194
#25 0x027f4587 in imgRequestProxy::OnStopRequest (this=0x89d43b0, request=0x0, ctxt=0x0, statusCode=2152398850, lastPart=1) at /Users/roc/mozilla-checkin/modules/libpr0n/src/imgRequestProxy.cpp:645
#26 0x027ef9c7 in imgRequest::RemoveProxy (this=0x89d4080, proxy=0x89d43b0, aStatus=2153644036, aNotify=0) at /Users/roc/mozilla-checkin/modules/libpr0n/src/imgRequest.cpp:203
#27 0x027f418a in imgRequestProxy::DoCancel (this=0x89d43b0, status=2153644036) at /Users/roc/mozilla-checkin/modules/libpr0n/src/imgRequestProxy.cpp:250
#28 0x027f52ff in imgRequestProxy::imgCancelRunnable::Run (this=0x89d6800) at imgRequestProxy.h:100

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, here's the problem. In nsImageDocument::CreateSyntheticDocument:

  // Make sure not to start the image load from here...
  imageLoader->SetLoadingEnabled(PR_FALSE);
  mImageContent->SetAttr(kNameSpaceID_None, nsGkAtoms::src, srcString, PR_FALSE);
  mImageContent->SetAttr(kNameSpaceID_None, nsGkAtoms::alt, srcString, PR_FALSE);

  body->AppendChildTo(mImageContent, PR_FALSE);
  imageLoader->SetLoadingEnabled(PR_TRUE);

If there is a script blocker in effect here, in AppendChildTo nsHTMLImageElement::BindToTree will trigger a *delayed* nsHTMLImageElement::MaybeLoadImage which ends up running after SetLoadingEnabled(PR_TRUE), so "make sure not to start the image load from here" doesn't.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Fix the regression by simply not even trying to launch MaybeLoadImage if image loading is disabled.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: non-fix

Tbis makes CheckOverflowing run off a scriptrunner in OnStartContainer like Boris suggested. Since it's not needed here, I'm not going to push for it, but you think it's the right thing to do, we can review and land on trunk.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 413499 [details] [diff] [review]
non-fix

I think this is the right thing to do, with a comment about how we need to flush to CheckOverflowing correctly, hence need to make sure we don't try to do it in a scriptblocker.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Pushed the regression fix as:
http://hg.mozilla.org/mozilla-central/rev/711a206bccb0

The CheckOverflow patch as:
http://hg.mozilla.org/mozilla-central/rev/7e93778b4b14

The fix for this bug as:
http://hg.mozilla.org/mozilla-central/rev/e09eac0b5c0a

Comment 37

[Boris Zbarsky [:bzbarsky]]

Pushed regression fix as:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a97581dbce97

The fix for this bug as:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0113c6c01241

Not sure about the checkoverflow thing on branch; I let it be for now.

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get the 1.9.1 rollup patch (fix+regression fix) here so we can resolve this for 1.9.1.8?

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 1.9.1 rollup

Comment 40

[Daniel Veditz [:dveditz]]

Comment on attachment 423593 [details] [diff] [review]
1.9.1 rollup

Approved for 1.9.1.8, a=dveditz for release-drivers

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d3f862ad8425
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e5dcfa07d1ab

Comment 42

[Al Billings [:abillings - ex-MoCo]]

Did the testcase ever get created for this? It looks like it didn't when the regression took over priority.

Comment 43

[Carsten Book [:Tomcat]]

(In reply to comment #42)
> Did the testcase ever get created for this? It looks like it didn't when the
> regression took over priority.

maybe comment #18 count as testcase here ?

Comment 44

[Al Billings [:abillings - ex-MoCo]]

Yes, it may (except it doesn't crash) but a testcase was going to be *checked in* for this according to comment 19. Comment 18 is just a manual case, non-crashing case.

Comment 45

[Daniel Veditz [:dveditz]]

Does this need to be fixed on the 1.9.0 branch as well?