836301 - Move enter() calls into Proxy::foo JSClass hooks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Luke Wagner [:luke]]

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.)

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=450250386dba

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=2041796c5386

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Assert against JSID_VOID in JSID wrapping. v1

Luke explained to me that it should never get there.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Stop using JSRESOLVE_ASSIGNING to determine GET vs SET. v1

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.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add Special handling to allow us to call enter() for defineProperty on Xrays. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 4 - Stop enter()ing with CALL for nativeCall. v1

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.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 5 - Add tracking for whether we have a non-trivial enter() trap. v1

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.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 6 - Hoist enter() into BaseProxyHandler. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 7 - Introduce an RAII class for entering policies. v1

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

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 8 - Hoist enter() calls from {Xray,}Wrapper::foo into Proxy::foo. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 9 - Hoist some assertions, remove a bunch of no-op trap overrides, and add assertions that we've entered our policy. v1

Comment 14

[Jeff Walden [:Waldo]]

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.

Comment 15

[Bobby Holley (:bholley)]

(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 16

[Blake Kaplan (:mrbkap) (inactive)]

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 17

[Blake Kaplan (:mrbkap) (inactive)]

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 18

[Jeff Walden [:Waldo]]

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 19

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 20

[Bobby Holley (:bholley)]

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

Comment 21

[Bobby Holley (:bholley)]

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

Comment 22

[Bobby Holley (:bholley)]

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

Comment 23

[Phil Ringnalda (:philor)]

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

Comment 24

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=541ee68ce65e

Comment 25

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=6b5ee8341073

Comment 26

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=fe796fe581d4

Comment 27

[Bobby Holley (:bholley)]

sigh, still fighting with MSVC warnings-as-errors:

https://tbpl.mozilla.org/?tree=Try&rev=1ae453da72f9

Comment 28

[Bobby Holley (:bholley)]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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