366828 - crash in venkman [@ nsContentTreeOwner::SetEnabled]

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Caio Tiago Oliveira (asrail)]

Attachment: Stacktrace of this bug

1 - Start SeaMonkey browser.
2 - Start Venkman within SeaMonkey.
3 - Trigger an exception or hit a breakpoint in Venkman.  Do not hit
"Continue".
4 - Leave Venkman open and close the Navigator window
5 - Close Venkman (in the 'x' at the window corner), clicking "OK" on the Debugging in progress dialog.

SeaMonkey crash at nsContentTreeOwner::SetEnabled with the attached stacktrace.


It's related to bug 235453, but I don't think any depends on another.

Comment 1

[timeless]

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpfe/appshell/src/nsChromeTreeOwner.cpp&rev=1.49&mark=256,554,562,149,151,153,155,157,159,191,268,274,283,320,362,367,372,377,382,388,394,399,404,415,426,431,436,441,446,451,458,466,471,476,514,523,

Comment 2

[Caio Tiago Oliveira (asrail)]

Attachment: Purposed fix to the bug

Something interesting I've seen is that Venkman won't quit if you follow comment #0 steps.
It will just stop debugging and you'll have to click the 'x' again or File->Quit.

I don't believe I've introduced this bug with the patch, because Venkman closes itself after clicking 'OK' in the 'debug is running' dialog if the browser still open.

Comment 3

[Caio Tiago Oliveira (asrail)]

Attachment: Purposed fix

Sorry, I've changed idea.

It now returns OK if the window had been closed.

It makes sense since we didn't had anything more to do.

Now it closes Venkman fine when following comment #0 steps.

Comment 4

[Caio Tiago Oliveira (asrail)]

Attachment: Another purposed fix

This fixes the bug, but still raising some warnings:


###!!! ASSERTION: Uh, IsInModalState() called w/o a reachable top window?: 'Error', file nsGlobalWindow.cpp, line 4906
vnk: Application venkman, 'JavaScript Debugger' unloading.

###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!

This is pretty much always bad. It usually means that native code is
making a callback to an interface implemented in JavaScript, but the
document where the JS object was created has already been cleared and the
global properties of that document's window are *gone*. Generally this
indicates a problem that should be addressed in the design and use of the
callback code.
: 'Error', file xpcwrappednativescope.cpp, line 597



Should we make it impossible to close the Navigator window while debugging JS in Venkman?

Comment 5

[Boris Zbarsky [:bzbarsky]]

You probably want to ask someone in particular for review....

Comment 6

[Caio Tiago Oliveira (asrail)]

Comment on attachment 251407 [details] [diff] [review]
Another purposed fix

It's obsolete, sorry for leaving it here.
I'll update it soon.

Comment 7

[Caio Tiago Oliveira (asrail)]

Attachment: Fix the possible crashes while using a closed window

Well...

the patch fixes the bug, but the way venkman handles the closing of the tab witht he page it was inspecting is still ugly.

It should be better to open the dialog about stopping the debugging  when closing the source tab (or the source window).
I don't know how to do this, yet, and I think it's another bug (since this one is a crash).

Comment 8

[Caio Tiago Oliveira (asrail)]

Comment on attachment 253907 [details] [diff] [review]
Fix the possible crashes while using a closed window

Since the touched code is under xpfe, asking for Neil's review.

Comment 9

[[e-mail]]

Comment on attachment 253907 [details] [diff] [review]
Fix the possible crashes while using a closed window

Notes: 1) NS_ENSURE_STATE should always be before nsCOMPtr<nsIDOMElement> (or whatever) 2) are you sure it doesn't need to be added anywhere else?

Comment 10

[Caio Tiago Oliveira (asrail)]

Attachment: Patch with changes regarding Neil's comment on the previous one

Assuming a bad extension writer or a mad extension writer, I'm guarding everything that could eventually get called in a closed window.
I also moved the ensure-state to most near as possible to the top of the methods (it is not on the top of them when there is another check condition which could exit).

Comment 11

[timeless]

just to make my life simpler, did you use comment 1 at all? :|

Comment 12

[[e-mail]]

Comment on attachment 254003 [details] [diff] [review]
Patch with changes regarding Neil's comment on the previous one

I can't actually reproduce your crash. Windows won't let me close a debugged window, and Linux crashes in ::JS_ClearScope instead.

Comment 13

[Caio Tiago Oliveira (asrail)]

timeless: Yes, off course. Why did you asked?

Neil...

this bug was about the crash you see because of the SetEnabled method.
Asqueela told me to guard all places, so I did it.

I already have crashed at JS_ClearScope too, inside the debugger:
[...]
#7  0xb7dc0a83 in JS_ClearScope (cx=0x9609c00, obj=0x97ae460) at jsapi.c:3222
#8  0xb690c939 in nsJSContext::ClearScope (this=0x9641c68, aGlobalObj=0x97ae460, aClearFromProtoChain=1) at nsJSEnvironment.cpp:2971
#9  0xb692eed6 in nsGlobalWindow::FreeInnerObjects (this=0x98380e8, aClearScope=1) at nsGlobalWindow.cpp:636
#10 0xb693174e in nsGlobalWindow::SetDocShell (this=0x975da18, aDocShell=0x0) at nsGlobalWindow.cpp:1693
#11 0xb6f90d54 in nsDocShell::Destroy (this=0x9641d80) at nsDocShell.cpp:3482
#12 0xb6725d3a in nsFrameLoader::Destroy (this=0x9641c48) at nsFrameLoader.cpp:274
#13 0xb655203c in nsSubDocumentFrame::Destroy (this=0x9779cc8) at nsFrameFrame.cpp:623
#14 0xb655445f in nsFrameList::DestroyFrames (this=0x9779c70) at nsFrameList.cpp:60
#15 0xb653d35a in nsContainerFrame::Destroy (this=0x9779c3c) at nsContainerFrame.cpp:281
#16 0xb6692955 in nsBoxFrame::Destroy (this=0x9779c3c) at nsBoxFrame.cpp:980
#17 0xb6692807 in nsBoxFrame::RemoveFrame (this=0x949c7e8, aListName=0x0, aOldFrame=0x9779c3c) at nsBoxFrame.cpp:1043
#18 0xb64f04a8 in nsFrameManager::RemoveFrame (this=0x8ea7444, aParentFrame=0x949c7e8, aListName=0x0, aOldFrame=0x9779c3c) at nsFrameManager.cpp:709
#19 0xb64b5fba in nsCSSFrameConstructor::ContentRemoved (this=0x8ea87c8, aContainer=0x9008ae0, aChild=0x96bdb48, aIndexInContainer=0, aInReinsertContent=0)
    at nsCSSFrameConstructor.cpp:9588
#20 0xb65072d0 in PresShell::ContentRemoved (this=0x8ea7428, aDocument=0x8ea3e50, aContainer=0x9008ae0, aChild=0x96bdb48, aIndexInContainer=0) at nsPresShell.cpp:4841
#21 0xb68d388c in nsBindingManager::ContentRemoved (this=0x8ea4760, aDocument=0x8ea3e50, aContainer=0x9008ae0, aChild=0x96bdb48, aIndexInContainer=0) at nsBindingManager.cpp:1387
#24 0xb67351e0 in nsGenericElement::RemoveChildAt (this=0x9008ae0, aIndex=0, aNotify=1) at nsGenericElement.cpp:2370
#25 0xb69b2ff0 in nsXULElement::RemoveChildAt (this=0x9008ae0, aIndex=0, aNotify=1) at nsXULElement.cpp:1055
#26 0xb672e501 in nsGenericElement::doRemoveChild (aOldChild=0x96bdb64, aParent=0x9008ae0, aDocument=0x8ea3e50, aReturn=0xbfee67b0) at nsGenericElement.cpp:2983
#27 0xb672e564 in nsGenericElement::RemoveChild (this=0x9008ae0, aOldChild=0x96bdb64, aReturn=0xbfee67b0) at nsGenericElement.cpp:2552
#28 0xb69b5a9f in nsXULElement::RemoveChild (this=0x9008ae0, oldChild=0x96bdb64, _retval=0xbfee67b0) at ./nsXULElement.h:576
#29 0xb7f297c9 in NS_InvokeByIndex () at xptiInterfaceInfo.cpp:73
#30 0xb6d6a13b in XPCWrappedNative::CallMethod (ccx=@0xbfee68dc, mode=XPCWrappedNative::CALL_METHOD) at xpcwrappednative.cpp:2215
#31 0xb6d76f5c in XPC_WN_CallMethod (cx=0x83c1f68, obj=0x95e0220, argc=1, argv=0x96536cc, vp=0xbfee69fc) at xpcwrappednativejsops.cpp:1470
#32 0xb7e0d832 in js_Invoke (cx=0x83c1f68, argc=1, flags=0) at jsinterp.c:1348
#33 0xb7e045fd in js_Interpret (cx=0x83c1f68, pc=0x906a7b7 ":", result=0xbfee6e0c) at jsinterp.c:4047
#34 0xb7e0d8a9 in js_Invoke (cx=0x83c1f68, argc=1, flags=2) at jsinterp.c:1367
#35 0xb6d6478a in nsXPCWrappedJSClass::CallMethod (this=0x917be30, wrapper=0x917be70, methodIndex=4, info=0x82abe98, nativeParams=0xbfee7140) at xpcwrappedjsclass.cpp:1419
#36 0xb6d5db85 in nsXPCWrappedJS::CallMethod (this=0x917be70, methodIndex=4, info=0x82abe98, params=0xbfee7140) at xpcwrappedjs.cpp:530
#37 0xb7f2a59b in PrepareAndDispatch (methodIndex=<value optimized out>, self=0x917bea8, args=<value optimized out>) at xptcstubs_gcc_x86_unix.cpp:95
[...]


So... this bug has a lot of associated stacktraces, since it's different for each of those methods.

And... yes, this patch fixes this bug too...

Comment 14

[[e-mail]]

(In reply to comment #13)
>timeless: Yes, off course. Why did you asked?
It looks as if you missed nsChromeTreeOwner::OnLocationChange?

>I already have crashed at JS_ClearScope too
OK, but that means that I won't grant r+ on the patch, only sr+

Comment 15

[Caio Tiago Oliveira (asrail)]

(In reply to comment #14)
> (In reply to comment #13)
> >timeless: Yes, off course. Why did you asked?
> It looks as if you missed nsChromeTreeOwner::OnLocationChange?

OnLocationChange is not called on a closed window...
it is a listener called when the URL bar or the active tab changes.

Is there any security issue involved?
If not, it's better to leave it alone.

> >I already have crashed at JS_ClearScope too
> OK, but that means that I won't grant r+ on the patch, only sr+
> 

Sorry, but...
the touched code is only on xpfe/appshell.
It's crashing somewhere, but it's because of the methods inside those two classes (it est, nothing should be done to a closed window).

Why couldn't you grant r+?
Who should do this?

Comment 16

[[e-mail]]

(In reply to comment #15)
>OnLocationChange is not called on a closed window...
Aren't you supposed to assume a bad or a mad extension writer?

>Why couldn't you grant r+?
>Who should do this?
I don't grant r+ on a bug I can't easily reproduce. timeless might grant r+.

Comment 17

[timeless]

Comment on attachment 254003 [details] [diff] [review]
Patch with changes regarding Neil's comment on the previous one

not covering all bases from evil extensions (or my original check list) isn't going to get an r+ from me :).

Comment 18

[Caio Tiago Oliveira (asrail)]

Attachment: Patch covering OnLocationChange (checked, a null mXULWindow can't be called anywhere)

Thanks a lot timeless.

Comment 19

[Caio Tiago Oliveira (asrail)]

Attachment: Patch covering OnLocationChange (checked, a null mXULWindow can't be called anywhere)

Wrong file above =/.

Comment 20

[timeless]

Comment on attachment 254079 [details] [diff] [review]
Patch covering OnLocationChange (checked, a null mXULWindow can't be called anywhere)

please talk w/ bz, but i don't like the change to nsChromeTreeOwner::FindItemWithName.

there's a difference between crashing and not allowing something that normally worked to work. (i'm nor asaying that _content or _main should crash, just that you don't need to check that early for just that one case). everything else basically involves the window mediator which is perfectly fine to use.

same concern about nsContentTreeOwner::FindItemWithName

Comment 21

[Boris Zbarsky [:bzbarsky]]

Yeah, I'd rather change the behavior of those methods as little as possible...

Comment 22

[Caio Tiago Oliveira (asrail)]

Attachment: Not breaking if it wouldn't actually crash

I've changed GetInterface and OnLocationChange as well.
Now it's allowing the extensions to work with closed windows except where it would actually crash.

Comment 23

[Boris Zbarsky [:bzbarsky]]

I'm not the right person to review this.  The only part of this code I know is the window-targeting code (which is what timeless asked me about).  The changes to that look fine.

Comment 24

[Caio Tiago Oliveira (asrail)]

Comment on attachment 255414 [details] [diff] [review]
Not breaking if it wouldn't actually crash

Asking again for timeless, so.

Comment 25

[[e-mail]]

Comment on attachment 255414 [details] [diff] [review]
Not breaking if it wouldn't actually crash

>+  if(aIID.Equals(NS_GET_IID(nsIPrompt))) {
>+    NS_ENSURE_STATE(mXULWindow);
>     return mXULWindow->GetInterface(aIID, aSink);
>+  }
>+  if(aIID.Equals(NS_GET_IID(nsIAuthPrompt))) {
>+    NS_ENSURE_STATE(mXULWindow);
>     return mXULWindow->GetInterface(aIID, aSink);
>+  }
>+  if(aIID.Equals(NS_GET_IID(nsIWebBrowserChrome))) {
>+    NS_ENSURE_STATE(mXULWindow);
>     return mXULWindow->GetInterface(aIID, aSink);
>+  }
>+  if (aIID.Equals(NS_GET_IID(nsIEmbeddingSiteWindow))) {
>+    NS_ENSURE_STATE(mXULWindow);
>     return mXULWindow->GetInterface(aIID, aSink);
>+  }
>+  if (aIID.Equals(NS_GET_IID(nsIEmbeddingSiteWindow2))) {
>+    NS_ENSURE_STATE(mXULWindow);
>     return mXULWindow->GetInterface(aIID, aSink);
>+  }
>+  if (aIID.Equals(NS_GET_IID(nsIXULWindow))) {
>+    NS_ENSURE_STATE(mXULWindow);
>     return mXULWindow->QueryInterface(aIID, aSink);
>+  }
This is a bit ugly but I guess it will get optimised into a big || anyway.

Comment 26

[Caio Tiago Oliveira (asrail)]

Attachment: Last patch changing the check in QueryInterface

Changing what you told Neil.

Comment 27

[[e-mail]]

Comment on attachment 255582 [details] [diff] [review]
Last patch changing the check in QueryInterface

Sorry, but I think this is worse, partly because you left your bracing changes in and partly because you've now testing the same condition twice.

Comment 28

[Caio Tiago Oliveira (asrail)]

Comment on attachment 255582 [details] [diff] [review]
Last patch changing the check in QueryInterface

I'll go back to the older version, so.

Comment 29

[Caio Tiago Oliveira (asrail)]

Comment on attachment 255414 [details] [diff] [review]
Not breaking if it wouldn't actually crash

Use this version so...

can someone check in this one?