431082 - Crash [@ nsDocShell::DoChannelLoad] with DOMAttrModified removing window and editor and observes

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk builds within 500ms. 

The iframe source consists of this:
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
<box  id="a" observes="b">
  <box id="b"/>
</box>

<box  id="a"  src="javascript:"/>
<box id="b" src="javascript://"/>
<editor observes="a"/>

<script> 
function doe() {
window.addEventListener('DOMAttrModified', function() {window.frameElement.parentNode.removeChild(window.frameElement);}, true);
document.documentElement.appendChild(document.getElementsByTagName('box')[0]);
}
setTimeout(doe,500);
</script>
</window>

This regressed between 2008-03-20 and 2008-03-21:
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=2008-03-20+07&maxdate=2008-03-21+09&cvsroot=%2Fcvsroot
I guess a regression from bug 402983.

http://crash-stats.mozilla.com/report/pending/0ebf9b80-1496-11dd-8c6c-0013211cbf8a
(breakpad report is not there yet)

From my debug build:
>	docshell.dll!nsDocShell::DoChannelLoad(nsIChannel * aChannel=0x06214460, nsIURILoader * aURILoader=0x04253418, int aBypassClassifier=0)  Line 7616 + 0x21 bytes	C++
 	docshell.dll!nsDocShell::DoURILoad(nsIURI * aURI=0x052670d0, nsIURI * aReferrerURI=0x06944b10, int aSendReferrer=1, nsISupports * aOwner=0x0557ce90, const char * aTypeHint=0x00000000, nsIInputStream * aPostData=0x00000000, nsIInputStream * aHeadersData=0x00000000, int aFirstParty=0, nsIDocShell * * aDocShell=0x00000000, nsIRequest * * aRequest=0x0012e480, int aIsNewWindowTarget=0, int aBypassClassifier=0)  Line 7482 + 0x29 bytes	C++
 	docshell.dll!nsDocShell::InternalLoad(nsIURI * aURI=0x052670d0, nsIURI * aReferrer=0x06944b10, nsISupports * aOwner=0x0557ce90, unsigned int aFlags=0, const unsigned short * aWindowTarget=0x069ae490, const char * aTypeHint=0x00000000, nsIInputStream * aPostData=0x00000000, nsIInputStream * aHeadersData=0x00000000, unsigned int aLoadType=1, nsISHEntry * aSHEntry=0x00000000, int aFirstParty=0, nsIDocShell * * aDocShell=0x00000000, nsIRequest * * aRequest=0x00000000)  Line 7191 + 0x7f bytes	C++
 	docshell.dll!nsDocShell::LoadURI(nsIURI * aURI=0x052670d0, nsIDocShellLoadInfo * aLoadInfo=0x051e01e0, unsigned int aLoadFlags=0, int aFirstParty=0)  Line 904 + 0x56 bytes	C++
 	gklayout.dll!nsFrameLoader::ReallyStartLoading()  Line 211 + 0x35 bytes	C++
 	gklayout.dll!nsDocument::InitializeFinalizeFrameLoaders()  Line 3903	C++
 	gklayout.dll!nsDocument::EndUpdate(unsigned int aUpdateType=1)  Line 2748	C++
 	gklayout.dll!mozAutoDocUpdate::~mozAutoDocUpdate()  Line 66 + 0x22 bytes	C++
 	gklayout.dll!nsGenericElement::doInsertChildAt(nsIContent * aKid=0x055b3488, unsigned int aIndex=4, int aNotify=1, nsIContent * aParent=0x06918f08, nsIDocument * aDocument=0x0535f1d0, nsAttrAndChildArray & aChildArray={...})  Line 2765 + 0x12 bytes	C++
 	gklayout.dll!nsGenericElement::InsertChildAt(nsIContent * aKid=0x055b3488, unsigned int aIndex=4, int aNotify=1)  Line 2674 + 0x25 bytes	C++
 	gklayout.dll!nsXULElement::InsertChildAt(nsIContent * aKid=0x055b3488, unsigned int aIndex=4, int aNotify=1)  Line 1772	C++
 	gklayout.dll!nsGenericElement::doReplaceOrInsertBefore(int aReplace=0, nsIDOMNode * aNewChild=0x055b34a4, nsIDOMNode * aRefChild=0x00000000, nsIContent * aParent=0x06918f08, nsIDocument * aDocument=0x0535f1d0, nsIDOMNode * * aReturn=0x0012eb28)  Line 3407 + 0x1c bytes	C++
 	gklayout.dll!nsGenericElement::InsertBefore(nsIDOMNode * aNewChild=0x055b34a4, nsIDOMNode * aRefChild=0x00000000, nsIDOMNode * * aReturn=0x0012eb28)  Line 2989 + 0x20 bytes	C++
 	gklayout.dll!nsXULElement::InsertBefore(nsIDOMNode * newChild=0x055b34a4, nsIDOMNode * refChild=0x00000000, nsIDOMNode * * _retval=0x0012eb28)  Line 614 + 0x18 bytes	C++
 	gklayout.dll!nsGenericElement::AppendChild(nsIDOMNode * aNewChild=0x055b34a4, nsIDOMNode * * aReturn=0x0012eb28)  Line 508	C++
 	gklayout.dll!nsXULElement::AppendChild(nsIDOMNode * newChild=0x055b34a4, nsIDOMNode * * _retval=0x0012eb28)  Line 614 + 0x14 bytes	C++
 	xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000012, unsigned int methodIndex=2, unsigned int paramCount=1239832, nsXPTCVariant * params=0x00000000)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=18)  Line 2369 + 0x21 bytes	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2369 + 0x21 bytes	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x05512dc8, JSObject * obj=0x044012a0, unsigned int argc=1, long * argv=0x054e0c8c, long * vp=0x0012ede8)  Line 1473 + 0xe bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x05512dc8, unsigned int argc=1, long * vp=0x054e0c84, unsigned int flags=2048)  Line 1283 + 0x20 bytes	C
 	js3250.dll!js_Interpret(JSContext * cx=0x05512dc8)  Line 4837 + 0x16 bytes	C
 	js3250.dll!js_Invoke(JSContext * cx=0x05512dc8, unsigned int argc=1, long * vp=0x054e0c78, unsigned int flags=2)  Line 1299 + 0x9 bytes	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x05512dc8, JSObject * obj=0x03fc1a20, long fval=67046976, unsigned int flags=0, unsigned int argc=1, long * argv=0x041598c0, long * rval=0x0012f628)  Line 1355 + 0x18 bytes	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x05512dc8, JSObject * obj=0x03fc1a20, long fval=67046976, unsigned int argc=1, long * argv=0x041598c0, long * rval=0x0012f628)  Line 5053 + 0x1f bytes	C
 	gklayout.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x06186558, void * aScope=0x03fc1a20, void * aHandler=0x03ff0e40, nsIArray * aargv=0x054f25d4, nsIVariant * * arv=0x0012f6e0)  Line 1962 + 0x24 bytes	C++
 	gklayout.dll!nsGlobalWindow::RunTimeout(nsTimeout * aTimeout=0x0532a500)  Line 7753 + 0xab bytes	C++
 	gklayout.dll!nsGlobalWindow::TimerCallback(nsITimer * aTimer=0x0532a570, void * aClosure=0x0532a500)  Line 8087	C++
 	xpcom_core.dll!nsTimerImpl::Fire()  Line 400 + 0xe bytes	C++
 	xpcom_core.dll!nsTimerEvent::Run()  Line 492	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012f848)  Line 511	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x012b4020, int mayWait=1)  Line 227 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 170 + 0xc bytes	C++
 	tkitcmps.dll!nsAppStartup::Run()  Line 181 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=1, char * * argv=0x003ff750, const nsXREAppData * aAppData=0x003ffdf8)  Line 3170 + 0x25 bytes	C++
 	firefox.exe!NS_internal_main(int argc=1, char * * argv=0x003ff750)  Line 158 + 0x12 bytes	C++
 	firefox.exe!wmain(int argc=1, unsigned short * * argv=0x003fa060)  Line 87 + 0xd bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 583 + 0x19 bytes	C
 	firefox.exe!wmainCRTStartup()  Line 403	C
 	kernel32.dll!_BaseProcessStart@4()  + 0x23 bytes

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like a regression from bug 395609 or bug 420415.  smaug, it looks like mPrefs is null in the docshell (which means Create() never got called?  Or that the docshell is dead?).

Is it a problem that EnsureDocshell doesn't null mDocShell out on various failures?

Comment 2

[Samuel Sidler (old account; do not CC)]

Olli, do you have time to own this?

Comment 3

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

Attachment: proposed patch

Sorry this took so long.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 335943 [details] [diff] [review]
proposed patch

Though really, the QI to nsIBaseWindow can't fail.

Comment 5

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

Bah, something somewhere doesn't like the patch.
If I run the whole mochitest, I usually get some exception in 
content/events/test/test_bug328885.html coming from nsDocument::HasFocus.
All the tests do pass, but it is the mochitest framework which throws the error.
Problem is that I need to run lots of tests before that to be able to reproduce.
But without the patch I haven't got that exception (I think).
Will debug...

Comment 6

[Tomas]

Any progress?

Comment 7

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

Bah, this is tricky to debug.

Comment 8

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

It is my testcase which causes problems. I'll change it a bit and land this
on trunk.

Comment 9

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

(In reply to comment #8)
> It is my testcase which causes problems.
Or maybe not. Something is randomly failing because of the patch :(

Comment 10

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

Hmm, seems like it is the testcase after all. Even a bit modified testcase
which opens in a new window causes tests a lot later in mochitest to fail.

Comment 11

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

Attachment: better

I added the Destroy so that docshell gets removed properly from
parent. Create is needed so that right notifications are fired before Destroy.

To fix the problem, which the testcase causes, I had to add 
nsFocusUnsuppressEvent, so that when PresShell/CSSFC are destroyed, 
focussuppression is correctly removed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 352115 [details] [diff] [review]
better

If the test is just a crash test, could add it to crashtests instead.  Either way, though; this one is already written.

The rest looks good to me.

Comment 13

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

Well I thought it was just a crashtest, but it did find the focus suppressing bug, so it is useful to keep it as a mochitest.

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 352115 [details] [diff] [review]
better

{
patching file layout/base/nsCSSFrameConstructor.cpp
Hunk #1 FAILED at 1848
1 out of 4 hunks FAILED
patching file layout/base/nsCSSFrameConstructor.h
Hunk #1 FAILED at 148
1 out of 2 hunks FAILED
}

Comment 15

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

The patch applies cleanly when some --fuzz is used.
I'll land it today or tomorrow.

Comment 16

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081214 Minefield/3.2a1pre

Was the test checked in? I can't seem to find it with mxr.

Comment 17

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

The testcase was checked in (also to branch), and it is in hg http://hg.mozilla.org/mozilla-central/annotate/b858eeb64326/content/base/test/test_bug431082.html
but for some reason mxr can't find it :/

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #17)
> but for some reason mxr can't find it :/

bug 468698

Comment 19

[Marcia Knous [:marcia]]

Looks good using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20081217 Shiretoko/3.1b3pre, adding verified1.9.1 keyword.

Comment 20

[Daniel Veditz [:dveditz]]

This is a null dereference, we don't have to block the older releases on it. If you'd like to fix this crash anyway we'll approve a patch.

Comment 21

[Samuel Sidler (old account; do not CC)]

Dan: I think we want this per bug 477237 comment 15 and on.

Comment 22

[Daniel Veditz [:dveditz]]

Olli: does this patch apply to the 1.9.0 branch? Assuming it does, please request approval on it.