Closed Bug 894149 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: khuey, Unassigned)

References

Details

(Whiteboard: [MemShrink])

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.
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.
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.
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.