88130 - XPConnect disturbs exception state of the call-context

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Robert Ginda]

The JS Debugger allows users to execute arbitrary javascript, entered in a
textbox.  It also keeps track of when scripts are created and destroyed, and
calls into javascript via xpconnect when these events happen.

The situation I'm seeing is as follows...
1) The user enters a script that would generate an exception, such as "throw 1",
or "bla" (where bla is not defined.)
2) A script create notification occurs.
3) An exception is thrown, and not caught by the script which threw it, leaving
cx->throwing set on the context.
4) the script is deleted, a script delete notification goes out
5) the script delete notification eventually makes it to the JS Debugger XPCOM
component <http://lxr.mozilla.org/mozilla/source/js/jsd/jsd_xpc.cpp#239>, where
a call is made that passes through XPConnect.

6) Because the call to eval(), and the implementation of the xpcom interface are
part of the same window, they share the same context.  At this step, the context
has cx->throwing set, from the exception generated by the call to eval(), but
xpconnect is about to use that context to make a call, which will clear
cx->throwing at xpcwrappedjsclass.cpp:976.

7) things are now in a bad state, the script delete notification happens
alright, but the exception generated by the eval() is lost, and what's worse,
the code following the eval() never runs.  Things just fail very quietly.

Save the attached .xul file to your local disk, and start mozilla with -chrome
file://path-to-testcase to see the problem for yourself.  You'll get a window
with two textboxes, the one on top accepts js code, while the one on the bottom
displays the result of script evaluation.  Type "1 + 1" and hit enter, and you
should see "2" in the bottom box, and the input box text should be cleared.  Now
type "throw 1", and the bottom box does not change, the the input text is not
cleared.  Finally, type "++1", and the bottom box should show "Caught exception:
SyntaxError: invalid increment operand".  Note that this exception is not
affected by this bug because it happens at parse time, before the script is
created, so no script notifications go out.

Could this show up in another situation?  What if some exception handling code
called across to an XPCOM component that happened to be in the same context?

Comment 1

[Robert Ginda]

Attachment: eval.xul; testcase

Comment 2

[Robert Ginda]

Attachment: xpc.diff; proposed patch

Comment 3

[David Bradley]

I think this may play into what we've been talking about in
http://bugzilla.mozilla.org/show_bug.cgi?id=83426. I'm adding jst to the list as
well. I'll take a look at the patch, thanks Rob.

Comment 4

[Robert Ginda]

This also makes "stop on throw" lose the exception when the user continues
execution.  A user can stop for an exception and muck around with the state of
their script, but when they continue, the exception is lost.  It makes venkman's
new "trace exceptions" and "break on exception" features mostly useless.  I'd
*really* like to check this patch in.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Looks pretty clean; maybe put some of this lore into a comment?

sr=shaver.

Comment 6

[David Bradley]

r=dbradley

Comment 7

[David Bradley]

Attachment: Invalidates JS Proto's objects and creates new ones for wrappers

Comment 8

[David Bradley]

Dang it did it again! Ignore the patch, wrong bug.

Comment 9

[Robert Ginda]

Lore added, fix checked in.

Comment 10

[Phil Schwartau]

Using Mozilla trunk binaries 20010706xx on WinNT, Linux, and Mac.
Marking Verified: Rob's eval.xul testcase now passes on all three platforms.

Comment 11

[John Bandhauer]

Rob, Thanks for fixing this. It was clearly my bad. I want to propose a minor 
change... Move the JS_RestoreExceptionState to happen before the JS_EndRequest. 
I'm having trouble thinking of a scenerio where it would really matter, but I 
think it is better to get the state restored correctly before the JS_EndRequest 
- which has the potential of unblocking blocked threads and doing who knows 
what. Oh, and who said you could add code with 'if (' here? :) I'll attack a 
little patch.

Comment 12

[John Bandhauer]

Attachment: small diff to reorder calls and fix a whitespace nit

Comment 13

[Robert Ginda]

r=rginda

Comment 14

[Johnny Stenback (:jst)]

sr=jst

Comment 15

[David Bradley]

Moving out.

Comment 16

[David Bradley]

jband's patch is commited.

Comment 17

[Phil Schwartau]

Verified -

Comment 18

[Phil Schwartau]

Oops, hit the wrong option button - reopeneng

Comment 19

[Phil Schwartau]

Resolving FIXED, as above - 

Comment 20

[Phil Schwartau]

And VERIFIED - 

Comment 21

[Robert Ginda]

do'h.  forgot about the case where the xpconnect call raises an exception in a
context that previously had none.  more patch coming up.

Comment 22

[Robert Ginda]

Attachment: patch clear the exception state on return, if there was no previous state

Comment 23

[John Bandhauer]

I thought this was a no brainer but remembered that I done something like this 
in the past. The code in CallQueryInterfaceOnJSObject had been doing this 
before. I saw that with your change from the initial patch in this bug the 
exception management in CallQueryInterfaceOnJSObject became redundant - we were 
doing twice the work required. 

It also occured to me that we are going to the trouble to save and restore the 
exception state *and* to cleanup any new exceptions that we might cause, but we 
were not being sure to clear the exception state *before* making our inner call 
to JS code. I've got myself convinced that we should be doing that cleanup too.

I'll attach a patch that includes rginda's patch to ensure cleanup on the way 
out, adds cleanup on the way in, and removes the redundant code from 
CallQueryInterfaceOnJSObject.

Opinions?

Comment 24

[John Bandhauer]

Attachment: 'enhancement' of rginda's previous patch

Comment 25

[Robert Ginda]

I'm embarassed to have missed the existing exception juggling in
CallQueryInterfaceOnJSObject, having changed a line just below it.  Maybe I need
larger fonts.

r=rginda

Comment 26

[David Bradley]

r=dbradley
Applied patch and excerised the app and all seems well.

Comment 27

[Robert Ginda]

jst, sr=?
I'd like to get this into 0.9.4 so we can distribute a debugger xpi that'll work
with whatever comes of the branch.

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 47621 [details] [diff] [review]
'enhancement' of rginda's previous patch

sr=jst

Comment 29

[chris hofmann]

how broken are we if we don't take the latest patch...
trying to shutdown 0.9.4 changes...
what should we do.  can we move this out?

Comment 30

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Please don't push this out.  Downstream application authors are begging for an
installable XPI of the debugger.  Can we take this on the branch?  It seems
pretty low risk and high-return for our consumers.

Comment 31

[Robert Ginda]

This is a stability issue for the debugger.  We won't lose any specific feature,
but we'll crash more.  

Comment 32

[Brendan Eich [:brendan]]

I'm good with this. a=[e-mail] on behalf of drivers.

/be

Comment 33

[Robert Ginda]

Checked in.

Marking fixed (for the third time.)

Comment 34

[Phil Schwartau]

Verified Fixed using Rob's testcase "eval.xul" above. It passes using
Mozilla trunk binaries 2001-09-09, 2001-09-10 on Linux, WinNT, and Mac.