395897 - [FIX] Crash [@ nsWindowSH::SetProperty] with removing iframe and setting to a non-existing site

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P4)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build after 500ms.

This regressed between 2007-08-24 and 2007-08-25:
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=2007-08-24+04&maxdate=2007-08-25+09&cvsroot=%2Fcvsroot
I have no idea what caused it.

http://crash-stats.mozilla.com/report/index/865fa54a-6131-11dc-baf7-001a4bd46e84
0  	nsWindowSH::SetProperty(nsIXPConnectWrappedNative*, JSContext*, JSObject*, long, long*, int*)  	 mozilla/dom/src/base/nsDOMClassInfo.cpp:4529
1 	XPCWrapper::GetOrSetNativeProperty(JSContext*, JSObject*, XPCWrappedNative*, long, long*, int, int) 	mozilla/js/src/xpconnect/src/XPCWrapper.cpp:408
2 	XPC_XOW_GetOrSetProperty 	mozilla/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp:631
3 	XPC_XOW_SetProperty 	mozilla/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp:693
4 	js_NativeSet 	mozilla/js/src/jsobj.c:3490
5 	js_SetProperty 	mozilla/js/src/jsobj.c:3749
6 	js_Interpret 	mozilla/js/src/jsinterp.c:3865
7 	js_Invoke 	mozilla/js/src/jsinterp.c:1399
8 	js_InternalInvoke 	mozilla/js/src/jsinterp.c:1474
9 	JS_CallFunctionValue 	mozilla/js/src/jsapi.c:4940
10 	nsJSContext::CallEventHandler(nsISupports*, void*, void*, nsIArray*, nsIVariant**) 	mozilla/dom/src/base/nsJSEnvironment.cpp:1842
11 	nsGlobalWindow::RunTimeout(nsTimeout*) 	mozilla/dom/src/base/nsGlobalWindow.cpp:7106
12 	nsGlobalWindow::TimerCallback(nsITimer*, void*) 	mozilla/dom/src/base/nsGlobalWindow.cpp:7437
13 	nsTimerImpl::Fire() 	mozilla/xpcom/threads/nsTimerImpl.cpp:384
14 	nsTimerEvent::Run() 	mozilla/xpcom/threads/nsTimerImpl.cpp:457
15 	nsThread::ProcessNextEvent(int, int*) 	mozilla/xpcom/threads/nsThread.cpp:490
16 	NS_ProcessNextEvent_P(nsIThread*, int) 	nsThreadUtils.cpp:227
17 	nsBaseAppShell::Run() 	mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:154
18 	nsAppStartup::Run() 	mozilla/toolkit/components/startup/src/nsAppStartup.cpp:170
19 	XRE_main 	mozilla/toolkit/xre/nsAppRunner.cpp:3069
20 	main 	mozilla/browser/app/nsBrowserApp.cpp:153
21 	WinMain 	mozilla/browser/app/nsBrowserApp.cpp:166
22 	__tmainCRTStartup 	crtexe.c:589
23 	BaseProcessStart

Comment 1

[Martijn Wargers (dead)]

Ok, apparently the testcase is only crashing locally on the computer.

Comment 2

[Dave Townsend [:mossop]]

Looks like bug 395707 ?

Comment 3

[Martijn Wargers (dead)]

Well, it also happens with a clean profile (no NTT installed).

Comment 4

[Dave Townsend [:mossop]]

Well the stack looks identical and I've never reproduced it using the steps in bug 395707

Comment 5

[Martijn Wargers (dead)]

This is still crashing, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102005 Minefield/3.0a9pre

http://crash-stats.mozilla.com/report/index/f7150a61-7f5d-11dc-9273-001a4bd43e5c

Comment 6

[Johnny Stenback (:jst)]

I ran the testcase several times on both Win32 and Linux. No crash. Did this crash recently go away, or is it still happening?

Comment 7

[Martijn Wargers (dead)]

It's still crashing with current trunk build.
Remember, you need to download the testcase to your computer.
I think it's possible to make it crash online, I just need to get a 'Page not Found' page, but takes much longer online.

Comment 8

[Johnny Stenback (:jst)]

Attachment: Fix.

This fixes this crash. This is a simple problem of us trying to set the location property on a window that no longer has a docshell, and thus we end up getting a null location from the window and crash here dereferencing it.

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 286870 [details] [diff] [review]
Fix.

I don't really understand why window.frames[0] isn't undefined at that point :-/.

Comment 10

[Johnny Stenback (:jst)]

(In reply to comment #9)
> (From update of attachment 286870 [details] [diff] [review])
> I don't really understand why window.frames[0] isn't undefined at that point
> :-/.

Because it's been accessed before, and thus the property value is cached by the JS engine. If you look at nsWindowSH::GetProperty() in the place where it deals with int properties, it checks for the frame by index, but if it's not there, it returns w/o setting *vp, which would already be set to the property's old value by the JS engine. And I don't think we can change that easily w/o breaking sites that might use window[some_int] for something (of which there might be none, but who knows).

Comment 11

[Johnny Stenback (:jst)]

Fix checked in.

Comment 12

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111404
Minefield/3.0b2pre