Right now, we've got this wacky situation where we depend on virtual proxy traps to call enter(). This causes various problems: 1 - Every derived proxy needs to remember to call enter() appropriately for every trap. And sometimes they forget. 2 - If any proxy wants to bounce to the behavior if its base class by calling Base::foo in ::foo, it's very hard to avoid calling enter() twice. This has frustrated me for a while, and I've just discovered that we do a very bad job with all of this stuff in XrayWrapper. This is currently blocking some of Waldo's work, so I want to improve the situation. In order to get this right, we basically need a second layer, outside the hierarchy of virtual traps. Thankfully, we already have this layer, via the Proxy::foo JSClass hooks. So my plan is to hoist the responsibility of calling enter() into the Proxy:: hooks. This means moving enter() from Wrapper into BaseProxyHandler, and would naively incur an extra unnecessary virtual function call for certain non-wrapper proxies. To avoid this, I'm going to put a static bit on the proxy handler called "mHasPolicy". This will allow us to only call enter() in cases where we have a nontrivial enter() trap, which will actually be a performance boost for transparent wrappers.

Sounds good. (Btw, I just saw again and remembered how goofy it is that 'Proxy' is a class. It's be rather less confusing for newcomers if it was a namespace.)

One compile failure and one test failure, both of which I fixed. Uploading patches and flagging blake for review.

Luke explained to me that it should never get there.

This is just a heuristic, anyway, and some of the usage is downright broken. There are two cases here: 1 - Deciding what to do for get{Own,}PropertyDescriptor. In these cases, we can just enter with GET and rely on the filtering machinery to filter out dangerous setters for security wrappers. 2 - Custom Xray props. None of these make sense in a |set| context. In fact, they generally have null setters anyway, so we can just assume GET. The policy-entering code in XrayWrapper is super haphazard. We'll get rid of it entirely later in these patches.

This is kind of nonsensical, because CALL means "the wrapped object is being called", whereas nativeCall means "the wrapped object is being unwrapped to have a JSNative invoked on it", which are two very different things. We _could_ add a NATIVECALL enter() trap, but our current policy enforcement around nativeCall involves overriding the trap itself, so we wouldn't use it for anything. So let's just get rid of it.

This will allow us to skip the virtual function call for non-security-wrapper proxies, which are the cases where we care most about performance.

This will allow us to make some hard assertions that a given policy has been entered exactly once.

Comment on attachment 710598 [details] [diff] [review] Part 2 - Stop using JSRESOLVE_ASSIGNING to determine GET vs SET. v1 Review of attachment 710598 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/wrappers/XrayWrapper.cpp @@ +1521,5 @@ > desc->attrs); > } > > + // NB: We still need JSRESOLVE_ASSIGNING here for the time being, because it > + // tells things like nodelists whether they should create the property or not. The way I've removed every other JSRESOLVE_* has been to remove every test of it. Once every test for presence/absence in the flags bitfield is gone, the uses are all obviously pointless. In the meantime, tho, I assume every use is meaningful. And really it's not the uses that are painful (except in the sense of knowing when a flag should be used versus not), only the eventual consumption of those uses in tests. If you really want the comment go for it -- just saying that this comment doesn't make it easier to remove the flag, in my view, because the tests are what really matter.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14) > If you really want the comment go for it -- just saying that this comment > doesn't make it easier to remove the flag, in my view, because the tests are > what really matter. Well, the point of the comment was that I tried to remove it, because most of the other uses in this file were haphazard security checks, but that this one caused orange, because of the nodelist issue. I thought that was worth annotating.

Comment on attachment 710599 [details] [diff] [review] Part 3 - Add Special handling to allow us to call enter() for defineProperty on Xrays. v1 Review of attachment 710599 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/wrappers/FilteringWrapper.cpp @@ +135,5 @@ > + // scriptable helper, and pass the wrapper itself as the object upon which > + // the resolve is happening. Then, special handling happens in > + // XrayWrapper::defineProperty to detect the resolve and redefine the > + // property on the holder. Really, we should just pass the holder itself to > + // NewResolve, but there's too much code in nsDOMClassInfo that assumes this This isn't quite true, passing the holder to these functions doesn't work because they also need to look up properties on the passed-in object, and you can't do that directly through the holder. ::: js/xpconnect/wrappers/XrayWrapper.h @@ +43,1 @@ > } Nit: blank line above the closing brace.

Comment on attachment 710604 [details] [diff] [review] Part 7 - Introduce an RAII class for entering policies. v1 Review of attachment 710604 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/js.msg @@ +388,5 @@ > MSG_DEF(JSMSG_UNDEFINED_CURRENCY, 335, 0, JSEXN_TYPEERR, "undefined currency in NumberFormat() with currency style") > MSG_DEF(JSMSG_INVALID_TIME_ZONE, 336, 1, JSEXN_RANGEERR, "invalid time zone in DateTimeFormat(): {0}") > MSG_DEF(JSMSG_DATE_NOT_FINITE, 337, 0, JSEXN_RANGEERR, "date value is not finite in DateTimeFormat.format()") > +MSG_DEF(JSMSG_OBJECT_ACCESS_DENIED, 338, 0, JSEXN_ERR, "Permission denied to access object") > +MSG_DEF(JSMSG_PROPERTY_ACCESS_DENIED, 339, 1, JSEXN_ERR, "Permission denied to access property '{0}'") Rather than adding to the end here, please use some the UNUSED messages found earlier in this file. ::: js/src/jsproxy.h @@ +359,5 @@ > + } > + > + inline bool allowed() { return allow; } > + inline bool returnValue() { JS_ASSERT(!allowed()); return rv; } > +protected: Uber-nit: newline before "protected:".

Comment on attachment 710604 [details] [diff] [review] Part 7 - Introduce an RAII class for entering policies. v1 Review of attachment 710604 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsproxy.h @@ +346,5 @@ > JSObject * > RenewProxyObject(JSContext *cx, JSObject *obj, BaseProxyHandler *handler, Value priv); > > +class AutoEnterPolicy { > +public: class AutoEnterPolicy { public: Access labels are indented two spaces ("half-indented") in SpiderMonkey code, so change the protected below here too.

Comment on attachment 710605 [details] [diff] [review] Part 8 - Hoist enter() calls from {Xray,}Wrapper::foo into Proxy::foo. v1 Review of attachment 710605 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsproxy.cpp @@ +2225,5 @@ > +// policy to live on the stack until the end of the call. > +#define CHECK(cx, proxy, id, handler, act) \ > + AutoEnterPolicy policy(cx, handler, proxy, id, act, true); \ > + if (!policy.allowed()) \ > + return policy.returnValue(); \ For three lines I don't know if this warrants a macro. In particular, macros that declare variables visible outside of the macros are particularly evil. I'd prefer to nuke it.

I had to do some nontrivial rebasing, so I'm pushing to try again just to be safe: https://tbpl.mozilla.org/?tree=Try&rev=e1a0762bcae9

There was some opt-only build bustage, so I cancelled the rest of the run and pushed again: https://tbpl.mozilla.org/?tree=Try&rev=8fb9718046e8

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e0632e639097 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/81db941b0b76 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/741efe957058 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/804898e09f6e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/240174280d52 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b5992ec030e3 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fa2d0bb5e6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab07392f2424 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d301b2bcad0

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dee88fe417fe for Windows build bustage

sigh, still fighting with MSVC warnings-as-errors: https://tbpl.mozilla.org/?tree=Try&rev=1ae453da72f9

Finally green. Repushing: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e2cd7f968aaf remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/520ff1852daf remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b16b15352782 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f976eaa7bbf4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f353efb7e15a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1638e21ef27d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d7322c063fc1 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf8b5dab36c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/51483e470216

https://hg.mozilla.org/mozilla-central/rev/e2cd7f968aaf https://hg.mozilla.org/mozilla-central/rev/520ff1852daf https://hg.mozilla.org/mozilla-central/rev/b16b15352782 https://hg.mozilla.org/mozilla-central/rev/f976eaa7bbf4 https://hg.mozilla.org/mozilla-central/rev/f353efb7e15a https://hg.mozilla.org/mozilla-central/rev/1638e21ef27d https://hg.mozilla.org/mozilla-central/rev/d7322c063fc1 https://hg.mozilla.org/mozilla-central/rev/fdf8b5dab36c https://hg.mozilla.org/mozilla-central/rev/51483e470216