Closed Bug 880917 Opened 11 years ago Closed 11 years ago

Move JS versioning from the cx to the compartment

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox24 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(13 files)

3.99 KB, patch
luke
: review+
Details | Diff | Splinter Review
31.10 KB, patch
luke
: review+
Details | Diff | Splinter Review
2.74 KB, patch
luke
: review+
Details | Diff | Splinter Review
6.43 KB, patch
luke
: review+
Details | Diff | Splinter Review
3.74 KB, patch
luke
: review+
Details | Diff | Splinter Review
2.62 KB, patch
luke
: review+
Details | Diff | Splinter Review
14.53 KB, patch
luke
: review+
Details | Diff | Splinter Review
1.99 KB, patch
luke
: review+
Details | Diff | Splinter Review
6.03 KB, patch
luke
: review+
Details | Diff | Splinter Review
4.15 KB, patch
luke
: review+
Details | Diff | Splinter Review
9.32 KB, patch
luke
: review+
Details | Diff | Splinter Review
9.39 KB, patch
luke
: review+
Details | Diff | Splinter Review
4.58 KB, patch
luke
: review+
Details | Diff | Splinter Review
I want to do this before doing bug 880330, because they're isomorphic problems, and regressions from this will be easier to spot. Also, smaug wants me to do this before landing bug 865745.
From irc: there is still an inherent stackiness to this (see JSContext::findVersion, which consumes cx->stack.currentScript->getVersion() and the versionOverride tragedy).  Some of it is I think essential (consulting the currently-running script's version), hopefully the override silliness can be removed altogether, and hopefully the defaultVersion would be a JSRuntime-wide value.
Summary: Move JS versionining from the cx to the compartment → Move JS versioning from the cx to the compartment
Green! Uploading patches and flagging for review.
A quick grep of Firebug indicates that it doesn't use this. And even if I
missed it, doing so from a debugger with the current semantics is a feature
we can't support going forward.
Attachment #761659 - Flags: review?(luke)
This will be useful for versioning, as well as JIT options and all the other
stuff that eventually needs to move out of the JSContext.
Attachment #761660 - Flags: review?(luke)
This has lower precedence than 'overrides' and running script, and higher
precedence than the cx's version. We can migrate the API consumers who clearly
want something like this, which will eventually let us remove the override
mechanism.
Attachment #761662 - Flags: review?(luke)
This test coverages seems to be mostly testing functionality we're removing like
overrides and version introspection. Let's just kill it.
Attachment #761664 - Flags: review?(luke)
Looks like cdleary added this back in bug 595691, as an equivalence to some
even-older-and-more-overly-cautious XBL code that was saving and restoring
the version across XBL calls. It doesn't seem like this should be an issue
anymore, and it's just a debugging aid to boot. Let's kill it.
Attachment #761665 - Flags: review?(luke)
Sandboxes always default to JSVERSION_DEFAULT in the browser. But XPCShell sets
up a ContextCallback that does JS_SetVersion(cx, JSVERSION_LATEST) on every
context that gets created, including the ephemerial Sandbox JSContexts. Since
httpd.js runs in xpcshell and evaluates SJS in a sandbox, we've (somewhat
accidentally) supported JSVERSION_LATEST in SJS, which certain SJS files have
taken advantage of. Let's continue to support it explicitly.
Attachment #761667 - Flags: review?(luke)
This test calls the version() shell command, and expects it to work like an
override, rather than the dumb compartment-mutator that I'm turning it into.
Let's just kill the test.
Attachment #761671 - Flags: review?(luke)
This doesn't do anything anymore. The compile options should generally carry
the right version through, with the exception of eval, which will end up using
the version of the running script anyway.
Attachment #761677 - Flags: review?(luke)
"compartment" being a JSAPI-specific term of art best avoided when possible, would you object to s/CompartmentOptions/GlobalOptions/ or GlobalObjectOptions?  (Or anything that says something about the options applying to stuff relevant to a global object, and not to a compartment, really.)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)
> "compartment" being a JSAPI-specific term of art best avoided when possible,
> would you object to s/CompartmentOptions/GlobalOptions/ or
> GlobalObjectOptions?  (Or anything that says something about the options
> applying to stuff relevant to a global object, and not to a compartment,
> really.)

From my perspective, that seems less clear. In XPConnect parlance, this stuff is per "scope", that is to say, it refers to a global and everything parented to that global. In JSAPI terms, I think Compartment is the best identifier here. It also maps well to the "CompartmentPrivate" that we use in the browser. 

This is going to be a sink for a lot of the random crap that lives on the JSContext (like JIT options, etc). 'GlobalObjectOptions' seems to imply that it has something to do with the global itself, which it really doesn't. So I'd kinda prefer to call it CompartmentOptions tbh.
XPConnect and the JSAPI are slightly incestuous, as one hacker once noted.  :-)  What's clear to you is not so clear to the average JSAPI user.

If we want to make compartments a primary concept in JSAPI, tho, what you say would make sense.  Right now they aren't, and the difference only shows up in rare cases like accessing reserved slots (where you have to know if the object you're accessing on is the object, or a wrapper around it).  This ties back into Luke's thoughts on removing requests and having enterScope(cx, object) as the JSAPI entry point, and then being able to get rid of cx->globalObject.  I don't know what I think about all this, except that if compartments become a primal JSAPI concept, that users can't just mostly ignore, we need a whole lot better documentation of them.
Attachment #761659 - Flags: review?(luke) → review+
Attachment #761660 - Flags: review?(luke) → review+
Comment on attachment 761662 [details] [diff] [review]
Part 3 - Introduce an API for callers to set the version for a compartment. v2

Instead of hasVersion, couldn't you use JSVERSION_UNKNOWN as the sentinel value to mean !hasVersion?  Instead, you could have a CompartmentOptions::hasVersion() const { return version == JSVERSION_UNKNOWN; }.
Attachment #761662 - Flags: review?(luke) → review+
Attachment #761664 - Flags: review?(luke) → review+
Attachment #761665 - Flags: review?(luke) → review+
Attachment #761667 - Flags: review?(luke) → review+
Attachment #761670 - Flags: review?(luke) → review+
Attachment #761671 - Flags: review?(luke) → review+
Attachment #761672 - Flags: review?(luke) → review+
Comment on attachment 761673 [details] [diff] [review]
Part 10 - Remove js_RevertVersion and associated shell functionality. v1

Oh but this was my favorite one!
Attachment #761673 - Flags: review?(luke) → review+
Comment on attachment 761674 [details] [diff] [review]
Part 11 - Remove JS_SetVersion and version override machinery. v1

with fire
Attachment #761674 - Flags: review?(luke) → review+
Attachment #761676 - Flags: review?(luke) → review+
Attachment #761677 - Flags: review?(luke) → review+
I agree with Waldo that "global" is the single JSAPI-facing concept that we want.  Before CPG, "compartment" made sense, since it was an independent concept, but after CPG, I'd ultimately like to remove the parlance "compartment" altogether (s/JSCompartment/js::Global/, s/cx->compartment/cx->global/) etc.

I see the distinction between global-the-object and global-the-partition, I just don't think it needs a whole new term.  In passing, I use phrases like "object A is inside global G" and there doesn't seem to be ambiguity.
But on second whisky, er, thought, I suppose we already have like a bajillion places where we say "compartment" so the compartment->global conversion would be best performed by one big renaming patch.  So, for now JS::CompartmentOptions seems fine and more consistent; we can sweep it up with the rest when/if that time comes.
(In reply to Luke Wagner [:luke] from comment #23)
> Comment on attachment 761662 [details] [diff] [review]
> Part 3 - Introduce an API for callers to set the version for a compartment.
> v2
> 
> Instead of hasVersion, couldn't you use JSVERSION_UNKNOWN as the sentinel
> value to mean !hasVersion?  Instead, you could have a
> CompartmentOptions::hasVersion() const { return version ==
> JSVERSION_UNKNOWN; }.

I didn't do this because the browser uses JSVERSION_UNKNOWN in various places, and if one of them made it into the JS engine, we'd probably want to assert/fail rather than just assuming no version was passed.
If that is the concern, can you just push to try a JS_ASSERT(!= JSVERSION_UNKNOWN) patch and check whether this happens?  Alternatively, we could add a new JSVERSION_UNSPECIFIED.
Oops, too late.
(In reply to Luke Wagner [:luke] from comment #30)
> If that is the concern, can you just push to try a JS_ASSERT(!=
> JSVERSION_UNKNOWN) patch and check whether this happens?  Alternatively, we
> could add a new JSVERSION_UNSPECIFIED.

I can do that in a followup patch if you'd like. It just didn't seem worth adding to the risk matrix of this landing.
Sure, that's fine; not a big deal.
Backed out for Android and B2G test bustage. Post-clobber builds showed the same failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d176e71ce2

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=71c1ce2cb0a4

Also, when this does re-land, please be sure to touch CLOBBER so we don't hit the testVersion bustage again.
Blocks: 882905
Depends on: 882932
(In reply to Ryan VanderMeulen [:RyanVM][UTC-4] from comment #34)
> Backed out for Android and B2G test bustage. Post-clobber builds showed the
> same failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d176e71ce2
> 
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=71c1ce2cb0a4

This turned out to be an automation issue, for which I've filed bug 882932. Once that lands, this can reland.

> Also, when this does re-land, please be sure to touch CLOBBER so we don't
> hit the testVersion bustage again.

I have added it to my local patch set.
Looks like evalInSandbox uses optional_argc, which can tell the difference between passing undefined and passing nothing at all.

https://tbpl.mozilla.org/?tree=Try&rev=b5caaf0d7a85
The dependent bug was pushed to inbound. Here's a try push on top of that:

https://tbpl.mozilla.org/?tree=Try&rev=58e89fac3e00
Hooray, looks like the httpd.js issues on mobile are fixed! Landing to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=d20fa876e95d
You need to log in before you can comment on or make changes to this bug.