146596 - Crash in JavaScript when catch parameter "hidden" by var

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[jim-patterson]

Mozilla consistently crashes when it tries to send an XML request using the
XMLHttpRequest object from JavaScript. However, if the code is stepped through
carefully in Venkman, it doesn't crash, so this seems to be a timing issue of
some kind.

From the above web page, click on Send. You may want to point the URL somewhere
valid (it just points to 'localhost' because my ISP doesn't allow me to upload
scripts), but it will likely crash anyway.

You'll notice that the "open" call on the XMLHttpRequest object specifies not
asynchronous (i.e. synchronous), which is apparently not supported from what I
see in the code. However, a crash is not a good response.

Here is a traceback from a recent Linux build (source is about three days old).
It's also occurring in 1.0RC1 and on Windows:

#0  0x400cc059 in js_CheckRedeclaration (cx=0x8628d38, obj=0x0, id=137749992, 
    attrs=5, foundp=0xbfffdf7c) at jsinterp.c:1107
#1  0x400dc4be in js_Interpret (cx=0x8628d38, result=0xbfffe19c)
    at jsinterp.c:3244
#2  0x400cb1a7 in js_Invoke (cx=0x8628d38, argc=1, flags=2) at jsinterp.c:805
#3  0x400cb525 in js_InternalInvoke (cx=0x8628d38, obj=0x861b780, 
    fval=140621728, flags=0, argc=1, argv=0xbfffe518, rval=0xbfffe34c)
    at jsinterp.c:880
#4  0x4009996f in JS_CallFunctionValue (cx=0x8628d38, obj=0x861b780, 
    fval=140621728, argc=1, argv=0xbfffe518, rval=0xbfffe34c) at jsapi.c:3424
#5  0x410528d9 in nsJSContext::CallEventHandler (this=0x8628c88, 
    aTarget=0x861b780, aHandler=0x861b7a0, argc=1, argv=0xbfffe518, 
    aBoolResult=0xbfffe3e8, aReverseReturnResult=0) at nsJSEnvironment.cpp:1041
#6  0x41098f93 in nsJSEventListener::HandleEvent (this=0x87d0d68, 
    aEvent=0x87413f0) at nsJSEventListener.cpp:182
#7  0x41345693 in nsEventListenerManager::HandleEventSubType (this=0x87d0d30, 
    aListenerStruct=0x87d0dc8, aDOMEvent=0x87413f0, aCurrentTarget=0x8788318, 
    aSubType=4, aPhaseFlags=7) at nsEventListenerManager.cpp:1221
#8  0x4134616b in nsEventListenerManager::HandleEvent (this=0x87d0d30, 
    aPresContext=0x85a87b0, aEvent=0xbfffec50, aDOMEvent=0xbfffe9ec, 
    aCurrentTarget=0x8788318, aFlags=7, aEventStatus=0xbfffef24)
    at nsEventListenerManager.cpp:1396
#9  0x41590847 in nsGenericElement::HandleDOMEvent (this=0x87d0c28, 
    aPresContext=0x85a87b0, aEvent=0xbfffec50, aDOMEvent=0xbfffe9ec, aFlags=1, 
    aEventStatus=0xbfffef24) at nsGenericElement.cpp:1836
#10 0x41385eda in nsHTMLButtonElement::HandleDOMEvent (this=0x87d0c28, 
    aPresContext=0x85a87b0, aEvent=0xbfffec50, aDOMEvent=0x0, aFlags=1, 
    aEventStatus=0xbfffef24) at nsHTMLButtonElement.cpp:475
#11 0x41c7673f in PresShell::HandleEventInternal (this=0x85e1798, 
    aEvent=0xbfffec50, aView=0x0, aFlags=1, aStatus=0xbfffef24)
    at nsPresShell.cpp:6115
#12 0x41c7659d in PresShell::HandleEventWithTarget (this=0x85e1798, 
    aEvent=0xbfffec50, aFrame=0x87db5fc, aContent=0x87d0c28, aFlags=1, 
    aStatus=0xbfffef24) at nsPresShell.cpp:6084
#13 0x41353b42 in nsEventStateManager::CheckForAndDispatchClick (
    this=0x852da80, aPresContext=0x85a87b0, aEvent=0xbffff150, 
    aStatus=0xbfffef24) at nsEventStateManager.cpp:2641
#14 0x413511c0 in nsEventStateManager::PostHandleEvent (this=0x852da80, 
    aPresContext=0x85a87b0, aEvent=0xbffff150, aTargetFrame=0x87db5fc, 
    aStatus=0xbfffef24, aView=0x8788e30) at nsEventStateManager.cpp:1727
#15 0x41c76890 in PresShell::HandleEventInternal (this=0x85e1798, 
    aEvent=0xbffff150, aView=0x8788e30, aFlags=1, aStatus=0xbfffef24)
    at nsPresShell.cpp:6135
#16 0x41c76324 in PresShell::HandleEvent (this=0x85e1798, aView=0x8788e30, 
    aEvent=0xbffff150, aEventStatus=0xbfffef24, aForceHandle=0, 
    aHandled=@0xbfffeeb8) at nsPresShell.cpp:6038
#17 0x41f37abf in nsViewManager::HandleEvent (this=0x8687130, aView=0x8784650, 
    aEvent=0xbffff150, aCaptured=0) at nsViewManager.cpp:2074
#18 0x41f2ade5 in nsView::HandleEvent (this=0x8784650, aVM=0x8687130, 
    aEvent=0xbffff150, aCaptured=0) at nsView.cpp:305
#19 0x41f37237 in nsViewManager::DispatchEvent (this=0x8687130, 
    aEvent=0xbffff150, aStatus=0xbffff020) at nsViewManager.cpp:1881
#20 0x41f2a612 in HandleEvent (aEvent=0xbffff150) at nsView.cpp:80
#21 0x40b6c377 in nsWidget::DispatchEvent (this=0x8777268, aEvent=0xbffff150, 
    aStatus=@0xbffff0dc) at nsWidget.cpp:1432
#22 0x40b6bf5e in nsWidget::DispatchWindowEvent (this=0x8777268, 
    event=0xbffff150) at nsWidget.cpp:1320
#23 0x40b6c43d in nsWidget::DispatchMouseEvent (this=0x8777268, 
    aEvent=@0xbffff150) at nsWidget.cpp:1459
#24 0x40b6d207 in nsWidget::OnButtonReleaseSignal (this=0x8777268, 
    aGdkButtonEvent=0x8296b98) at nsWidget.cpp:1932
#25 0x40b7260e in nsWindow::OnButtonReleaseSignal (this=0x8777268, 
    aGdkButtonEvent=0x8296b98) at nsWindow.cpp:1621
#26 0x40b72987 in nsWindow::HandleGDKEvent (this=0x8777268, event=0x8296b98)
    at nsWindow.cpp:1717
#27 0x40b63a2e in dispatch_superwin_event (event=0x8296b98, window=0x8777268)
    at nsGtkEventHandler.cpp:958
#28 0x40b63703 in handle_gdk_event (event=0x8296b98, data=0x0)
    at nsGtkEventHandler.cpp:833

Comment 1

[jim-patterson]

As my traceback didn't implicate the XML components, I have tried to verify
whether they are in fact involved; it seems my initial conclusion was a bit
hasty. If I replace DOMParser and XMLHttpRequest with dummy objects, it still
crashes.

I've changed the category to JavaScript Engine, since at this point I can't be
more specific about what's triggering it. (I'll keep trying to narrow it down).

Comment 2

[Adam Hauner]

Confirming:
2002052208/trunk/Win2K -> TB6615967M.
2002052306/branch-RC3/W2K -> TB6616026Y

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Brendan, this is a case I can reproduce on Win2k, looks somewhat similar to bug
94102 but this still crashes. Loading the URL and just hitting send will get the
stack trace provided here.

I'll reassign to JS engine but this might be DOM stuff or something else as well.

Reporter, do you have any idea when this started?

Comment 4

[Adam Hauner]

Probably bad news: M099 -> TB6616930G, M098 -> TB6617047K

Comment 5

[Brendan Eich [:brendan]]

Taking.

/be

Comment 6

[jim-patterson]

Reply to Heikki - I noticed it a while ago, likely 0.9.9 or perhaps even 0.9.8.
However, it has always happened with this particular page or variations of it (I
just started playing with it a month or so ago).

Some of those Talkback crashes are probabally mine, too.

Comment 7

[jim-patterson]

I have tracked down the trigger for this bug. It seems to be simply entering a
try-catch block where there is a variable with the same name as the parameter to
the catch inside the catch block. Here's the simple case; calling this function
will trigger the crash:

function btnSend_onclick()
{
	try {
		return "A simple exception";
	}
	catch(e) {
		var e = "Another exception";
	}
	return '';
}

I have put the simpler test case on my public web-space (check URL above).

Comment 8

[Mike Shaver (:shaver emeritus)]

Attachment: Don't DEFVAR if var's name is shadowed by the catch exception variable

I'm not sure this is the right fix, but it seems viable.

It'd be nice if we could get LookupArgOrVar to tell us if it already decided we
were in the appropriate catch block, via an out param "reason", but that's a
more involved patch, and I want to make sure that this is the right approach
first.

I'll pull and run the test suite shortly.

Comment 9

[Mike Shaver (:shaver emeritus)]

I get 8 failures (the usual 8?) with this patch:

ecma_3/Function/15.3.4.3-1.js
ecma_3/RegExp/regress-123437.js
ecma_3/RegExp/regress-85721.js
js1_5/Array/regress-101964.js
js1_5/Exceptions/errstack-001.js
js1_5/Object/regress-90596-001.js
js1_5/Object/regress-90596-002.js
js1_5/Object/regress-90596-003.js

Rebuilding my branch browser with this patch now.

Comment 10

[Brendan Eich [:brendan]]

Attachment: proposed minimal&correct fix

Counter-example for shaver's patch:

function f(o){try{throw 42}catch(e){with(o){var e;print(e)}print(e)}print(e)}
f({e:24})

should print

24
42
undefined

and throw no uncaught exception.

I'll attach a diff -wu next to show how teeny this patch is.  Note also that
the only other place in jsemit.c where we CG_SWITCH_TO_PROLOG and emit a prolog
bytecode is for top-level functions, where we know the enclosing function or
script has a prolog (because heavyweight, if function encloses; all scripts may
have prologs).

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: diff -wu of last patch

This should go in for 1.0 final.

/be

Comment 12

[Brendan Eich [:brendan]]

Attachment: er, make that *this* patch for 1.0

diff -wu coming right up.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: diff -wu of last patch

All done, testsuite happy as ever (same results with unpatched js/src tree).

/be

Comment 14

[Mike Shaver (:shaver emeritus)]

Comment on attachment 85045 [details] [diff] [review]
er, make that *this* patch for 1.0

sr=shaver.  Thanks.

Comment 15

[Brendan Eich [:brendan]]

Jim, thanks for the testcase.  But note that var in a JS function makes a
function-scoped variable -- JS lacks block scope apart from for catch variables
within catch blocks.  Therefore, the catch variable hides the function-local
variable in your case.  I'm tweaking the summary.

/be

Comment 16

[John Bandhauer]

Comment on attachment 85045 [details] [diff] [review]
er, make that *this* patch for 1.0

r=jband.
Also, is there a bug on file for a strict warning when catch var names collide
with local var and arg names? Perhaps not since we wanted to support the idea
that catch vars in multiple catch blocks could share the same name without
appearing to be an 'error'.

Comment 17

[Brendan Eich [:brendan]]

I think jband's right in saying "Perhaps not ...."  A strict warning goes
against the intent of ECMA, although it might save someone grief due to bugs in
his or her script, or (as in this case) in the engine.

/be

Comment 18

[Brendan Eich [:brendan]]

Checked into trunk, keeping open to provoke 1.0final checkin.

/be

Comment 19

[jim-patterson]

The patch works fine for me. 

Thanks for the info on scoping, Brendan (and for the patch). It makes it even
more puzzling what the author was trying to do (I can't claim credit for it; I
was just porting it to Mozilla). I'm glad I could help you track it down, though. 

Comment 20

[Brendan Eich [:brendan]]

Checked into branch, too -- got a=chofmann,shaver,..., no one recorded approval
with the patch mgr, but I don't want to miss tomorrow morning's builds.

/be

Comment 21

[Brendan Eich [:brendan]]

Marking fixed, but a driver or two (other than me) should feel free to add
approval via the patch mgr.

/be

Comment 22

[chris hofmann]

Comment on attachment 85045 [details] [diff] [review]
er, make that *this* patch for 1.0

a=chofmann

Comment 23

[Phil Schwartau]

Testcase added to JS testsuite:

          mozilla/js/tests/js1_5/Regress/regress-146596.js

This includes both Jim's example in Comment #7, and
Brendan's example in Comment #10.

Comment 24

[Phil Schwartau]

Marking Verified Fixed. 

The above testcase passes in the current debug and optimized JS shell;
in addition, Mozilla now passes Jim's HTML testcase above -

I've also verified that the fix is on both the trunk and the 1.0 branch -