Closed
Bug 1042996
Opened 10 years ago
Closed 10 years ago
Remove custom Sandbox JSContext
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(2 files)
(deleted),
patch
|
gkrizsanits
:
review+
bobowen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
I found this patch in an old branch for bug 981218. I have yet to finish that bug, but we might as well get this into the tree for bug 951991. Right now we create a custom JSContext for every call to EvalInSandbox. We can just get rid of it.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8461163 -
Flags: review?(gkrizsanits)
Attachment #8461163 -
Flags: review?(bobowencode)
Assignee | ||
Comment 2•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=3ec01f780aa0
Assignee | ||
Updated•10 years ago
|
Attachment #8461163 -
Attachment description: Remove the special JSContext for Sandboxes. v1 → Part 2 - Remove the special JSContext for Sandboxes. v1
Assignee | ||
Comment 3•10 years ago
|
||
If setVersion() is not invoked on compileOptions, it ends up with JSVERSION_UNKNOWN, which invokes findVersion() on the JSContext, which does a bunch of crazy hunting of previous scripted stack frames that we most certainly don't want for sandboxes, which are supposed to be controlled environments. Using a separate JSContext in evalInSandbox isolates us from these effects, so once we stop doing that we need to be more explicit here.
Attachment #8461295 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 4•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1a6e77921e72
Comment 5•10 years ago
|
||
Comment on attachment 8461163 [details] [diff] [review] Part 2 - Remove the special JSContext for Sandboxes. v1 Review of attachment 8461163 [details] [diff] [review]: ----------------------------------------------------------------- So, as the SandboxPrivate isn't an nsIScriptGlobalObject, we just default to the SafeJSContext. I was wondering if we could do something like that here, thanks. Most of the remaining context pushing will need an AutoEntryScript I think. So, I'm waiting for jimb to land his patch as he seems pretty close, otherwise I'd have to revisit everything again. Looks good to me, apart from my comment and musings below. ::: js/xpconnect/src/Sandbox.cpp @@ +1479,5 @@ > RootedValue v(cx, UndefinedValue()); > RootedValue exn(cx, UndefinedValue()); > bool ok = true; > { > + mozilla::dom::AutoEntryScript entry(priv); Although it's obvious, comment saying why we need this and, more importantly, that it's Gecko-specific (which I hope I'm right in saying :-) ). I wonder if it would be worth adding another constructor to AutoEntryScript that just takes nsIGlobalObject*. It might simplify things a bit, as the way the current constructor works we can only do this on main thread anyway. I'll think about it when I add the Init function. I also wonder if it would be worth choosing a standard variable name, like what tends to happen for JSAutoCompartment. It's already got a couple of different ones. If you have a preference, I could do the change along with the Init, as I'd have to hit all the code anyway.
Attachment #8461163 -
Flags: review?(bobowencode) → review+
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #5) > Most of the remaining context pushing will need an AutoEntryScript I think. > So, I'm waiting for jimb to land his patch as he seems pretty close, > otherwise I'd have to revisit everything again. Ok. Jim, do you have a rough ETA? Given that this is a Q3 goal I don't want to let it sit for too long. ;-) > Although it's obvious, comment saying why we need this and, more > importantly, that it's Gecko-specific (which I hope I'm right in saying :-) > ). OK, I'll add the comment. > I also wonder if it would be worth choosing a standard variable name, like > what tends to happen for JSAutoCompartment. > It's already got a couple of different ones. > If you have a preference, I could do the change along with the Init, as I'd > have to hit all the code anyway. I would prefer aes. For some reason I didn't do that here - I'll fix that.
Flags: needinfo?(jimb)
Comment 7•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #6) > (In reply to Bob Owen (:bobowen) from comment #5) > > Most of the remaining context pushing will need an AutoEntryScript I think. > > So, I'm waiting for jimb to land his patch as he seems pretty close, > > otherwise I'd have to revisit everything again. > > Ok. Jim, do you have a rough ETA? Given that this is a Q3 goal I don't want > to let it sit for too long. ;-) Actually, I was thinking about this last night, I don't want to put extra pressure on Jim. It looks like he has quite a few complicated things to juggle for that bug and we don't need it for functional reasons for bug 951991. I'll start working on the patches with the current set-up. If I get a chance to do the AutoEntryScript changes before, I will and switch the patches round. If not, the follow-up changes will be pretty quick.
Flags: needinfo?(jimb)
Updated•10 years ago
|
Attachment #8461295 -
Flags: review?(gkrizsanits) → review+
Updated•10 years ago
|
Attachment #8461163 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 8•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ed20a6b9938d
Assignee | ||
Comment 9•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/60d9d4dffca4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa252d7f676e
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/60d9d4dffca4 https://hg.mozilla.org/mozilla-central/rev/fa252d7f676e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•