330089 - [FIX]crash when retrieving contentFrame.contentDocument

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Eyal Rozenberg]

In the last couple of nightlies, in:

http://www.mozdev.org/source/browse/~checkout~/bidiui/source/tbird/chrome/content/bidimailpack/bidimailpack-composer.js?rev=1.137&content-type=text/plain

from the code of BiDi Mail UI has been triggering a crash; we have determined that the crash occurs when retrieving contentFrame.contentDocument, on the line

        direction =
          document.defaultView
                  .getComputedStyle(document.getElementById("content-frame")
                  .contentDocument.body, "").getPropertyValue("direction");

in the setCasterPair function of the directionSwitchController.

Comment 1

[Eyal Rozenberg]

Bug is first triggered with the nightly from 2006-03-07.

Comment 2

[Eyal Rozenberg]

Oooh, lots of possible suspects:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-03-06+09%3A10%3A00&maxdate=2006-03-07+09%3A10%3A00&cvsroot=%2Fcvsroot

Setting helpwanted.

Comment 3

[Eyal Rozenberg]

Install the extension

http://downloads.mozdev.org/bidiui/latest/bidimailui_suite.xpi

to reproduce.

Comment 4

[Eyal Rozenberg]

To reproduce:

1. Install the extension

http://downloads.mozdev.org/bidiui/latest/bidimailui_suite.xpi

2. close sm
3. start sm mail & news
4. press 'compose'
5. crash and burn!

Comment 5

[Eyal Rozenberg]

Talkback:

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB16204763K

Stack:

0x00000000
nsIFrame::SetStyleContext  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/../generic\nsIFrame.h, line 558]
nsFrameManager::ReResolveStyleContext  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsFrameManager.cpp, line 1312]
nsFrameManager::ComputeStyleChangeFor  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsFrameManager.cpp, line 1351]
nsCSSFrameConstructor::RestyleElement  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 10610]
nsCSSFrameConstructor::ProcessOneRestyle  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 13435]
nsCSSFrameConstructor::ProcessPendingRestyles  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 13487]
PresShell::FlushPendingNotifications  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp, line 5060]
nsBoxObject::GetFrame  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxObject.cpp, line 163]
nsEditorBoxObject::GetDocShell  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsEditorBoxObject.cpp, line 73]
XPCWrappedNative::CallMethod  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2152]
XPC_WN_GetterSetter  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1476]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1246]
js_InternalInvoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1347]
js_InternalGetOrSet  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1406]
js_GetProperty  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsobj.c, line 3022]
js_Interpret  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 5614]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1270]
js_InternalInvoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1347]
js_InternalGetOrSet  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1406]
js_GetProperty  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsobj.c, line 3022]
js_Interpret  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 5614]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1270]
js_InternalInvoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1347]
js_InternalGetOrSet  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1406]
js_GetProperty  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsobj.c, line 3022]
js_Interpret  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 5614]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1270]
js_InternalInvoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1347]
js_InternalGetOrSet  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1406]
js_GetProperty  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsobj.c, line 3022]
js_Interpret  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 5614]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1270]
nsXPCWrappedJSClass::CallMethod  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp, line 1379]
nsXPCWrappedJS::CallMethod  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp, line 466]
SharedStub  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, line 147]
XPTC_InvokeByIndex  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102]
XPCWrappedNative::CallMethod  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2152]
XPC_WN_CallMethod  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1444]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1246]
js_Interpret  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 3886]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1270]
js_InternalInvoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1347]
JS_CallFunctionValue  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsapi.c, line 4189]
nsJSContext::CallEventHandler  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1426]
nsJSEventListener::HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/events/nsJSEventListener.cpp, line 195]
nsEventListenerManager::HandleEventSubType  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp, line 1653]
nsEventListenerManager::HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp, line 1760]
nsEventTargetChainItem::HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 335]
nsEventTargetChainItem::HandleEventTargetChain  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 478]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 405]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 392]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 392]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 392]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 392]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 392]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 392]
nsEventDispatcher::Dispatch  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 575]
nsXULCommandDispatcher::UpdateCommands  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/xul/document/src/nsXULCommandDispatcher.cpp, line 415]
nsGlobalWindow::UpdateCommands  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsGlobalWindow.cpp, line 4499]
nsFocusController::UpdateCommands  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsFocusController.cpp, line 217]
nsFocusController::SetSuppressFocus  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsFocusController.cpp, line 513]
PresShell::UnsuppressPainting  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp, line 4796]
nsDocShell::EndPageLoad  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/docshell/base/nsDocShell.cpp, line 4743]

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, I followed the steps in comment 4 with a current trunk Linux debug build.  I saw no changes to the UI (should I have?) and didn't get a crash.

That said, the stack is kinda useful.  Makes me suspect that bug 329181 is to blame.  Hard to tell how, though, without having a way to reproduce the crash.  :(  Any ideas on how I can do that?  How do I tell whether the extension is correctly installed, for one thing?

Comment 7

[Eyal Rozenberg]

Uh, note I said platform = windows XP. Dunno about Linux.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Well, I can't test on Windows, unfortunately....  Does that extension not work on other OSes?  Again, how do I tell whether the extension is properly installed?

Comment 9

[Eyal Rozenberg]

Oh yes, I forgot to mention you must be composing a text message, not an HTML message. Screenshot of the composition window with BiDi Mail UI operational:

http://bidiui.mozdev.org/images/new-message-ltr.png

And the extension does work on both Linux and Mac.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Aha!  There we go.  With text composition I do see the crash.  I was using a default profile for testing, as usual.  ;)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Well, the good news (for me) is that this is not a regression from bug 329181 (tested by local backout).

The bad news is that I'm not sure what it's a regression from.  ;)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Better bonsai query:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-03-06+06&maxdate=2006-03-07+10+&cvsroot=%2Fcvsroot

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK, testing by local backout indicates the problem is bug 234455.

Why, I'm not sure.  But the immediate cause of the crash is that we end up with a dead frame in ReResolveStyleContext somehow, which of course crashes....  :(

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Stack showing part of the problem

I added an assert that fires if nsFrame::Destroy is called while we're in the middle of style reresolution.  That happens in this case.  Basically, a child docloader fires its onload event (frame 149).  This triggers some script, which tries to get at docshell of an editor.  That triggers a style reresolve on the parent document, which causes a background image load to start and synchronously(!) stop.  The stop causes the parent docloader to fire its own onload in frame 60 (since the child is no longer busy by this point), which runs more script and triggers style changes.  Then it _again_ tries to get at the docshell of our editor, which again flushes style reresolves, which destroys some frames.  That's where we assert.

If we now continue, we unwind to the outer style reresolve, but that's now working with deleted frames and we crash.

I think I can fix docloader to prevent this by preventing onload firing on the outer docloader while the inner one is in the middle of its own onload handler, but what in the event patch causes a change in behavior here?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

The handling of NS_PAGE_LOAD seems to have changed.
Earlier it didn't go through even the capture phase.
There was this in the nsGenericElement (for capture phase)
aEvent->message != NS_PAGE_LOAD &&
aEvent->message != NS_SCRIPT_LOAD &&
aEvent->message != NS_IMAGE_LOAD &&
aEvent->message != NS_IMAGE_ERROR &&
aEvent->message != NS_SCROLL_EVENT
I think I need to add those back (at least temporarily), although that is wrong :(

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Now I was going debug this, but I can't get this crashing.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Prevent capture of some events

So, I can't reproduce the bug, but I think we should anyway put back 
this.
See the old version
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsGenericElement.cpp&rev=3.448#1965

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #17) 
> So, I can't reproduce the bug, but I think we should anyway put back 
> this.

And if somehow who can reproduce the bug could test whether the patch helps, that'd be great.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

grr, nsXULElement::HandleDOMEvent didn't have those checks
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xul/content/src/nsXULElement.cpp&rev=1.613#1859

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 214910 [details] [diff] [review]
Prevent capture of some events

This didn't help, and bz should have a real fix. (something not related to changes in 234455)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 214910 [details] [diff] [review]
Prevent capture of some events

This doesn't fix the crash for me, and I don't think we want this in general...

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase to not break (shouldn't assert or lead to similar crashes)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

I tried to reproduce this on the 1.8 branch without XUL, but the cache validation stuff imgLoader is just too complicated for me.  In any case, any time an image is in a loadgroup and doesn't need validation and gets used for a background image we can hit this bug.

Which means that I suspect we need this on the 1.8 branches, assuming this is safe and doesn't cause regressions.  This code is not exactly happy that way.  :(

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

I will likely not get to this before monday next week (mar 20)

Comment 25

[Darin Fisher]

Wouldn't this be better solved by making LoadImage not call RemoveRequest synchronously?  I'd be happy to take on that task, especially if we are only talking about a fix for Mozilla 1.9.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Wouldn't this be better solved by making LoadImage not call RemoveRequest
> synchronously?

Probably yes.  ;)

If you're willing to do that, though, that's great, and I agree that we should do it anyway.  I do think we might need _some_ fix for this on 1.8.1, if not 1.8.0.x, though.  :(

Comment 27

[Boris Zbarsky [:bzbarsky]]

Or at least, I'll need some sort of argument about when LoadImage can and cannot make a sync call to RemoveRequest to convince me that we don't need this on the branches.  As things stand, we're crashing because we touch deleted objects, and that's usually a bad sign.

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 214942 [details] [diff] [review]
Proposed patch

+        // It's ok if we're already in the list -- we'll just be in there twice
+        // and then the RemoveObject calls from ChildDoneWithOnload will remove
+        // us.

Maybe this comment should make it clearer that each RemoveObject will only remove one element, so this is expecting a balanced number of EnteringOnload and DoneWithOnload calls.

seems ok otherwise.

Comment 29

[Eyal Rozenberg]

The behavior of the text message composition window keeps getting weirder. The direction gets switched back and forth, the view/s are not available for JS access, problems with cursor movement, etc. etc.

Something is rotten in the kingdom of composermark.

Comment 30

[Eyal Rozenberg]

Oh, I should mention this:

https://bugzilla.mozilla.org/show_bug.cgi?id=330958

is probably a related issue, since that's what happens when I do

var contentDocument = document.getElementById("content-frame").contentDocument;
direction = contentDocument.defaultView.getComputedStyle(contentDocument.body, "").getPropertyValue("direction");

instead of the code I posted in comment 0.

Comment 31

[Darin Fisher]

Comment on attachment 214942 [details] [diff] [review]
Proposed patch

>Index: uriloader/base/nsDocLoader.h

>+    // Inform a parent docloader that aChild is about to call its onload
>+    // handler.
>+    PRBool ChildEnteringOnload(nsIDocumentLoader* aChild) {
>+        // It's ok if we're already in the list -- we'll just be in there twice
>+        // and then the RemoveObject calls from ChildDoneWithOnload will remove
>+        // us.
>+        return mChildrenInOnload.AppendObject(aChild);
>+    }

Should we be concerned about array growth?  Under what conditions
would this method be called more than once for a particular child?


sr=darin (sorry for not reviewing this sooner... i think it's not 
worth holding this patch for an improved imglib)

Comment 32

[Boris Zbarsky [:bzbarsky]]

Any time an onload handler causes recursion into itself, basically...  I'm not sure that's possible, but I'm not sure it's not either, so I figured I'd make the code just deal with it (and we'd crash when we overflow the stack if we really recurse infinitely).

Comment 33

[Boris Zbarsky [:bzbarsky]]

Fixed.