973889 - XDR functions within with block.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently in XDRScript, we SGEV if we attempt to encode/clone a function which is within a With block, because FindScopeObjectIndex is called with ssi.block().

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Handle static with objects in XDR/Clone of inner functions.

Comment 2

[Andy Wingo [:wingo]]

Comment on attachment 8377552 [details] [diff] [review]
Handle static with objects in XDR/Clone of inner functions.

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

Nice catch!  One comment below.  It also seems that we could unify these cases by using a staticScope() accessor on StaticScopeIter; as you like, though.

::: js/src/jsscript.cpp
@@ +2769,5 @@
>                      RootedObject enclosingScope(cx);
>                      if (!ssi.done() && ssi.type() == StaticScopeIter<CanGC>::BLOCK)
>                          enclosingScope = objects[FindScopeObjectIndex(src, ssi.block())];
> +                    else if (ssi.type() == StaticScopeIter<CanGC>::WITH)
> +                        enclosingScope = objects[FindScopeObjectIndex(src, ssi.staticWith())];

Do we not need an !ssi.done() check here?

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Andy Wingo [:wingo] from comment #2)
> Comment on attachment 8377552 [details] [diff] [review]
> Handle static with objects in XDR/Clone of inner functions.
> 
> Review of attachment 8377552 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice catch!  One comment below.  It also seems that we could unify these
> cases by using a staticScope() accessor on StaticScopeIter; as you like,
> though.
> 
> ::: js/src/jsscript.cpp
> @@ +2769,5 @@
> >                      RootedObject enclosingScope(cx);
> >                      if (!ssi.done() && ssi.type() == StaticScopeIter<CanGC>::BLOCK)
> >                          enclosingScope = objects[FindScopeObjectIndex(src, ssi.block())];
> > +                    else if (ssi.type() == StaticScopeIter<CanGC>::WITH)
> > +                        enclosingScope = objects[FindScopeObjectIndex(src, ssi.staticWith())];
> 
> Do we not need an !ssi.done() check here?

Good catch, yes, will would need it to mirror what the XDR function is doing.

I will add it here, unless you prefer if I just reorder the whole conditions to look exactly like the XDR function?

Comment 4

[Andy Wingo [:wingo]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > Do we not need an !ssi.done() check here?
> 
> I will add it here, unless you prefer if I just reorder the whole conditions
> to look exactly like the XDR function?

Probably best to make them look the same, yes.  Either way is fine, your choice.

Comment 5

[Nicolas B. Pierron [:nbp]]

I still have not landed this patch because of an exact-rooting/ggc failure while running the test-suite on Try.
I will investigate it later today.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> I still have not landed this patch because of an exact-rooting/ggc failure
> while running the test-suite on Try.
> I will investigate it later today.

I opened Bug 977011 for the problem, and modified the test case to work around this issue while checking the with scope.

https://hg.mozilla.org/integration/mozilla-inbound/rev/af02f3883d75

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af02f3883d75