Allow arbitrary proxies on scope chains

RESOLVED FIXED in mozilla20

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 694110 [details] [diff] [review]
Allow arbitrary proxies on the scope chain. v1
Attachment #694110 - Flags: review?(luke)

Comment 2

5 years ago
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?
Attachment #694110 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/24a640bec501
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.