Components.utils.evalInSandbox should throw good exceptions

RESOLVED FIXED

Status

()

RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: axel, Assigned: mrbkap)

Tracking

({fixed1.8})

Trunk
fixed1.8
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

14 years ago
Created attachment 191355 [details] [diff] [review]
use caller source info, call SetExceptionWasThrown on error
(Reporter)

Comment 2

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

putting this on shaver's radar for feedback.
Attachment #191355 - Flags: review?(shaver)
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.
Attachment #191355 - Flags: review?(shaver) → review?(mrbkap)
(Assignee)

Comment 4

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

Comment 6

13 years ago
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.
Assignee: dbradley → mrbkap
Flags: blocking1.8b4?
(Assignee)

Updated

13 years ago
Attachment #191355 - Attachment is obsolete: true
Attachment #191355 - Flags: review?(mrbkap)

Updated

13 years ago
Flags: blocking1.8b4? → blocking1.8b4+
(Assignee)

Comment 7

13 years ago
Created attachment 193984 [details] [diff] [review]
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.
Attachment #193984 - Flags: review?(shaver)
(Assignee)

Comment 8

13 years ago
Created attachment 193986 [details] [diff] [review]
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.
Attachment #193984 - Attachment is obsolete: true
Attachment #193986 - Flags: review?(shaver)
(Assignee)

Updated

13 years ago
Attachment #193984 - Flags: review?(shaver)
Comment on attachment 193986 [details] [diff] [review]
one that actually compiles

r=shaver.  We'll fix this API yet!
Attachment #193986 - Flags: review?(shaver) → review+
(Assignee)

Comment 10

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

We probably want this on the branch as well.
Attachment #193986 - Flags: approval1.8b4?

Updated

13 years ago
Attachment #193986 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 11

13 years ago
Fix check into MOZILLA_1_8_BRANCH and trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
(Assignee)

Updated

13 years ago
Depends on: 306382
You need to log in before you can comment on or make changes to this bug.