622781 - entry frame Call object can differ between recording and trace

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

The tracer assumes that a call object seen at record time is the exact same as seen on trace.  This is enforced/documented by guardCallee.  There is a loophole however (also the cause of bug 620637 (see comment 3)) in that the identity of the entry frame's scope chain is not guarded upon.  This means that setCallProp can write to the call object when it needs to write to the stack:

function outer(b) {
    var upvar = 0;
    var g = function() {they need to write to / 
        for (var i = 0; i < 15; ++i)
            upvar = 9;
    }
    if (!b)
        return g;
    g();
    assertEq(upvar, 9);  // got 0, expect 9
}
outer(false)();
outer(true);

(Because call objects' reserved slots are initialized to undefined, the analogous test case for callProp always branch exits safely.)

This is rather annoying to fix: either we have to stop depending on the same-call-object-identity property or include scopeChain in fragment lookup.

AFAICS, this is only an unlikely correctness issue, nothing exploitable, since, active or not, call objects always have backing reserved slots which are initialized to undefined.  So not nominating for blocking.

Comment 1

[Luke Wagner [:luke]]

Oops, cut out the "they need to write to /" text in the testcase.

Comment 2

[Brendan Eich [:brendan]]

Guarding on entry frame scope chain identity looks necessary to me.

/be

Comment 3

[Luke Wagner [:luke]]

Reading TR::traverseScopeChain, it seems that we have logic that is intended to deal with scope chain variance caused by this variance in the entry frame's Call object.  IIUC, once we fix this bug by guarding on entry frame scope chain identity, we have a pretty strong invariant and, at first glance, it seems like a lot of TR::traverseScopeChain can be ripped out.

Comment 4

[Brendan Eich [:brendan]]

This blocks a softblocker, nominating. Luke, were you gonna fix this one?

/be

Comment 5

[Luke Wagner [:luke]]

I hadn't planned on it (until the part about it blocking a softblocker), but I can.

Comment 6

[Luke Wagner [:luke]]

The fear is that, by keying traces on the scope chain, we will endlessly record and flush.  E.g., this should not constantly record:

  for (...)
    (function() { eval(''); for (var i = 0; i < 20; ++i) {} })()

The same thing can happen atm for change global shape (just filed bug 627600 on that), but this seems a lot more likely.

This could be fixed with bug 627600 but it still leaves us effectively not recording loops in heavyweight functions which get called repeatedly.  Maybe this is rare.  Also, we guard on the parent (i.e., entrained scope) of functions called *from* trace, so we are already mostly in this situation.

A more attractive option is to remove all dependencies on the identity of the scope chain (going more the route of traverseScopeChain).  The whole area is a bit of a hairball, though.  I'll think some more about it; suggestions welcome.

Comment 7

[Luke Wagner [:luke]]

Both options in comment 6 seem risky for FF 4.0 (esp. compared to the relatively small fix in bug 624110), so I'm tentatively proposing that bug 624110 land its patch (after review, of course) and this unblock.

Comment 8

[Luke Wagner [:luke]]

Throwing back into the pool.