Closed Bug 146596 Opened 19 years ago Closed 19 years ago

Crash in JavaScript when catch parameter "hidden" by var

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jim-patterson, Assigned: brendan)

References

()

Details

(Keywords: crash, testcase)

Attachments

(2 files, 3 obsolete files)

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
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).
Component: XML → JavaScript Engine
Summary: Crash trying to send using XMLHTTPRequest → Crash in JavaScript trying to construct SOAP request
Confirming:
2002052208/trunk/Win2K -> TB6615967M.
2002052306/branch-RC3/W2K -> TB6616026Y
Severity: major → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, testcase
Summary: Crash in JavaScript trying to construct SOAP request → Crash in JavaScript trying to construct SOAP request M1RC3 Trunk
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?
Assignee: heikki → rogerl
QA Contact: petersen → pschwartau
Probably bad news: M099 -> TB6616930G, M098 -> TB6617047K
Taking.

/be
Assignee: rogerl → brendan
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.
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).
Summary: Crash in JavaScript trying to construct SOAP request M1RC3 Trunk → Crash in JavaScript when catch parameter hidden by var
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.
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.
Attached patch proposed minimal&correct fix (obsolete) — Splinter Review
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
Attachment #85041 - Attachment is obsolete: true
Attached patch diff -wu of last patch (obsolete) — Splinter Review
This should go in for 1.0 final.

/be
diff -wu coming right up.

/be
Attachment #85043 - Attachment is obsolete: true
All done, testsuite happy as ever (same results with unpatched js/src tree).

/be
Attachment #85044 - Attachment is obsolete: true
Comment on attachment 85045 [details] [diff] [review]
er, make that *this* patch for 1.0

sr=shaver.  Thanks.
Attachment #85045 - Flags: superreview+
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
Summary: Crash in JavaScript when catch parameter hidden by var → Crash in JavaScript when catch parameter "hidden" by var
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'.
Attachment #85045 - Flags: review+
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
Status: NEW → ASSIGNED
Checked into trunk, keeping open to provoke 1.0final checkin.

/be
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. 
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
Marking fixed, but a driver or two (other than me) should feel free to add
approval via the patch mgr.

/be
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment on attachment 85045 [details] [diff] [review]
er, make that *this* patch for 1.0

a=chofmann
Attachment #85045 - Flags: approval+
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.
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 -
Status: RESOLVED → VERIFIED
Flags: testcase+
You need to log in before you can comment on or make changes to this bug.