328161 - More evalInSandbox cleanup

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P3)

Description

[Blake Kaplan (:mrbkap) (inactive)]

In bug 265740, I fixed a GC hazard and added some auto requests to the sandbox code. timeless pointed out that I'd missed one more place for a request and wanted to file a new bug, however I have some cleanup that I want to do first:
*) Instead of having to delete the temporary sandcx at every early return, use the newly created XPCAutoJSContext.
*) There are a couple of ways that attempt to propagate uncaught exceptions that no longer are needed thanks to JSOPTION_DONT_REPORT_UNCAUGHT.
*) The request stuff.

Patch next.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Implementation

Comment 2

[timeless]

Comment on attachment 212704 [details] [diff] [review]
Implementation

note that AutoJSSuspendRequest exists. and is generally the right thing to do instead of explciitly calling .EndRequest().

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

I suppose I could go with XPCAutoSuspendRequest, except that after the JS_EvaluateUCScriptForPrincipals, we no longer need to use the sandcx at all.

Comment 4

[Jesse Ruderman]

This probably needs to block 1.8.0.2.  See bug 328044 comment 5 and 6.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 212704 [details] [diff] [review]
Implementation

I botched the request stuff here anyway. I'll patch the other bug since JSOPTION_DONT_REPORT_UNCAUGHT doesn't exist there.

Comment 6

[Daniel Veditz [:dveditz]]

Looks like a dupe of bug 328044, and that one has a combined patch

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better implementation

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 212784 [details] [diff] [review]
Better implementation

Thot I r+'d this already...

/be

Comment 9

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

Comment on attachment 212784 [details] [diff] [review]
Better implementation

sr=shaver.  Blake to test an edge case we think should be fine.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.