Closed
Bug 880917
Opened 12 years ago
Closed 11 years ago
Move JS versioning from the cx to the compartment
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
Tracking | Status | |
---|---|---|
firefox24 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(13 files)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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.
Comment 1•12 years ago
|
||
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.
Updated•12 years ago
|
Summary: Move JS versionining from the cx to the compartment → Move JS versioning from the cx to the compartment
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Comment 5•12 years ago
|
||
Assignee | ||
Comment 6•12 years ago
|
||
Green! Uploading patches and flagging for review.
Assignee | ||
Comment 7•12 years ago
|
||
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)
Assignee | ||
Comment 8•12 years ago
|
||
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)
Assignee | ||
Comment 9•12 years ago
|
||
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)
Assignee | ||
Comment 10•12 years ago
|
||
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)
Assignee | ||
Comment 11•12 years ago
|
||
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)
Assignee | ||
Comment 12•12 years ago
|
||
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)
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #761670 -
Flags: review?(luke)
Assignee | ||
Comment 14•12 years ago
|
||
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)
Assignee | ||
Comment 15•12 years ago
|
||
Attachment #761672 -
Flags: review?(luke)
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #761673 -
Flags: review?(luke)
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #761676 -
Flags: review?(luke)
Assignee | ||
Comment 19•12 years ago
|
||
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)
Comment 20•12 years ago
|
||
"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.)
Assignee | ||
Comment 21•12 years ago
|
||
(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.
Comment 22•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #761659 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #761660 -
Flags: review?(luke) → review+
Comment 23•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #761664 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #761665 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #761667 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #761670 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #761671 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #761672 -
Flags: review?(luke) → review+
Comment 24•12 years ago
|
||
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 25•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #761676 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #761677 -
Flags: review?(luke) → review+
Comment 26•12 years ago
|
||
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.
Comment 27•12 years ago
|
||
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.
Assignee | ||
Comment 28•12 years ago
|
||
(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.
Assignee | ||
Comment 29•12 years ago
|
||
Comment 30•12 years ago
|
||
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.
Comment 31•12 years ago
|
||
Oops, too late.
Assignee | ||
Comment 32•12 years ago
|
||
(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.
Comment 33•12 years ago
|
||
Sure, that's fine; not a big deal.
Comment 34•12 years ago
|
||
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.
Assignee | ||
Comment 35•12 years ago
|
||
(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.
Assignee | ||
Comment 36•11 years ago
|
||
Attempting a hack around bug 880917:
https://tbpl.mozilla.org/?tree=Try&rev=a1a6918f1956
Assignee | ||
Comment 37•11 years ago
|
||
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
Assignee | ||
Comment 38•11 years ago
|
||
The dependent bug was pushed to inbound. Here's a try push on top of that:
https://tbpl.mozilla.org/?tree=Try&rev=58e89fac3e00
Assignee | ||
Comment 39•11 years ago
|
||
Assignee | ||
Comment 40•11 years ago
|
||
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
Comment 41•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/578a00c1e88f
https://hg.mozilla.org/mozilla-central/rev/4bf50c8f0f80
https://hg.mozilla.org/mozilla-central/rev/465e3423f463
https://hg.mozilla.org/mozilla-central/rev/ffe13f368958
https://hg.mozilla.org/mozilla-central/rev/150af0bb7cf0
https://hg.mozilla.org/mozilla-central/rev/a1643005a881
https://hg.mozilla.org/mozilla-central/rev/824c8b43b7e5
https://hg.mozilla.org/mozilla-central/rev/f3ce9e50920c
https://hg.mozilla.org/mozilla-central/rev/37392d2cb158
https://hg.mozilla.org/mozilla-central/rev/a13f6d42179e
https://hg.mozilla.org/mozilla-central/rev/c0c70e083935
https://hg.mozilla.org/mozilla-central/rev/0ddc9350501d
https://hg.mozilla.org/mozilla-central/rev/18540faa1b49
https://hg.mozilla.org/mozilla-central/rev/d20fa876e95d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Assignee | ||
Comment 42•11 years ago
|
||
Uplifted to 24 as a dep of bug 860085:
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=dd08f5dd367c
status-firefox24:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•