Closed Bug 1052139 Opened 10 years ago Closed 9 years ago

Try to lock down down the global prototype chain

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: bholley, Assigned: Waldo)

References

Details

Attachments

(8 files, 4 obsolete files)

Implement the ability to prevent modifying an extensible object's [[Prototype]]
15.16 KB, patch
bholley
: review+
efaust
: review+
Details | Diff | Splinter Review
Refactor global-object creation code to distinguish the unique self-hosting global from all other run-of-the-mill globals
9.93 KB, patch
till
: review+
Details | Diff | Splinter Review
bug1052139-mark-entered-compartments
835 bytes, patch
terrence
: review+
Details | Diff | Splinter Review
Patch to flip the immutable-prototype enforcement bit, enabling such enforcement
1.25 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Adjust tests to tolerate an immutable global object prototype chain
31.44 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Reorder CGCreateInterfaceObjectsMethod a bit to correspond to the ordering of generated code
10.54 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Perform proper failure handling in interfaces' CreateInterfaceObjects method, after dom::CreateInterfaceObjects has been called and possibly created the interface's constructor and prototype
5.52 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
The leftover bits of the old patch, adjusted atop the previous changes
15.89 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
Waldo believes that we might be able to get away with making the prototypes of Object.prototype, the GSP, and the |window| object non-mutable. This would certainly be awesome if we could do it, though I'm personally doubtful that it would be web-compatible.

I have committed to paying Waldo 50 dollars (or equivalent in some fungible commodity) if we can ship this.
"GPS"?
Oops. I meant: "GSP"?
(In reply to Mark S. Miller from comment #1)
> "GPS"?

Global Scope Polluter, which is Gecko jargon for the Named Properties Object [1], i.e. the object between Window and Object.prototype.

[1] http://heycam.github.io/webidl/#named-properties-object
Super-trivial in-shell testing says this does the trick.  No exposure of a JSAPI method to mark an object as having immutable prototype yet, tho.  And the patch is not necessarily bug-free.  But it's a substantial start.
This patch seems to do the trick, as far as the shell test can determine.  Haven't tryservered it yet, but https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fdbb1a1ae5c4 may be informative.  I am cautiously optimistic (mostly because it's unused except for the one test, but :-) ).
Attachment #8499599 - Flags: review?(efaustbmo)
Attachment #8499599 - Flags: review?(bobbyholley)
Attachment #8496316 - Attachment is obsolete: true
Comment on attachment 8499599 [details] [diff] [review]
Implement the ability to prevent modifying an extensible object's [[Prototype]]

Review of attachment 8499599 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I would want to see a few tests of this, both at the JSAPI level and the script level as well.

r=me with condition addressed.

::: js/src/vm/ObjectImpl.cpp
@@ +132,5 @@
>  
> +/* static */ bool
> +js::ObjectImpl::setImmutablePrototype(ExclusiveContext *cx, Handle<ObjectImpl*> obj, bool *succeeded)
> +{
> +    if (obj->asObjectPtr()->is<ProxyObject>()) {

This check is too restrictive. We want to be able to setImmutablePrototype on certain DOM proxies, which store their prototype in the normal place, and do not implement the traps. It's a little hokey, but this test should actually be |obj->getTaggedProto().isLazy()|. While we're at it, can we switch this, as well as JSObject::{get,set}Proto to using hasLazyPrototype() instead?

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +2127,5 @@
> +
> +    // For now, lacking an obvious place to store a bit, prohibit making an
> +    // Xray's [[Prototype]] immutable.  We can revisit this (or maybe give all
> +    // Xrays immutable [[Prototype]], because who does this, really?) later if
> +    // necessary.

I leave this to Bobby to comment on. The more I think about this, the less I understand why chrome wants to swizzle content object's prototypes in a content invisible way. If that's the case, *and* they want this feature, then perhaps another expando slot is called for? If not, we should remove the setPrototypeOf functionality as a followup.
Attachment #8499599 - Flags: review?(efaustbmo) → review+
Comment on attachment 8499599 [details] [diff] [review]
Implement the ability to prevent modifying an extensible object's [[Prototype]]

Review of attachment 8499599 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with those things fixed.

::: js/src/jsproxy.h
@@ +221,4 @@
>      virtual bool defaultValue(JSContext *cx, HandleObject obj, JSType hint, MutableHandleValue vp) const;
>      virtual void finalize(JSFreeOp *fop, JSObject *proxy) const;
>      virtual void objectMoved(JSObject *proxy, const JSObject *old) const;
> +    virtual bool setImmutablePrototype(JSContext *cx, HandleObject proxy, bool *succeeded) const;

I think it would make more sense to put this next to preventExtensions, since the code there is very analogous and repeated patterns will make for easier reading.

::: js/src/proxy/CrossCompartmentWrapper.cpp
@@ +408,5 @@
> +{
> +    RootedObject wrapped(cx, wrappedObject(wrapper));
> +    AutoCompartment call(cx, wrapped);
> +    return JSObject::setImmutablePrototype(cx, wrapped, succeeded);
> +}

I think this should look like the surrounding code:

PIERCE(cx, wrapper,
       NOTHING,
       Wrapper::setImmutablePrototype(cx, wrapper, succeeded),
       NOTHING);

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +1084,5 @@
>  }
>  
>  bool
> +ScriptedDirectProxyHandler::setImmutablePrototype(JSContext *cx, HandleObject proxy,
> +                                                  bool *succeeded) const

Wait, what? Why is this necessary? We don't want to expose this operation to the web, and for C++ purposes the code inherited from DirectProxyHandler should be enough. Please remove it unless I'm missing something.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +2122,5 @@
> +{
> +    // Do this only for non-SecurityWrapper-inheriting |Base|. See the comment
> +    // in getPrototypeOf().
> +    if (Base::hasSecurityPolicy())
> +        return Base::setImmutablePrototype(cx, wrapper, succeeded);

Again, this should look like preventExtensions, and this block should be removed.

@@ +2127,5 @@
> +
> +    // For now, lacking an obvious place to store a bit, prohibit making an
> +    // Xray's [[Prototype]] immutable.  We can revisit this (or maybe give all
> +    // Xrays immutable [[Prototype]], because who does this, really?) later if
> +    // necessary.

Yes, this is fine, just like preventExtensions.

As for efaust's comment - we _do_ have an expando slot (JSSLOT_EXPANDO_PROTOTYPE), so I'm not quite sure what you're referring to here.

As for why we want to do this - unless we want to prohibit setting prototypes on XrayWrappers (which may affect content-scripts and such), Xray semantics require that the change be visible to the caller's origin only.
Attachment #8499599 - Flags: review?(bobbyholley) → review+
Also, maybe this machinery should land separately (and thus with a different bug number) from the actual change to make Window prototypes immutable?

I haven't forgotten that I owe you 50 dollars (or the equivalent in beer/wine/spirits) if this works. If it does, it will be well worth it. :P
(In reply to Bobby Holley (:bholley) from comment #7)
> ::: js/src/jsproxy.h
> >      virtual void objectMoved(JSObject *proxy, const JSObject *old) const;
> > +    virtual bool setImmutablePrototype(JSContext *cx, HandleObject proxy, bool *succeeded) const;
> 
> I think it would make more sense to put this next to preventExtensions,
> since the code there is very analogous and repeated patterns will make for
> easier reading.

It seems very clear to both efaust and me that we want the [[Prototype]] access methods in a single spot so that people generally won't override one or two of them and not realize they must override the third.  Placing this next to getPrototypeOf and setPrototypeOf seems much more important than placing it next to preventExtensions for mild signature-similarity reasons.

> >  bool
> > +ScriptedDirectProxyHandler::setImmutablePrototype(JSContext *cx, HandleObject proxy,
> > +                                                  bool *succeeded) const
> 
> Wait, what? Why is this necessary? We don't want to expose this operation to
> the web, and for C++ purposes the code inherited from DirectProxyHandler
> should be enough. Please remove it unless I'm missing something.

Scripted direct proxies also include revocable proxies, as I discovered when writing the test that I forgot to include in this patch:

  var revocableTarget = {};
  var revocable = Proxy.revocable(revocableTarget, {});
  revocable.revoke();
  assertEq(setImmutablePrototype(revocable.proxy), true); // crashes without this hookok

There's a strong argument that DirectProxyHandler should include revocability in its ambit.  This would also (maybe?) allow collapsing DeadProxyObject into DirectProxyHandler.  But it doesn't, so right now this is necessary.

...actually, here's a second argument for setImmutablePrototype going by {get,set}PrototypeOf: if SDPH only overrides for setImmutablePrototype for revoked proxies, then the DPH base class would have to handle revoked proxies wrt {get,set}PrototypeOf.  But it/they don't.  So these are lovely little crashers now:

  var p = Proxy.revocable({}, {});
  p.revoke();
  Object.setPrototypeOf(p.proxy, null); // must throw, instead crashes
  Object.getPrototypeOf(p.proxy); // must throw, instead crashes

I'll extract out SDPH {get,set}PrototypeOf methods into a base patch/separate bug under this, to fix.

As for exposing the ability to make a [[Prototype]] immutable, we're not exposing it to the web.  (Although I little see reason not to standardize it and perhaps expose it as a primitive.)  But wherever it is exposed, we need sensible semantics on all objects.  And throwing if revoked, forwarding if not seems like the obvious sensible semantics for this.

> Again, this should look like preventExtensions, and this block should be
> removed.

I assume you mean isExtensible (which does *extensible = true; return true).

> @@ +2127,5 @@
> > +
> > +    // For now, lacking an obvious place to store a bit, prohibit making an
> > +    // Xray's [[Prototype]] immutable.  We can revisit this (or maybe give all
> > +    // Xrays immutable [[Prototype]], because who does this, really?) later if
> > +    // necessary.
> 
> As for efaust's comment - we _do_ have an expando slot
> (JSSLOT_EXPANDO_PROTOTYPE), so I'm not quite sure what you're referring to
> here.

We have no spot to store a prototype-is-immutable bit right now.  We could add another slot here to store that single bit, but it's probably not worth it.  The better fix as I refer to would be saying, "No, you can't mutate the [[Prototype]] of an Xray, ever." and having this method always report success as a consequence.

> As for why we want to do this - unless we want to prohibit setting
> prototypes on XrayWrappers (which may affect content-scripts and such), Xray
> semantics require that the change be visible to the caller's origin only.

Why would making the [[Prototype]] of an Xray immutable affect content scripts at all?  Or do you mean something by content-script, different from scripts running in the pages themselves?
(In reply to Bobby Holley (:bholley) from comment #8)
> Also, maybe this machinery should land separately (and thus with a different
> bug number) from the actual change to make Window prototypes immutable?

It'll land separately.  I don't much care about the same-bug, different-bug thing, tho.  So it'll probably be here with a leave-open in the meantime or so.

Also, depending on how Gecko goes, it may be the case that we'll need/want more API to set immutable prototype than this basic code provides.  Creating objects with immutable [[Prototype]] ab initio, rather than changing later, is better in some ways, maybe better enough to make worth supporting with API.  I dunno.  I haven't actually tried to change any mutability of stuff yet, just implemented the architecture so that immutable [[Prototype]]s work/are enforced if they occur.

> I haven't forgotten that I owe you 50 dollars (or the equivalent in
> beer/wine/spirits) if this works. If it does, it will be well worth it. :P

Honestly, at this point I don't even remember you saying this, so I can't quite in good faith accept it.  If it comes to it, donate it to Mozilla or something.  :-)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> > I think it would make more sense to put this next to preventExtensions,
> > since the code there is very analogous and repeated patterns will make for
> > easier reading.
> 
> It seems very clear to both efaust and me that we want the [[Prototype]]
> access methods

This is no more of a prototype access method than preventExtensions, right? Both of them set a bit on the shape the prevents the prototype from being modified. The only difference is that preventExtensions also prevents other things from being modified.

> in a single spot so that people generally won't override one
> or two of them and not realize they must override the third.

If they need to override setImmutablePrototype, they also need to override preventExtensions (per the above), so the issue is moot.

> for mild signature-similarity reasons.

I'm talking about behavioral reasons. They operate very similarly, and having that logic side-by-side is important.

> 
> > >  bool
> > > +ScriptedDirectProxyHandler::setImmutablePrototype(JSContext *cx, HandleObject proxy,
> > > +                                                  bool *succeeded) const
> > 
> > Wait, what? Why is this necessary? We don't want to expose this operation to
> > the web, and for C++ purposes the code inherited from DirectProxyHandler
> > should be enough. Please remove it unless I'm missing something.
> 
> Scripted direct proxies also include revocable proxies, as I discovered when
> writing the test that I forgot to include in this patch:

But why do we need setImmutablePrototype to operate on scripted proxies at all? It seems to me like we should just throw in that case. It's just a shell function, and it seems like we shouldn't implement that functionality just for the sake of writing tests for it. Or am I missing something?

>   var revocableTarget = {};
>   var revocable = Proxy.revocable(revocableTarget, {});
>   revocable.revoke();
>   assertEq(setImmutablePrototype(revocable.proxy), true); // crashes without
> this hook

Per the above, that seems self-inflicted.

> So these are lovely little crashers now:
> 
>   var p = Proxy.revocable({}, {});
>   p.revoke();
>   Object.setPrototypeOf(p.proxy, null); // must throw, instead crashes
>   Object.getPrototypeOf(p.proxy); // must throw, instead crashes

I don't understand what this has to do with setImmutablePrototype.

> As for exposing the ability to make a [[Prototype]] immutable, we're not
> exposing it to the web.  (Although I little see reason not to standardize it
> and perhaps expose it as a primitive.)  But wherever it is exposed, we need
> sensible semantics on all objects.

In general, I'm only interested in exposing it to C++. Exposing it to the js shell is only useful for objects where it doesn't cause more problems than it solves.

> > Again, this should look like preventExtensions, and this block should be
> > removed.
> 
> I assume you mean isExtensible (which does *extensible = true; return true).

No, I mean preventExtensions, which throws (which is exactly what we should  do).

> We have no spot to store a prototype-is-immutable bit right now.  We could
> add another slot here to store that single bit, but it's probably not worth
> it.  The better fix as I refer to would be saying, "No, you can't mutate the
> [[Prototype]] of an Xray, ever." and having this method always report
> success as a consequence.

I think we should throw, just like we do for preventExtensions.
> 
> > As for why we want to do this - unless we want to prohibit setting
> > prototypes on XrayWrappers (which may affect content-scripts and such), Xray
> > semantics require that the change be visible to the caller's origin only.
> 
> Why would making the [[Prototype]] of an Xray immutable affect content
> scripts at all?  Or do you mean something by content-script, different from
> scripts running in the pages themselves?

I mean jetpack/greasemonkey scripts that operate on page content over XrayWrappers, often including JS libraries that munge prototypes.
(In reply to Bobby Holley (:bholley) from comment #11)
> This is no more of a prototype access method than preventExtensions, right?
> Both of them set a bit on the shape the prevents the prototype from being
> modified. The only difference is that preventExtensions also prevents other
> things from being modified.

It's better to think of an object's [[Prototype]] as a special, hidden property, accessible and modifiable via a fascicle of operations: {get,set}PrototypeOf and setImmutablePrototype.  All access goes through them, and they stand and fall in concert.  That setImmutablePrototype is *sometimes* implemented similar to preventExtensions is irrelevant.  I honestly don't understand why that matters -- at the writing stage, very slightly, but location is far more relevant to the *reading* stage, and then adjacency to {get,set}PrototypeOf matters more.

To the extent that preventExtensions *also* does what setImmutablePrototype does (as almost an afterthought), some adjacency makes sense.  I could move preventExtensions and isExtensible by the [[Prototype]] triad.  But that adjacency is less important than setImmutablePrototype's adjacency to {get,set}PrototypeOf.

> If they need to override setImmutablePrototype, they also need to override
> preventExtensions (per the above), so the issue is moot.

Not so.  That one means to ensure [[Prototype]] immutability exists, doesn't mean there can't be others.  And what's important is whether any means to ensure this *works*.  That only requires that setPrototypeOf be consistent with both.

> They operate very similarly, and having that logic side-by-side is important.

There's no requirement they do so.

> But why do we need setImmutablePrototype to operate on scripted proxies at
> all? It seems to me like we should just throw in that case. It's just a
> shell function, and it seems like we shouldn't implement that functionality
> just for the sake of writing tests for it. Or am I missing something?

We're only adding to JS the ability to make a [[Prototype]] link immutable if it'll be standardized in TC39.  Making that happen requires serious effort to anticipate and address questions that would arise then.  Interaction with scripted proxies would certainly arise.  There's clear, simple baseline behavior: throw if revoked, else forward to target.  At a minimum we should do that.  (ES6 may want to permit handlers to override it, but those precise details are flourishes to defer til after the concept is accepted.)

Additionally, there's been a push to make every DOM object capability, also available to JS proxies.  That requires this hook work on scripted proxies.  There's an obvious way to define it and no reason not to.

> In general, I'm only interested in exposing it to C++.

That's fine.  I care about the JS language/standardization aspects as well.

> I mean preventExtensions, which throws (which is exactly what we should do).

The contract of setImmutablePrototype is that it returns a success/failure boolean, or throws if the proxy itself says no for some reason.  That's also the spec's contract for preventExtensions.  Thanks for reminding me that we need to fix that.

> I mean jetpack/greasemonkey scripts that operate on page content over
> XrayWrappers, often including JS libraries that munge prototypes.

You're certain that functionality can't be provided any other way?  There's a cost to providing and supporting this, and it seems quite disproportionate to just having libraries included in this esoteric case "not do that".
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)
> (In reply to Bobby Holley (:bholley) from comment #11)
> > This is no more of a prototype access method than preventExtensions, right?
> > Both of them set a bit on the shape the prevents the prototype from being
> > modified. The only difference is that preventExtensions also prevents other
> > things from being modified.
> 
> It's better to think of an object's [[Prototype]] as a special, hidden
> property, accessible and modifiable via a fascicle of operations:
> {get,set}PrototypeOf and setImmutablePrototype.  All access goes through
> them, and they stand and fall in concert.

Except for preventExtensions right? If that trap succeeds, a subsequent setPrototypeOf must fail. I don't see how those can be separated.

>  That setImmutablePrototype is
> *sometimes* implemented similar to preventExtensions is irrelevant.  I
> honestly don't understand why that matters -- at the writing stage, very
> slightly, but location is far more relevant to the *reading* stage

I'm talking about the reading stage, where I'm scanning through a proxy handler to see what it does. We have two traps on the proxy handler that are similar in the following ways:
* They share the same signature.
* In the simple case, they forward their call through wrappers until they hit a JSObject, at which point they set a bit on the shape.
* Invoking them has an effect on the required behavior of setPrototypeOf.
* If I override one, I probably want to overwrite the other.

> To the extent that preventExtensions *also* does what setImmutablePrototype
> does (as almost an afterthought), some adjacency makes sense.  I could move
> preventExtensions and isExtensible by the [[Prototype]] triad.

This sounds like a fine compromise, honestly. Let's do that and move on please.

> > They operate very similarly, and having that logic side-by-side is important.
> 
> There's no requirement they do so.

In our implementation they do and are likely to continue to do so, which is what matters for now. If this gets added to the spec as a proxy trap, _and_ the spec defines an ordering of these traps, _and_ we decide to reorder our traps to match the spec (because they currently don't), I'm happy to realign them.
 
> We're only adding to JS the ability to make a [[Prototype]] link immutable
> if it'll be standardized in TC39.  Making that happen requires serious
> effort to anticipate and address questions that would arise then. 
> Interaction with scripted proxies would certainly arise.  There's clear,
> simple baseline behavior: throw if revoked, else forward to target.  At a
> minimum we should do that.  (ES6 may want to permit handlers to override it,
> but those precise details are flourishes to defer til after the concept is
> accepted.)

Fair.

> > I mean jetpack/greasemonkey scripts that operate on page content over
> > XrayWrappers, often including JS libraries that munge prototypes.
> 
> You're certain that functionality can't be provided any other way?  There's
> a cost to providing and supporting this, and it seems quite disproportionate
> to just having libraries included in this esoteric case "not do that".

Then the libraries break, and people wonder why they can't use prototype.js in their jetpack addon. See bug 787070 as an example where we went through an immense amount of effort to provide logical prototype semantics over XrayWrappers.

XrayWrappers are an abstraction that we are committed to providing and getting 100% right. That abstraction dictates the prototype semantics that we have now, and so far I haven't heard a compelling reason to change it.
Initial patches adding the hook and rearranging the hooks landed:

https://bugzilla.mozilla.org/show_bug.cgi?id=1052139
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eee647a5bd4

Next step is to start working on making all the global prototype chain's elements have immutable [[Prototype]].  More patches soon.

(In reply to Bobby Holley (:bholley) from comment #13)
> Except for preventExtensions right? If that trap succeeds, a subsequent
> setPrototypeOf must fail. I don't see how those can be separated.

Ultimately preventExtensions will probably call setImmutablePrototype, so there'd be ~no connection.  (Scripted direct proxies would end by calling setImmutablePrototype on the target and complaining if an inconsistency were detected.)  But one step at a time.

> I'm talking about the reading stage, where I'm scanning through a proxy
> handler to see what it does.

This should be addressed by handler class overview comments giving the lay of the land for what the handler does.  We should add more of those.

> If this gets added to the spec as a proxy trap, _and_
> the spec defines an ordering of these traps, _and_ we decide to reorder our
> traps to match the spec (because they currently don't), I'm happy to realign
> them.

Traps aren't ordered in any sense I can think of.  The table of contents order is likely historical holdover more than anything.

> XrayWrappers are an abstraction that we are committed to providing and
> getting 100% right.

By design, not always.  All things considered, [[Prototype]] mutation on Xrays remains extremely uncommon (property visibility on prototypes is an entirely separate concern irrelevant to this issue), and I continue to think it's not a worthwhile use of engineering time to support it.
Keywords: leave-open
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 1048535
I don't understand what I"m being flagged for.
Flags: needinfo?(bobbyholley)
I'd like to try eagerly initializing Object and Function on globals, to eliminate issues that arise if lazy standard classes are used, but the global object's prototype is made immutable at creation time.  Just adding an ensureConstructor to JS_NewGlobalObject fails because that method is also used for the SHG, and if the basic self-hosted code isn't run in a particular way by initSelfHosting, things fall over.

This prerequisite patch refactors global creation code such that the SHG is created *not* using JS_NewGlobalObject.  The SHG is unique.  It can share code with the normal global-creation path when it makes sense, but it should be separate in order to deal with the SHG's unique requirements.  This patch shouldn't change any behavior; it just makes it easier to adjust *only* the non-SHG global creation path in a later patch.
Attachment #8529165 - Flags: review?(till)
Comment on attachment 8529165 [details] [diff] [review]
Refactor global-object creation code to distinguish the unique self-hosting global from all other run-of-the-mill globals

Review of attachment 8529165 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

::: js/src/vm/SelfHosting.cpp
@@ +1223,5 @@
>  #endif
>  }
>  
> +GlobalObject *
> +JSRuntime::newSelfHostingGlobal(JSContext *cx)

I'd call this createSelfHostingGlobal, as it will only be called once, and new... sounds, to my ears, like something that can happen multiple times. Also, does this need to be o JSRuntime instead of being a static function in SelfHosting.cpp?
Attachment #8529165 - Flags: review?(till) → review+
Comment on attachment 8529165 [details] [diff] [review]
Refactor global-object creation code to distinguish the unique self-hosting global from all other run-of-the-mill globals

Review of attachment 8529165 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/SelfHosting.cpp
@@ +1223,5 @@
>  #endif
>  }
>  
> +GlobalObject *
> +JSRuntime::newSelfHostingGlobal(JSContext *cx)

createSHG works for me.

The method has to be on JSRuntime because of this line:

  cx->runtime()->selfHostingGlobal_ = shg;

(Originally I had it as a GlobalObject method, but I had to move it to be able to perform this assignment, which has to happen there so that initSelfHostingBuiltins works correctly.  We could also add friendships, but I'd rather not expose an entire class to the other class here.  I also briefly considered passing in a GlobalObject** for this, but storing into a random pointer outparam at a just-so point seemed far, far worse than even friendship.)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #23)
> The method has to be on JSRuntime because of this line:
> 
>   cx->runtime()->selfHostingGlobal_ = shg;

Right, obviously. And I agree: adding friendships just for this doesn't make sense, and the other thing, well, just no.
https://hg.mozilla.org/integration/mozilla-inbound/rev/72c672d91f9c for the SHG creation stuff.  Still more work to do here.
Backed out this and the bug 1105261 patch in https://hg.mozilla.org/integration/mozilla-inbound/rev/7b8eb088c380 because one of them was making b2g desktop crash like https://treeherder.mozilla.org/logviewer.html#?job_id=4324215&repo=mozilla-inbound in linux64 mochitest-oop nearly all the time, and in both linux64 and mac gaia-ui-test-functional-1 and -3 quite often.

Pushed just this one to try for you in https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3ba384952a02
So apparently that's a permafail on Tinderbox.  Bleh.  I can't reproduce this locally building/running b2g-desktop with -oop applied.

jonco, you have any sense of what do to do approach this one last weirdness, short of splitting the patch into micro-steps and then applying them one after another until the busting thing is found?
Flags: needinfo?(jcoppeard)
(I should note there's been some bitrotting along the way here, which I have fixed locally.  I can upload the fixed patches if needed.)
I had a another look at this, and it seems you removed AutoCompartmentRooter.  This is necessary to prevent the compartment being collected before it has been fully initialised.

I did a try run here and those tests are passing now: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c5dffc3dfa49
Flags: needinfo?(jcoppeard)
Attachment #8535660 - Flags: review?(jwalden+bmo)
Comment on attachment 8535660 [details] [diff] [review]
bug1052139-auto-compartment-rooter

Review of attachment 8535660 [details] [diff] [review]:
-----------------------------------------------------------------

I guess I'll package this up into the original patch and land it all as one on Monday, or earlier if I'm feeling motivated.  Thanks for catching my stupid mistake!  :-(  Wonder why it didn't reproduce locally, or with lots of extra GC or anything.

::: js/src/vm/GlobalObject.cpp
@@ +274,5 @@
>          zone = nullptr;
>      else
>          zone = static_cast<Zone *>(options.zonePointer());
>  
> +    AutoCompartmentRooter compartment(cx, NewCompartment(cx, zone, principals, options));

So before it was just a raw pointer.  But!  On line 290 below, or line 1293 in SelfHosting.cpp, before any operation that could GC, I had |AutoCompartment ac(cx, compartment);|.  Which I would have thought would effectively root the compartment.  Is it really *not* the case that entering a compartment also roots it, via some doubly-linked list chain just like any sort of scoped stack thing would do by any sane assumption?  That's just messed up.  :-(
Attachment #8535660 - Flags: review?(jwalden+bmo) → review+
That's a good idea, and also seems to fix the problem.

Terrence, do you remember if there's any reason we don't do this?
Attachment #8536602 - Flags: review?(terrence)
Comment on attachment 8536602 [details] [diff] [review]
bug1052139-mark-entered-compartments

Review of attachment 8536602 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think so? The only thing I could see going wrong is if we're in a compartment when we destroy the last context, we'll probably iloop in gcCycle since repeatForDeadZone will always be set for the cleanUpEverything GC. If it passes try, I'd be for it.
Attachment #8536602 - Flags: review?(terrence) → review+
Attachment #8535660 - Attachment is obsolete: true
Looking back at this again now, running into an issue I need to figure out.

The problem is that lazy standard classes play a little funny with this.  We have to have resolved, at a minimum, the Object class, in order that Object.prototype be already inserted into the global object prototype chain that we're making immutable.  My current patchwork had ensured this by doing a GlobalObject::ensureConstructor(cx, global, JSProto_Object) in GlobalObject::new_.  But that happens to be too early for XPConnect Sandbox.cpp code, which does a post-hoc mutation of that particular global object's prototype to a different value -- and moreover, also wants to invoke a sandbox_addProperty hook that depends upon the sandbox global's private value having been set already.  So we're too early in that one weird case.

Whether I deal with this by making every global-object-creator say it wants an immutable prototype chain (and have the sandbox code not do so), or something else, is as yet unclear.  Still thinking.
After some more futzing, I *think* I have something that works for sandboxes.  I think.

Turns out if you reorder a few things, and you use a frob to suppress sandbox_addProperty behavior that I don't remember existing the last time I looked at this, you can get behavior that doesn't trigger the bad cases -- if I'm reading right.

Tryserver seems to think I'm reading right, pretty much -- with a few oranges that look closer to permanent than intermittent.  I'm not sure whether the issues I'm hitting are from this patch, or from a patch underneath it.  Probably the latter, but I can't be sure.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a8c0d027437
Depends on: 1206300
In *theory* this makes it a one-liner to fix the problem of someone shoving a proxy into their global object's prototype chain -- or any other web-exposed context that uses a global object (did I get all of them?  think so, not certain) -- and using that to intercept any name lookup for which a property doesn't exist on the global object.  That one-liner is:

-static const bool ImmutablePrototypesEnabled = false;
+static const bool ImmutablePrototypesEnabled = true;

As we don't know whether people mutate these prototypes or not, in sufficient quantities that this wouldn't be immediately web-shippable, this seems a good state to start with.  I'll post a separate patch to actually flip the flag, and land them separately.  But now seems a great time to try it, at the start of a cycle.
Attachment #8663938 - Flags: review?(bzbarsky)
Attachment #8660388 - Attachment is obsolete: true
Comment on attachment 8663938 [details] [diff] [review]
Add all the code to make the global object's [[Prototype]] chain immutable, able to be turned on with a single-line change

Do you want me to review just the dom bits, or all of this?
Flags: needinfo?(jwalden+bmo)
More or less all of it, I think, because the non-DOM bits of this are pretty negligible.  It's:

* a bunch of test-fixups that don't really need examination;
* js/src/builtin/Object.cpp and js.cpp, whose changes are identical to those in several of the DOM files;
*  jsapi.{cpp,h} that just adds a trivial JSAPI function; and
* jsobj.cpp whose change *does* have some interaction with lazy-standard-classes of the sort the browser embedding uses.

Oh, and also Sandbox.cpp changes -- those are the fugliest/trickiest changes in the patch, that *do* deserve particular attention.  If there's any part of this that I do really care about, it's this part.  (Of course say something if someone else should look at this part.)
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8663939 [details] [diff] [review]
Patch to flip the immutable-prototype enforcement bit, enabling such enforcement

r=duh
Attachment #8663939 - Flags: review?(bzbarsky) → review+
Comment on attachment 8663938 [details] [diff] [review]
Add all the code to make the global object's [[Prototype]] chain immutable, able to be turned on with a single-line change

>+++ b/dom/base/WindowNamedPropertiesHandler.cpp
>+  gsp = js::NewProxyObject(aCx, WindowNamedPropertiesHandler::getInstance(),
>                             JS::NullHandleValue, aProto,
>                             options);

Please fix the indentation.

>+++ b/dom/bindings/Codegen.py
>+        if self.descriptor.interface.getExtendedAttribute("ProtoHasImmutablePrototype"):

Why do you need this extended attribute?  Please use self.descriptor.interface.isOnGlobalProtoChain().  If there's a reason that doesn't do the right thing, I'd love to know what that reason is.  Of course you'll need to check needInterfacePrototypeObject too.

>+                JS::Heap<JSObject*>* proto = &aProtoAndIfaceCache.EntrySlotOrCreate(prototypes::id::${name});

Just use protoCache.

Actually, while we're here, there's a preexisting bug that needs to get fixed or at least filed.  The defineAliases calls this code reentrantly; it should probably null-check *protoCache first

>+                  JS::Rooted<JSObject*> prot(aCx, *proto);

Once you've null-checked *protoCache, you can just pass GetProtoObjectHandle to JS_SetImmutablePrototype, right?

>+                    *proto = nullptr;

No, that's not good enough.  You want to do something more like the failureCode bit we have in the hasUnforgeableMembers case.  In fact, maybe just common it out; it's only applicable when we know we have an inteface prototype object, so naming it might be a bit hard if it does get hoisted higher....

>+++ b/dom/bindings/parser/WebIDL.py
>+                  identifier == "ProtoHasImmutablePrototype"):

Like I said above, you shouldn't need this.  Or any of the .webidl file changes.

>+++ b/js/src/jit-test/tests/TypedObject/fuzz3.js
>-this.__proto__ =  Proxy.create({});

This rather changes what the test is testing, no?  In fact, how is this a useful test after this change?

Can you put that inside a try/catch at least?

>+++ b/js/src/jit-test/tests/auto-regress/bug568855.js
>-this.__proto__ = Proxy.create({

Likewise.

>--- a/js/src/jit-test/tests/auto-regress/bug589103.js
>+++ /dev/null

And this?  Or somehow condition it on our boolean?  I have doubts about this being web-compatible, and would rather not lose test coverage if we end up flipping the boolean off.

>--- a/js/src/jit-test/tests/auto-regress/bug637205.js
>+++ /dev/null

And here...

>+++ b/js/src/jit-test/tests/auto-regress/bug652177.js
>-y.__proto__ = Function.toLocaleString
>+Object.defineProperty(y, "__proto__", { value: 17, writable: true });

Why is this the right change?

>--- a/js/src/jit-test/tests/auto-regress/bug672104.js
>+++ /dev/null

Again, would rather not lose test coverage for the mode we're actually shipping.  This applies to all the tests, whether deleted or simply nerfed to no longer test anything useful; I'll stop saying it.  I'd like to see the patch again once the tests have been fixed to continue testing useful things...

>+++ b/js/xpconnect/src/Sandbox.cpp
>@@ -1048,16 +1048,31 @@ xpc::CreateSandboxObject(JSContext* cx, 
>+        {
>+            // Don't try to mirror standard class properties.

Shouldn't this whole block only happen if options.writeToGlobalPrototype?
Attachment #8663938 - Flags: review?(bzbarsky) → review-
I'm not at all convinced these tests do anything useful at all, so the "rightness" of any particular change as far as preserving the test's behavior is extraordinarily dubious, to be pretty blunt about it.  Especially not the scumbag auto-regress/ tests that are literal scattershot with no sense at all to them, because people couldn't do their job and include a properly reduced, readable testcase in their fixes, and yes I am being inordinately bitter and cynical right now stab.

Basically I tried to vaguely preserve approximate semantics, somehow, and left it at that.  It's not worth your time or mine to deeply delve into the semantics under test in any of these tests, particularly not when it might be something like bareword semantics where the test becomes literally unwritable with an immutable global object prototype.
Attachment #8666959 - Flags: review?(bzbarsky)
Why this was ordered as it was before, I dunno.

I could also imagine just doing this as a series of string-appends -- pretty sure I've seen that elsewhere, more sensibly -- but this preserves the existing structure a bit.

This is just moving big blocks of code around unchanged, *except* that I combined the adjacent cases that handle creating the unforgeable holder, then setting it.  Seems no point in separation if they're part of a single logical step.

More refactoring to follow.  Also willing to do more if necessary, atop these patches, tho I'd like to pocket incremental gains and not let any one patch snowball too hard.
Attachment #8667043 - Flags: review?(bzbarsky)
I think this was the failure-handling thing you wanted done, right?

I'm not convinced there's an issue with the defineAliasing code.  Copying from FontFaceSetBinding.cpp after this patch,

  // Set up aliases on the interface prototype object we just created.
  JS::Handle<JSObject*> proto = GetProtoObjectHandle(aCx, aGlobal);
  if (!proto) {
    *protoCache = nullptr;
    if (interfaceCache) {
      *interfaceCache = nullptr;
    }
    return;
  }

  JS::Rooted<JS::Value> aliasedVal(aCx);

  if (!JS_GetProperty(aCx, proto, "values", &aliasedVal)) {
    *protoCache = nullptr;
    if (interfaceCache) {
      *interfaceCache = nullptr;
    }
    return;
  }
  JS::Rooted<jsid> iteratorId(aCx, SYMBOL_TO_JSID(JS::GetWellKnownSymbol(aCx, JS::SymbolCode::iterator)));
  if (!JS_DefinePropertyById(aCx, proto, iteratorId, aliasedVal, JSPROP_ENUMERATE)) {
    *protoCache = nullptr;
    if (interfaceCache) {
      *interfaceCache = nullptr;
    }
    return;
  }
  if (!JS_DefineProperty(aCx, proto, "keys", aliasedVal, JSPROP_ENUMERATE)) {
    *protoCache = nullptr;
    if (interfaceCache) {
      *interfaceCache = nullptr;
    }
    return;
  }

If GPOH returns nullptr, we failure-handle it.  If one of the alias bits fails, we return without trying any more.  And if it succeeds, it's not going to clear the iface cache, right?  So it seems like there's no defineAlias issue at all.

But if there is, please explain harder and I'll kill it off here.
Attachment #8667044 - Flags: review?(bzbarsky)
I think this covers the rest of the bases in comment 42.

"Why do you need this extended attribute?"  Because I didn't know it existed; looks like it'll work.

"Just use protoCache."  Done, I think, tho I think there's something very screwball about having a falsy-check guard a method call that happens to spookily depend on the null-check.  Pfui.

"Once you've null-checked *protoCache, you can just pass GetProtoObjectHandle"  I think I did what you want here...

"Shouldn't this whole block only happen if options.writeToGlobalPrototype?"  This is a little screwy.  The dodgy code that wants a private set, is in sandbox_addProperty.  That hook is only associated with the aptly-named SandboxWriteToProtoClass.  That class is present as the global's class only if options.writeToGlobalPrototype.  So the rationale of avoiding the mirroring behavior that motivated enumeration in the old code, isn't present.  BUT.  To assure an immutable prototype chain for this global, we need Object.prototype initialized -- in *all* cases.  That's whether we have sandbox_addProperty or not.  Enumerating here, ensures that's been created for everything, and it makes both sorts of sandbox more consistent.  Then when we do a JS_SplicePrototype below, we can safely know we'll have the proper prototype chain created.  So really, the comments by that code could be better/clearer/more accurate.  Changes made.
Attachment #8667060 - Flags: review?(bzbarsky)
Attachment #8663938 - Attachment is obsolete: true
> If GPOH returns nullptr, we failure-handle it.

My point was that if we tried to create the object above and failed (which doesn't immediately bail out afaict), we may get down here, then call GetProtoObjectHandle, which will try to create the object again.  If it now succeeds, and sets up aliases, and we then unwind back here and start setting up aliases again... it might work or it might not depending on whether anything here is non-configurable.  And in general that reentrancy is not ok, imo.

Will look at the rest of this stuff when I'm more awake.
(In reply to Boris Zbarsky [:bz] from comment #47)
> if we tried to create the object above and failed (which
> doesn't immediately bail out afaict), we may get down here, then call
> GetProtoObjectHandle, which will try to create the object again.  If it now
> succeeds, and sets up aliases, and we then unwind back here and start
> setting up aliases again... it might work or it might not depending on
> whether anything here is non-configurable.

Ugh.  Tell me it's fine to convert this stuff to a sensible system of failure-bool-returning with outparams, and not a crazy Handle-returning function, and I'll do it in a heartbeat.  Clearly this current setup is unusable.

I might fix the defineAliases GPOH issue, but it's going to be atop the patches here, I think.  Need to pocket this bug's gains before getting into this voodoo -- or do it as part of the bigger refactor in the last graf.
I'd have to think pretty carefully and look at the generated assembly to see whether it's fine.  That stuff is pretty hot.

> I might fix the defineAliases GPOH issue

Just getting it on file is good enough for now.
(In reply to Boris Zbarsky [:bz] from comment #49)
> I'd have to think pretty carefully and look at the generated assembly to see
> whether it's fine.  That stuff is pretty hot.

How so?  Each CreateInterfaceObjects method is called at most once per interface per global, not repeatedly.  JS::Heap* is binary-identical to JS::Rooted in terms of ABI to use/pass it.  Tacking onto/off of the root stack should have only token startup/teardown cost, and even that must be outweighed by the repeated cost of Get{Constructor,Proto}ObjectHandle, and of rechecking *protoCache or similar when necessary to validate something didn't clear the caches.  What am I missing?
The hot thing is GetProtoObjectHandle, which happens on every object creation, not just in CreateInterfaceObjects.
So if I look at a generated GPOH, I see:

  if (!protoAndIfaceCache.EntrySlotIfExists(prototypes::id::Window)) {
    CreateInterfaceObjects(aCx, aGlobal, protoAndIfaceCache, aDefineOnGlobal);
  }

Looks to me like we could add Rooteds *only* if the slot doesn't exist yet, i.e. within that block alone, and the common case would be fast.  We could even add a MOZ_UNLIKELY there to tell the compiler it's extra-unlikely.  That'd solve the GPOH problem, wouldn't it?
That might be doable yes.  I'm trying to remember why CreateInterfaceObjects was written the way it wsa now...  Anyway, definitely out of the scope of this bug.
Comment on attachment 8666959 [details] [diff] [review]
Adjust tests to tolerate an immutable global object prototype chain

Thank you.  I know this was a pain, but it does make me much happier!  r=me
Attachment #8666959 - Flags: review?(bzbarsky) → review+
Comment on attachment 8667043 [details] [diff] [review]
Reorder CGCreateInterfaceObjectsMethod a bit to correspond to the ordering of generated code

> Why this was ordered as it was before, I dunno.

Partially probably accretion.  Partially I think some of the generated code used to be in a slightly different order.   E.g. the unforgeable stuff used to live on an object that was set up in a somewhat different way.

r=me
Attachment #8667043 - Flags: review?(bzbarsky) → review+
Comment on attachment 8667044 [details] [diff] [review]
Perform proper failure handling in interfaces' CreateInterfaceObjects method, after dom::CreateInterfaceObjects has been called and possibly created the interface's constructor and prototype

>+        # From here onward, failure cases must be handled with this code.


In cases when needInterfacePrototypeObject, no?  If all the cases after this point in fact have that true, worth somehow noting that dependency anyway.

r=me
Attachment #8667044 - Flags: review?(bzbarsky) → review+
Comment on attachment 8667060 [details] [diff] [review]
The leftover bits of the old patch, adjusted atop the previous changes

>+        if self.descriptor.interface.isOnGlobalProtoChain() and \
>+           needInterfacePrototypeObject:

I think local style is:

        if (self.descriptor.interface.isOnGlobalProtoChain() and
            needInterfacePrototypeObject):


r=me.  Thank you for the better Sandbox comments!
Attachment #8667060 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #57)
> I think local style is:
> 
>         if (self.descriptor.interface.isOnGlobalProtoChain() and
>             needInterfacePrototypeObject):

Unfortunate, because then you have

  if (... and
      ...):
      ...

and things blend together.  Pfui.

Landed all the patches but the enabling bit -- will land that after a delay, and probably a merge to m-c or at least no backout.  Pocket the gains...
Backed out 3dc503961322 and 6f278bc63614 in https://hg.mozilla.org/integration/mozilla-inbound/rev/891371c4f1fe for mochitest-jetpack bustage.
Those were the last bits of the implementing patches, which were just the reviewed patches here, subdivided into bite-sized-landable pieces.  \o/

Turns out all the mess was because of...some sort of thing...where non-insane Sandboxes didn't like having their standard classes enumerated.  I didn't fully debug it, and I couldn't get solid reproduction locally.  But fortunately, instantiating *just* Object (all we need) doesn't break stuff, so I slotted that in the relevant place.

And now, hopefully the final try-run before I flip the switch (after the last bits are merged to m-c):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fbf1a526129
One more try-run for a JS test needing fixing, producing a tree that looks maybe okay, amid a sea of intermittent orange:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18a0bc121bea

And with that, flipped the switch.  Time to see what happens.  :-)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/bfe8257bb187
https://hg.mozilla.org/mozilla-central/rev/62956dbc1bb8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
For standardizing this, I put something on the agenda for the TC39 January meeting, and wrote up a strawman spec pull request at https://github.com/tc39/ecma262/pull/308 . I'd be interested to hear what you think. Let me know if you want to delay discussion for a later meeting. My aim isn't necessarily to get something fully standardized, but just to get general consensus around restrictions like this so that we (Chrome) don't ship Proxy for use cases that later get locked down and banned by other forms of security restrictions.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: