822383 - Allow arbitrary proxies on scope chains

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

We currently throw JSMSG_NON_NATIVE_SCOPE when this happens, but luke says the following in bug 821850:

> So JSMSG_NON_NATIVE_SCOPE is an assertion-turned-dynamic-check (in bug
> 609990).  The assertion itself is pretty old.  Now, I know for a fact that we > can have proxies on the scope chain: DebugScopeProxy.  It looks like the
> assert comes from bug 566549 which had problems with non-native scope objects
> in JSOP_DEFVAR.  But, surprise surprise, I had to generalize JSOP_DEFVAR for
> DebugScopeProxy so I think you should just loosen up this assertion:
>  http://hg.mozilla.org/mozilla-central/diff/f45eec2bd4c7/js/src/jsinterpinlines.h#l1.13
> to accept any isProxy(), not just isDebugScope() proxies and then remove the
> JSMSG_NON_NATIVE_SCOPE error (and the following !defineProperty assertion)
> entirely (from js.msg).  Can you replace it with an assertion that every
> object (on the obj->enclosingScope() chain) is in the right compartment and
> that the chain terminates with an isGlobal?).  You might want to try some of
> the flash examples that were hitting this (listed in bug 609990) and then try > to land the change by itself (I can review) in case there are other issues.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Allow arbitrary proxies on the scope chain. v1

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 694110 [details] [diff] [review]
Allow arbitrary proxies on the scope chain. v1

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

::: js/src/jsinterp.cpp
@@ +551,5 @@
>      scopeChain = GetInnerObject(cx, scopeChain);
>      if (!scopeChain)
>          return false;
>  
> +    /* Make sure the scope chain is sane. */

How about a little more specific:
  "Ensure the scope chain is all same-compartment and terminates at a global."

@@ +553,5 @@
>          return false;
>  
> +    /* Make sure the scope chain is sane. */
> +#ifdef DEBUG
> +    RawObject debugScope = scopeChain;

"debugScope" has a special meaning (grep in vm/ScopeObject.cpp).  How about 's'?

::: js/src/jsinterpinlines.h
@@ +475,5 @@
>  inline bool
>  DefVarOrConstOperation(JSContext *cx, HandleObject varobj, HandlePropertyName dn, unsigned attrs)
>  {
>      JS_ASSERT(varobj->isVarObj());
> +    JS_ASSERT(!varobj->getOps()->defineProperty || varobj->isProxy());

On second thought, given that the only objects that don't match this predicate are dense and typed arrays (and that those would probably work fine anyhow), how about just remove the assert?

Comment 3

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24a640bec501

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/24a640bec501