Opened 6 years ago

Closed 6 years ago

#16794 closed defect (fixed)

dojo xhrs both leak memory

Reported by: haysmark Owned by: Bryan Forbes
Priority: blocker Milestone: 1.9
Component: IO Version: 1.8.3
Keywords: Cc: ben hockey, Eugene Lazutkin
Blocked By: Blocking:

Description

Both dojo._base.xhr and dojo.request.xhr leak their responses. If you make an xhr request many times in a long-running application, the Task Manager will report that the browser's process continues to grow. See the attached test case for a simulated example. Note that native XHR does not leak its response. The test was run in IE9 set to load a 1MB powerpoint repeatedly to make it easy to spot the problem.

The Deferred from a dojo.request.xhr has a "response" object on it. I've found that nulling out the fields 'text', 'data', and 'xhr' fields is effective at eliminating the leak; the "workaround" button in the test does exactly that.

The question is: when should we delete these fields? As the attached test case shows, it seems as if the invoking code is ultimately responsible for nulling out these fields, but we do not prescribe this action in our documentation. Worse, the mechanism that the legacy dojo._base.xhr uses to wrap dojo.request.xhr prevents the user from fixing the leak in this way.

Attachments (1)

dojoXhrGet.html (1.8 KB) - added by haysmark 6 years ago.
Test case.

Download all attachments as: .zip

Change History (7)

Changed 6 years ago by haysmark

Attachment: dojoXhrGet.html added

Test case.

comment:1 Changed 6 years ago by ben hockey

Cc: ben hockey added

comment:2 Changed 6 years ago by Eugene Lazutkin

Cc: Eugene Lazutkin added

comment:3 Changed 6 years ago by bill

Component: CoreIO
Owner: set to Bryan Forbes

I don't know a good solution either. Like you said, only the app knows when it's OK to delete those fields.

And to state the obvious, this must be a bug in IE's garbage collection because there shouldn't be any leak.

comment:4 Changed 6 years ago by Bryan Forbes

In [30992]:

Fixes circular reference between promises. !strict refs #16794

comment:5 Changed 6 years ago by Bryan Forbes

In [30993]:

Fixes circular reference between promises. (1.8 backport) !strict refs #16794

comment:6 Changed 6 years ago by Bryan Forbes

Milestone: tbd1.9
Priority: undecidedblocker
Resolution: fixed
Status: newclosed

I discovered that this was not a leak from the text, data, or xhr properties (and, in fact, nulling those out didn't solve the problem anyway), but a circular reference leak from the following line in dojo/request/util.js:

var promise = freeze(lang.delegate(dataPromise, {
        response: responsePromise
}));

The circular reference goes like this: responsePromise -> responsePromise.then() -> dataPromise -> promise.__prototype__ -> promise['response'] -> responsePromise. Normally, this should be garbage collected, but I'm guessing the addition of the prototype in there is throwing IE off. As soon as I removed the lang.delegate from the equation, it stopped leaking in IE. But this introduced a new problem: the promise property of dojo/Deferred is frozen on newer platforms (which is what lang.delegate is used to get around). I ended up doing the following:

var promise = new Promise();
for (var prop in dataPromise) {
        if (dataPromise.hasOwnProperty(prop)) {
                promise[prop] = dataPromise[prop];
        }
}
promise.response = responsePromise;
freeze(promise);

Creating a dojo/promise/Promise keeps dojo/when from creating a new dojo/Deferred to wrap this promise (it wraps anything that isn't an instance of dojo/promise/Promise) and it keeps a circular reference from occurring through the prototype.

Note: See TracTickets for help on using tickets.