303981 - crash after following javascript link to open new window

Status: VERIFIED FIXED

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

Description

[Franklin Bynum]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050806 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050806 Firefox/1.0+

When you go to the page 
http://www.costco.com/Tires/SearchResult.aspx?IV=true&YW=2000&MA=HONDA&MD=Accord+DX+4+Dr.&SP=0&MN=3095&cat=3962&MNo=0
and click the link entitled "Click here for information on tire sizing or if you
are unsure of your vehicle's fitment requirements" it calls function
ShowWindow() which promptly crashes Firefox.  It spawns a popup window but
crashes before loading the intended page.

This is filed under Javascript because the page that the JS link is supposed to
open in the new window (http://www.costco.com/Tires/SizeInformation.aspx) does
not cause any problems.

Reproducible: Always

Steps to Reproduce:
1. Go to 
http://www.costco.com/Tires/SearchResult.aspx?IV=true&YW=2000&MA=HONDA&MD=Accord+DX+4+Dr.&SP=0&MN=3095&cat=3962&MNo=0
2. Click link near top of page "Click here for information on tire sizing or if
you are unsure of your vehicle's fitment requirements"

Actual Results:  
Program crashed

Expected Results:  
Page should have opened in popup window

Talkback ID numbers: TB8205791E and TB8205725Y

Comment 1

[Bob Clary [:bc] (inactive)]

Not JS Engine afaict, over to to DOM Core since the crashing line is 

PRInt32 namespaceID = aContent->GetCurrentDoc()->GetDefaultNamespaceID();

but that is just a guess.

nsHTMLDocument::MatchLinks(nsIContent * 0x041b9b48, int 0x00000000, nsIAtom *
0x00000000, const nsAString_internal & {...}) line 1700 + 14 bytes
nsContentList::Match(nsIContent * 0x041b9b48) line 699 + 36 bytes
nsContentList::PopulateWith(nsIContent * 0x041b9b48, int 0x00000001, unsigned
int & 0xffffffff) line 771 + 12 bytes
nsContentList::PopulateSelf(unsigned int 0xffffffff) line 868
nsContentList::BringSelfUpToDate(int 0x00000001) line 973
nsContentList::Length(int 0x00000001) line 418
nsContentList::GetLength(nsContentList * const 0x041ba458, unsigned int *
0x0012e7b0) line 489 + 10 bytes
XPTC_InvokeByIndex(nsISupports * 0x041ba458, unsigned int 0x00000004, unsigned
int 0x00000001, nsXPTCVariant * 0x0012e7b0) line 102
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_GETTER) line 2119 + 43 bytes
XPCWrappedNative::GetAttribute(XPCCallContext & {...}) line 1918 + 14 bytes
XPC_WN_GetterSetter(JSContext * 0x03df0d40, JSObject * 0x0398ad20, unsigned int
0x00000000, long * 0x04248e10, long * 0x0012ea94) line 1433 + 12 bytes
js_Invoke(JSContext * 0x03df0d40, unsigned int 0x00000000, unsigned int
0x00000002) line 1173 + 23 bytes
js_InternalInvoke(JSContext * 0x03df0d40, JSObject * 0x0398ad20, long
0x0398ad40, unsigned int 0x00000000, unsigned int 0x00000000, long * 0x00000000,
long * 0x0012f528) line 1270 + 20 bytes
js_InternalGetOrSet(JSContext * 0x03df0d40, JSObject * 0x0398ad20, long
0x00b2ea68, long 0x0398ad40, int 0x00000004, unsigned int 0x00000000, long *
0x00000000, long * 0x0012f528) line 1313 + 31 bytes
js_GetProperty(JSContext * 0x03df0d40, JSObject * 0x0398ad20, long 0x00b2ea68,
long * 0x0012f528) line 2843 + 51 bytes
js_Interpret(JSContext * 0x03df0d40, unsigned char * 0x04209735, long *
0x0012f61c) line 3290 + 1641 bytes
js_Execute(JSContext * 0x03df0d40, JSObject * 0x041ba930, JSScript * 0x04279d38,
JSStackFrame * 0x00000000, unsigned int 0x00000000, long * 0x0012f724) line 1403
+ 19 bytes
JS_EvaluateUCScriptForPrincipals(JSContext * 0x03df0d40, JSObject * 0x041ba930,
JSPrincipals * 0x037e4efc, const unsigned short * 0x04200338, unsigned int
0x00003e78, const char * 0x03e235c8, unsigned int 0x00000001, long * 0x0012f724)
line 3854 + 25 bytes
nsJSContext::EvaluateString(const nsAString_internal & {...}, void * 0x041ba930,
nsIPrincipal * 0x037e4ef8, const char * 0x03e235c8, unsigned int 0x00000001,
const char * 0x100da83c _js_default_str, nsAString_internal * 0x00000000, int *
0x0012f788) line 1060 + 67 bytes
nsScriptLoader::EvaluateScript(nsScriptLoadRequest * 0x03a82cc8, const nsString
& {...}) line 757
nsScriptLoader::ProcessRequest(nsScriptLoadRequest * 0x03a82cc8) line 658 + 22 bytes
nsScriptLoader::OnStreamComplete(nsScriptLoader * const 0x0420f73c,
nsIStreamLoader * 0x03886b18, nsISupports * 0x03a82cc8, unsigned int 0x00000000,
unsigned int 0x00003e78, const unsigned char * 0x04255018) line 1020
nsStreamLoader::OnStopRequest(nsStreamLoader * const 0x03886b1c, nsIRequest *
0x03bca778, nsISupports * 0x03a82cc8, unsigned int 0x00000000) line 137
nsHttpChannel::OnStopRequest(nsHttpChannel * const 0x03bca780, nsIRequest *
0x03d57458, nsISupports * 0x03a82cc8, unsigned int 0x00000000) line 4061
nsInputStreamPump::OnStateStop() line 507
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x03d5745c,
nsIAsyncInputStream * 0x04236160) line 343 + 11 bytes
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x041b952c) line 120
PL_HandleEvent(PLEvent * 0x041b952c) line 688 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00b3d790) line 623 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x003d017c, unsigned int 0x0000c0ee, unsigned int
0x00000000, long 0x00b3d790) line 1408 + 9 bytes
USER32! 77d48734()
USER32! 77d48816()
USER32! 77d489cd()
USER32! 77d48a10()
nsAppShell::Run(nsAppShell * const 0x00da8680) line 135
nsAppStartup::Run(nsAppStartup * const 0x00da85e0) line 145 + 26 bytes
XRE_main(int 0x00000003, char * * 0x003f7830, const nsXREAppData * 0x0042201c
kAppData) line 2324 + 35 bytes
main(int 0x00000003, char * * 0x003f7830) line 61 + 18 bytes
mainCRTStartup() line 338 + 17 bytes

	namespaceID	0x0012e5b4
-	aContent	0x041b9b48
+	    [nsHTMLHtmlElement]	{...}
+	    nsISupports	{...}
	    sTabFocusModel	0x00000007
	    sTabFocusModelAppliesToXUL	0x00000000
	    mParentPtrBits	0x00000000
	aNamespaceID	0x00000000
-	aAtom	0x00000000
+	   nsISupports	{...}
-	aData	{...}
	    mVTable	0x00343bb0 const nsObsoleteAStringThunk::`vftable'
+	    mData	0x00343ba4 ""
	    mLength	0x00000000
	    mFlags	0x00000001
-	ni	0x041b9aa8
+	    [nsNodeInfo]	{...}
+	    nsISupports	{...}
+	    mInner	{...}
+	    mIDAttributeAtom	{...}
+	    mOwnerManager	0x03db9730

Comment 2

[Bob Clary [:bc] (inactive)]

err, really assigning to dom core this time.

Comment 3

[timeless]

offhand, i believe this means GetCurrentDoc() is 0 and that'd be fairly unhappy

Comment 4

[Martijn Wargers (dead)]

Works in 2005-07-30 build, crashes in 2005-07-31 build. Probably a fall-out from
bug 296639.

Comment 5

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b4) Gecko/20050806 SeaMonkey/1.0a
Firefox 1.0.6 opens a pop-up, Seamonkey opens a new tab and resizes and
positions itself like the pop-up seen on Firefox 1.0.6.
I also noticed the page didn't stop loading first time.

view-source:http://www.costco.com/Tires/SizeInformation.aspx shows JS preceding
the HTML:
<script language="javascript1.1" src="/Coremetrics/v40/eluminate.js"
type="text/javascript"></script><script language="javascript1.1"
src="/Coremetrics/cmdatatagutils.inc" type="text/javascript"></script><script
language="javascript1.1"
type="text/javascript">cmSetProduction();cmCreatePageviewTag('/Tires/SizeInformation.aspx','','BC-Misc');</script><!--
page rendered by: 14 , at time: 8/9/2005 4:28:39 AM //-->
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN" >
<HTML>
.....

http://www.costco.com/Coremetrics/v40/eluminate.js
http://www.costco.com/Coremetrics/cmdatatagutils.inc

First file is a long one-liner seen only on horizontal scrolling,
2nd file is of type application/octet-stream.

Comment 6

[Johnny Stenback (:jst)]

Attachment: Make sure document always points to the current document. Also fixes bug 304459.

The reason for this crash was that the site ended up using a document object
that's no longer the current document, the reason for that was that we ended up
reusing the inner window here, but we never cleared the cached "document"
property, so the site was stuck with a reference to an old (about:blank)
document. This also fixes bug 304459 which is not really related, but since I
had to move the same scope clearing code around in both bugs I didn't see a
good way to post two separate patches. The Freeze/Thaw/IsFrozen changes are
specific to the fix for bug 304459.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 193083 [details] [diff] [review]
Make sure document always points to the current document. Also fixes bug 304459.

>+    // Make sure to clear scope on the outer window *before* we
>+    // initialize the new inner window. If we don't, things
>+    // (Object.prototype etc) could leak from the old outer to the new
>+    // inner scope.
>+    ::JS_ClearScope(cx, mJSObject);
>+    ::JS_ClearWatchPointsForObject(cx, mJSObject);

Nit: extra newline here.

>+    // Clear the regexp statics for the new page unconditionally.
>+    // XXX They don't get restored on the inner window when we go back.
>+    ::JS_ClearRegExpStatics(cx);

That way this paragraph stands out better.

So which bug was fixed by this JS_Clear* movement up to earlier in
SetNewDocument?

>+        // [This happens with Object.prototype when XPConnect creates
>+        // a temporary global while initializing classes; the reason
>+        // being that xpconnect creates the temp global w/o a parent
>+        // and proto, which makes the JS engine look up classes in
>+        // cx->globalObject, i.e. this outer windo].

Missing w in "windo".

>+
>+        Freeze();
>+
>+        mInnerWindow = nsnull;

Spacey!  Would it hurt to lose one of the blank lines here, and after the
InitClasses... call?

>+
>         nsresult rv = xpc->
>           InitClassesWithNewWrappedGlobal(cx, sgo, NS_GET_IID(nsISupports),
>                                           flags,
>                                           getter_AddRefs(mInnerWindowHolder));
>+
>+        Thaw();
>+
>         NS_ENSURE_SUCCESS(rv, rv);
> 
>         mInnerWindowHolder->GetJSObject(&newInnerWindow->mJSObject);
>       }
> 
>       if (currentInner && currentInner->mJSObject) {
>         if (mNavigator && !aState) {
>           // Hold on to the navigator wrapper so that we can set
>           // window.navigator in the new window to point to the same

[snip...]

>       // Give the new inner window our chrome event handler (since it
>       // doesn't have one).
>       newInnerWindow->mChromeEventHandler = mChromeEventHandler;
>     }
>-  }
> 
>-  if (scx && IsOuterWindow()) {

Isn't scx potentially null throughout its live extent?

>     // Add an extra ref in case we release mContext during GC.
>     nsCOMPtr<nsIScriptContext> kungFuDeathGrip = scx;
>     scx->GC();
> 
>     scx->DidInitializeContext();
>   }

Clearing sr request pending answers -- sorry if I'm not reading enough, I
didn't get enough sleep last night!

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 193083 [details] [diff] [review]
Make sure document always points to the current document. Also fixes bug 304459.

>Index: dom/src/base/nsGlobalWindow.cpp
>+        // cx->globalObject, i.e. this outer windo].




Nit: "outer window"

>Index: dom/src/base/nsGlobalWindow.h
>+  // This member is also used on both, but for slightly different

rep ,"on both windows", perhaps?
?spahrep ,
r=mrbkap

Comment 9

[Johnny Stenback (:jst)]

Um, what? :)

Comment 10

[Johnny Stenback (:jst)]

New patch coming up with brendan's comments addressed later today (hopefully).

Comment 11

[Johnny Stenback (:jst)]

(In reply to comment #7)
> So which bug was fixed by this JS_Clear* movement up to earlier in
> SetNewDocument?

That's needed for both, conceptually at least. For this bug we need to make sure
to clear a possibly cached "document" property on the outer (not that I've
verified that that's actually needed), for the other bug we need to clear scope
early to prevent Object.prototype from leaking through to the new inner. New
patch coming up that fixes the rest of your comments.

Comment 12

[Johnny Stenback (:jst)]

Attachment: Updated diff

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 193129 [details] [diff] [review]
Updated diff

Thanks, this makes more sense now, although I am looking forward to a 1.9-soon
patch to clean up control flow in SetNewDocument as discussed the other day!

/be

Comment 14

[Johnny Stenback (:jst)]

Fixed on trunk and branch.

Comment 15

[Martijn Wargers (dead)]

(In reply to comment #9)
> Um, what? :)
I think the weird comment you're seeing in comment 8, could be bug 305239

Comment 16

[Scott MacGregor]

I think this patch broke the preferences dialog on Mac OS X for Thunderbird and
Firefox. See Bug 305288 for more details. I haven't started to figure out why it
broke preferences yet.

Comment 17

[Asa Dotzler [:asa]]

Johnny, can you look at the regression over at bug 307255?