XPConnect disturbs exception state of the call-context

VERIFIED FIXED in mozilla0.9.4

Status

()

Core
XPConnect
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: Robert Ginda, Assigned: David Bradley)

Tracking

Trunk
mozilla0.9.4
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Reporter)

Description

17 years ago
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?
(Reporter)

Comment 1

17 years ago
Created attachment 40377 [details]
eval.xul; testcase
(Reporter)

Comment 2

17 years ago
Created attachment 40378 [details] [diff] [review]
xpc.diff; proposed patch
(Assignee)

Comment 3

17 years ago
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.
Status: NEW → ASSIGNED
(Reporter)

Comment 4

17 years ago
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.
Looks pretty clean; maybe put some of this lore into a comment?

sr=shaver.
(Assignee)

Comment 6

17 years ago
r=dbradley
(Assignee)

Comment 7

17 years ago
Created attachment 41174 [details] [diff] [review]
Invalidates JS Proto's objects and creates new ones for wrappers
(Assignee)

Comment 8

17 years ago
Dang it did it again! Ignore the patch, wrong bug.
(Reporter)

Comment 9

17 years ago
Lore added, fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 10

17 years ago
Using Mozilla trunk binaries 20010706xx on WinNT, Linux, and Mac.
Marking Verified: Rob's eval.xul testcase now passes on all three platforms.
Status: RESOLVED → VERIFIED

Comment 11

17 years ago
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 12

17 years ago
Created attachment 42675 [details] [diff] [review]
small diff to reorder calls and fix a whitespace nit
(Reporter)

Comment 13

17 years ago
r=rginda
sr=jst
(Assignee)

Updated

17 years ago
Target Milestone: --- → mozilla0.9.3
(Assignee)

Comment 15

17 years ago
Moving out.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
(Assignee)

Comment 16

17 years ago
jband's patch is commited.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 17

17 years ago
Verified -
Status: RESOLVED → CLOSED

Comment 18

17 years ago
Oops, hit the wrong option button - reopeneng
Status: CLOSED → REOPENED
Resolution: FIXED → ---

Comment 19

17 years ago
Resolving FIXED, as above - 
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 20

17 years ago
And VERIFIED - 
Status: RESOLVED → VERIFIED
(Reporter)

Comment 21

17 years ago
do'h.  forgot about the case where the xpconnect call raises an exception in a
context that previously had none.  more patch coming up.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 22

17 years ago
Created attachment 47569 [details] [diff] [review]
patch clear the exception state on return, if there was no previous state

Comment 23

17 years ago
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

17 years ago
Created attachment 47621 [details] [diff] [review]
'enhancement' of rginda's previous patch
(Reporter)

Comment 25

17 years ago
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
(Assignee)

Comment 26

17 years ago
r=dbradley
Applied patch and excerised the app and all seems well.
(Reporter)

Comment 27

17 years ago
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 on attachment 47621 [details] [diff] [review]
'enhancement' of rginda's previous patch

sr=jst
Attachment #47621 - Flags: superreview+
Attachment #47621 - Flags: review+

Comment 29

17 years ago
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?
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.
(Reporter)

Comment 31

17 years ago
This is a stability issue for the debugger.  We won't lose any specific feature,
but we'll crash more.  
I'm good with this. a=brendan@mozilla.org on behalf of drivers.

/be
(Reporter)

Comment 33

17 years ago
Checked in.

Marking fixed (for the third time.)
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 34

17 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.