1141905 - [jsdbg2] DebuggerFrame_evalWithBindings creates random objects with non-global parents

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Specifically, this bit:

6163     if (evalWithBindings) {
6164         /* TODO - This should probably be a Call object, like ES5 strict eval. */
6165         RootedPlainObject nenv(cx, NewObjectWithGivenProto<PlainObject>(cx, NullPtr(), env));

This object then ends up on function scope chains, etc.

You could probably use a With object here, the way the DOM does for the scope chains it tries to build up... or yes, a Call object.

This is one of the few remaining things that are depending on object parents, as far as I can see.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make it possible to CreateScopeObjectsForScopeChain with a given non-global scope chain terminator

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make the 5-arg version of CreateScopeObjectsForScopeChain non-static so it can be called from multiple files, and move it to a possibly-saner location

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Use CreateScopeObjectsForScopeChain in DebuggerFrame_evalWithBindings

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Add some assertions about what enclosingScope can return for non-scope objects

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8576386 [details] [diff] [review]
part 4.  Add some assertions about what enclosingScope can return for non-scope objects

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

::: js/src/vm/ScopeObject.cpp
@@ +2725,5 @@
> +JSObject *
> +JSObject::enclosingScopeSlow()
> +{
> +    MOZ_ASSERT_IF(is<JSFunction>(), as<JSFunction>().isInterpreted());
> +    MOZ_ASSERT(getParent() == &global());

assertParentIs maybe?  Not that it matters.

::: js/src/vm/ScopeObject.h
@@ +1042,5 @@
>  
> +    if (is<js::GlobalObject>())
> +        return nullptr;
> +
> +    return enclosingScopeSlow();

Rather than this, I think you can just move this method definition into ScopeObject-inl.h, then add #includes of that file where needed.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8576380 [details] [diff] [review]
part 1.  Make it possible to CreateScopeObjectsForScopeChain with a given non-global scope chain terminator

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

::: js/src/jsapi.cpp
@@ +3195,5 @@
>  }
>  
>  static bool
>  CreateScopeObjectsForScopeChain(JSContext *cx, AutoObjectVector &scopeChain,
> +                                HandleObject enclosingScope,

nit: dynamicTerminatingScope? I'll leave the name up to you, but I'd like "dynamic" in there to avoid confusion.

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8576383 [details] [diff] [review]
part 3.  Use CreateScopeObjectsForScopeChain in DebuggerFrame_evalWithBindings

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

LGTM. r=me with comments addressed.

::: js/src/vm/Debugger.cpp
@@ +763,5 @@
>       * from GetDebugScopeFor(Frame|Function).
>       */
> +    MOZ_ASSERT(!env->is<ScopeObject>() ||
> +               (env->is<DynamicWithObject>() &&
> +                !env->as<DynamicWithObject>().isSyntactic()));

This condition is used in various places now (here and below in this patch, in AssertDynamicScopeMatchesStaticScope, and in ExecuteKernel off the top of my head). Do you mind refactoring it to something like IsValidTerminatingScope?

@@ +6024,5 @@
>       * ScopeIter should stop at any non-ScopeObject boundaries, and we are
>       * putting a DebugScopeProxy on the scope chain.
> +     *
> +     * XXXbz But maybe in the case when env is a DynamicWithObject we should in
> +     * fact pass the corresponding StaticWithObject for the static scope?

No, we shouldn't make a StaticWithObject. With your changes the notion of ScopeObject chains terminated by non-ScopeObject really means ScopeObject chains terminated by non-ScopeObject or non-syntactic Withs. Could you update the comment accordingly?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> nit: dynamicTerminatingScope?

Done.

> Do you mind refactoring it to something like IsValidTerminatingScope?

No, was considering that myself.  Done.

> Could you update the comment accordingly?

Done.

> assertParentIs maybe?

Yes, done.

> I think you can just move this method definition into ScopeObject-inl.h

Done.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b09f6900fe8
https://hg.mozilla.org/integration/mozilla-inbound/rev/455577de172e
https://hg.mozilla.org/integration/mozilla-inbound/rev/16723f5b0307
https://hg.mozilla.org/integration/mozilla-inbound/rev/37d8d0362318

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b09f6900fe8
https://hg.mozilla.org/mozilla-central/rev/455577de172e
https://hg.mozilla.org/mozilla-central/rev/16723f5b0307
https://hg.mozilla.org/mozilla-central/rev/37d8d0362318