894149 - The set of closed over variables is not optimized hard enough when references are made to properties the global object

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The title of this bug is a little vague, but we have a good testcase in bug 894135.

The code looks something like

function outer() {
  var thingThatShouldNotBeClosedOver;

  iframe.addEventListener("stuff", function(e) {
    // Do something innocuous stuff
    document.foo();
    // Do some more innocuous stuff
  });
};

Referencing document (which is a property on the global) causes thingThatShouldNotBeClosedOver to be entrained.  If that variable uses or entrains a large amount of memory this can cause a sizable memory leak.  The fix we implemented is to write this as:

function outer() {
  var thingThatShouldNotBeClosedOver;

  var doc = document;
  iframe.addEventListener("stuff", function(e) {
    // Do something innocuous stuff
    doc.foo();
    // Do some more innocuous stuff
  });
};

which does not entrain thingThatShouldNotBeClosedOver.  But this needs a JS engine fix.

Comment 1

[Luke Wagner [:luke]]

By itself, accessing a property of the global object won't entrain enclosing scopes (in fact, your change entrains *more* scopes); furthermore, not-closed-over vars won't be captured even if their scope is entrained.  There must be something more sinister afoot.  I think getting a reduced testcase (perhaps leaking a huge array in thingThatShouldNotBeClosedOver) would point to the problem.

Comment 2

[Steve Fink [:sfink] [:s:]]

I was just following the discussion from home (literally), but comment 0 does not match my understanding of the situation. I understood it to be:

  function setDisplayedApp(origin, callback) {
    var homescreenFrame = ensureHomescreen();

    var app = runningApps[newApp];
    var iframe = app.iframe;

    app.frame.addEventListener(type, function apploaded(e) {
      e.target.removeEventListener(e.type, apploaded, true);
      var evt = document.createEvent('CustomEvent');
      iframe.dispatchEvent(evt);
    }, true);

    if (!currentApp && newApp == homescreen) {
      var zoomInCallback = function() {
        homescreenFrame.classList.remove('zoom-in');
      };
      openWindow(newApp, zoomInCallback);
    }
  }

The problem, in the unlikely even that I understood all this correctly, is: zoomInCallback (correctly) closes over homescreenFrame. It does this by keeping a reference to a Call object for setDisplayedApp, and that Call object is shared across all closures. apploaded closes over some stuff -- document, iframe, and apploaded itself -- so it ends up hanging onto the Call object too. So now even when zoomInCallback is dead, apploaded hangs onto the reference to the Call object containing a homescreenFrame it doesn't even need, and you get your leak.

I don't know what the exact rules are for when apploaded hangs onto the Call object -- if it only closed over things from outside of setDisplayedApp(), would it point over the problematic Call object? Does the self-reference (to apploaded) count against it here? But in this case, iframe is being closed over, so none of that matters.

If this were to be fixed, I would think it would require chopping up the Call object into pieces, perhaps into a whole tree of references in the general case, which would suck for memory use and possibly for performance. (The closures need to refer to the *same* variable, so you can't just create a Call object for each one. And two closures may refer to overlapping subsets of the variables in scope, so they'd need to have multiple pointers or a chained tree of subscope objects or something.)

Anyway, I should shut up, since I don't know what I'm talking about. But given that this bug is likely to get wide viewership, I didn't want to leave the impression that this was a simple problem where the engine is doing something wildly wrong for no good reason, with a straightforward fix.

If I actually understood the data structures involved, I would suggest refcounting the closure -> scope variable links individually, or giving them private slot-granularity mark bits or something. But I don't, so I won't.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes, it appears there was some sort of non-determinism in our steps to reproduce that convinced us that that fixed the issue, but it was not the case.  I'll file a new bug on the safe-for-space closure stuff.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I filed bug 894971.