1120028 - DebugScopes::updateLiveScopes doesn't work with rematerialized frames

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

A frame's prevUpToDate bit is supposed to mean that it, and all frames older than it, have an entry in the liveScopes map. It is set by updateLiveScopes and only cleared by popping the frame.

This scheme needs to be revised for rematerialized frame. Consider 2 frames, with F1 being younger:

[0] F1 (interpreter, prevUpToDate=false)
[1] F2 (ion, not rematerialized)

First, we call updateLiveScopes and put F1 in liveScopes. F2 is skipped because it is not rematerialized and has no usable AbstractFramePtr.

Now we have:

[0] F1 (interpreter, prevUpToDate=true)
[1] F2 (ion, not rematerialized)

Then, we rematerialize F2. But not updateLiveScopes will never put an entry in liveScopes for F2, because F1's prevUpToDate is still true:

[0] F1 (interpreter, prevUpToDate=true)
[1] F2 (ion, rematerialized, prevUpToDate=false)

We've broken the invariant that prevUpToDate implies prevUpToDate on all older frames.

This should be fixed by rematerialization setting all the prevUpToDate bits to false on all younger frames than the one rematerialized.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Clear prevUpToDate of younger frames when rematerializing frames.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8546953 [details] [diff] [review]
Clear prevUpToDate of younger frames when rematerializing frames.

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

::: js/src/vm/ScopeObject.cpp
@@ +2407,5 @@
> +
> +        if (frame.scopeChain()->compartment() != cx->compartment())
> +            continue;
> +
> +        if (frame.isFunctionFrame() && frame.callee()->isGenerator())

I know updateLiveScopes has these two checks, but I think it would be conservatively safer to leave them out so that these two pieces of code don't have to be kept in sync.  Also, the more I think about it, the more it seems like continue'ing when frame.compartment != cx.compartment is wrong in updateLiveScopes for cross-global callstacks (and could lead to hasLiveScope unnecessarily returning null).

@@ +2417,5 @@
> +        for (ScopeIter si(frame, i.pc(), cx); !si.done(); ++si) {
> +            if (si.hasScopeObject()) {
> +                MOZ_ASSERT(si.scope().compartment() == cx->compartment());
> +                DebugScopes *scopes = cx->compartment()->debugScopes;
> +                scopes->liveScopes.remove(&si.scope());

I think it's fine to leave the scopes in liveScopes.  Consider that updateLiveScopes re-puts all scopes of the frame for the youngest frame every time it's called.  Given that, you could also take ou the prevUpToDate() check above since there's no point.

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #2)
> Comment on attachment 8546953 [details] [diff] [review]
> Clear prevUpToDate of younger frames when rematerializing frames.
> 
> Review of attachment 8546953 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/ScopeObject.cpp
> @@ +2407,5 @@
> > +
> > +        if (frame.scopeChain()->compartment() != cx->compartment())
> > +            continue;
> > +
> > +        if (frame.isFunctionFrame() && frame.callee()->isGenerator())
> 
> I know updateLiveScopes has these two checks, but I think it would be
> conservatively safer to leave them out so that these two pieces of code
> don't have to be kept in sync.  Also, the more I think about it, the more it
> seems like continue'ing when frame.compartment != cx.compartment is wrong in
> updateLiveScopes for cross-global callstacks (and could lead to hasLiveScope
> unnecessarily returning null).

updateLiveScopes operates compartment-at-a-time though, prevUpToDate only applies to previous frames in the same compartment. I don't see why it's wrong to for cross-global callstacks here.

> 
> @@ +2417,5 @@
> > +        for (ScopeIter si(frame, i.pc(), cx); !si.done(); ++si) {
> > +            if (si.hasScopeObject()) {
> > +                MOZ_ASSERT(si.scope().compartment() == cx->compartment());
> > +                DebugScopes *scopes = cx->compartment()->debugScopes;
> > +                scopes->liveScopes.remove(&si.scope());
> 
> I think it's fine to leave the scopes in liveScopes.  Consider that
> updateLiveScopes re-puts all scopes of the frame for the youngest frame
> every time it's called.  Given that, you could also take ou the
> prevUpToDate() check above since there's no point.

Good catch, will do.

Comment 4

[Luke Wagner [:luke]]

(In reply to Shu-yu Guo [:shu] from comment #3)
> updateLiveScopes operates compartment-at-a-time though, prevUpToDate only
> applies to previous frames in the same compartment. I don't see why it's
> wrong to for cross-global callstacks here.

Hmm, I guess I can't really think of a counter-example; n/m.

Comment 5

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c331251a037

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1c331251a037