303108 - Components.utils.evalInSandbox should throw good exceptions

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Axel Hecht]

Right now, evalInSandbox just throws an NS_ERROR_FAILURE on errors, it should
throw good exceptions. IMHO, using the source position of the caller is the 
right approach here, instead of the principal uri.

I have a patch that does just that.

I haven't gotten JSMSG_UNCAUGHT_EXCEPTION to work. That maps to JSEXN_NONE, and
I can't tell if it will be caught or not in the eval'd statement.
Components.utils.evalInSandbox("foo=Components.classes","about:blank") vs.
Components.utils.evalInSandbox("try{foo=Components.classes}catch(e){}","about:blank")

This may just be bug 66453, but I hope we can work around it.

Comment 1

[Axel Hecht]

Attachment: use caller source info, call SetExceptionWasThrown on error

Comment 2

[Axel Hecht]

Comment on attachment 191355 [details] [diff] [review]
use caller source info, call SetExceptionWasThrown on error

putting this on shaver's radar for feedback.

Comment 3

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

Comment on attachment 191355 [details] [diff] [review]
use caller source info, call SetExceptionWasThrown on error

mrbkap: does this fit with your pending patch?	Y'all should work together,
team-like and stuff.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 191355 [details] [diff] [review]
use caller source info, call SetExceptionWasThrown on error

>Index: xpccomponents.cpp
+	 if (frame) {
+	     frame->GetFilename(getter_Copies(filename));
+	     frame->GetLineNumber(&lineno);
+	 }

If we can't get the stack frame, do we want to fall back to what we did before
(using the codebase as the filename and 1 as the line number)?

Comment 5

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

Yes, I think we do.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

We should get the evalInSandbox stuff locked down for 1.5. I'll take this and
update the patch to my own review comment sometime today.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: handle uncaught exceptions too

Uncaught exceptions were being a pain since they were trying to be reported as
errors, but were unable to roundtrip back to being exceptions. With this patch,
we push a fake stack frame onto the sandcx so that we don't try to report
uncaught exceptions as errors.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: one that actually compiles

I moved the SetExceptionWasThrown call into the case where we actually set the
pending exception since I think it belongs there.

Comment 9

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

Comment on attachment 193986 [details] [diff] [review]
one that actually compiles

r=shaver.  We'll fix this API yet!

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 193986 [details] [diff] [review]
one that actually compiles

We probably want this on the branch as well.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Fix check into MOZILLA_1_8_BRANCH and trunk.