382871 - XMLHttpRequest JS wrapper not preserved by its event handlers

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Carl Fürstenberg]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.3) Gecko/20070310 Iceweasel/2.0.0.3 (Debian-2.0.0.3-2)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.3) Gecko/20070310 Iceweasel/2.0.0.3 (Debian-2.0.0.3-2)

The problem is t hat when using XmlHttpRequest object in a loop, after a while, a properties defined on the object is getting lost (undefined).
After a long discussion on the #js channel on irc.mozilla.org, the probable reason for the problem is that the garbage collector is removing properties from the object, since there are no one visible hanging on the XHR object. The problem was solved when enclosing the cose in a closure. Also, if activating firebig, the problem dissaper.

Reproducible: Always

Steps to Reproduce:
1.As this is only tested on wikipedia, to test the testcase, create an account on wikipedia.
2. add following to your monobook.js page:
 importScript('User:AzaToth/morebits.js');
 importScript('User:AzaToth/testcase.js');
3.push the button testcase
Actual Results:  
going for http://en.wikipedia.org/w/api.php?action=query&list=imageusage&titles=Image%3AExample.png&iulimit=50&format=xml
got 50 nodes
title: Wikipedia:WikiProject Albums
title: Talk:List of English monarchs
title: Wikipedia:Images
title: User talk:Masssiveego
title: User:Phil Boswell/Sandbox
title: Talk:Banjo-Kazooie
title: MediaWiki talk:Monobook.css
title: User:DO'Neil/Images Uploaded/Art
title: User:DO'Neil/Images Uploaded/Propaganda Material
title: User:Impi/sandbox
title: User:MacGyverMagic/Editing
title: Template:Infobox Album
title: Template talk:Infobox SuperBowl
title: User:Harajuku
title: User:Pikachu9000
title: Talk:List of The X-Files episodes
title: Portal:Test
title: User:Chochopk
title: User:Reisio/Sandbox
title: User:McPhail
title: User talk:Derex/1
title: Template talk:FFIX character
title: Help talk:Pipe trick
title: Template:ITC Productions Page Layout
title: Template talk:Infobox Television episode
title: User:Gakon5/Infoboxes
title: User:Redwolf24/Bootcamp heading
title: User:Redwolf24/Bootcamp
title: User:CDA/thumb
title: Wikipedia:Help desk/Archive 27
title: User:Paul Klenk/Sandbox
title: User talk:AHEMSLTD
title: User:Duncharris/playbox
title: User:NoSeptember/Main
title: User talk:Wikisoft*
title: Template talk:Album infobox soundtrack
title: User:Darren Jowalsen/workpad
title: User:SoM/Blink/test
title: User talk:CedarSavage
title: User:Jogers/Sandbox
title: Template:EU Agency
title: User:Jtmichcock/Sandbox
title: Template:MedalTop
title: Template:MedalSport
title: Template:MedalGold
title: Template:MedalSilver
title: Template:MedalBronze
title: Template:MedalBottom
title: Template talk:MedalTop
title: Template:MedalDisqualified
Added tag for Wikipedia:Images
Added tag for Talk:List of English monarchs
No tag for Talk:Banjo-Kazooie
Added tag for MediaWiki talk:Monobook.css
Added tag for User:DO'Neil/Images Uploaded/Propaganda Material
Added tag for User talk:Masssiveego
No tag for undefined
Added tag for undefined
Added tag for undefined
No tag for undefined
Added tag for undefined
Added tag for undefined
No tag for undefined
Added tag for undefined
No tag for undefined
No tag for undefined
No tag for undefined
No tag for undefined
Added tag for undefined
No tag for undefined
Added tag for undefined
No tag for undefined
Added tag for undefined
No tag for undefined
No tag for undefined
Added tag for undefined
No tag for undefined
No tag for undefined
Added tag for undefined
No tag for undefined
No tag for undefined
No tag for undefined
No tag for undefined
Added tag for undefined
No tag for undefined
No tag for undefined
Added tag for undefined
No tag for undefined
No tag for undefined
Added tag for undefined
No tag for undefined
No tag for undefined
No tag for undefined
No tag for undefined
No tag for undefined
No tag for Template:MedalDisqualified
No tag for undefined
No tag for undefined
No tag for undefined
No tag for undefined

Expected Results:  
going for http://en.wikipedia.org/w/api.php?action=query&list=imageusage&titles=Image%3AExample.png&iulimit=50&format=xml
got 50 nodes
title: Wikipedia:WikiProject Albums
title: Talk:List of English monarchs
title: Wikipedia:Images
title: User talk:Masssiveego
title: User:Phil Boswell/Sandbox
title: Talk:Banjo-Kazooie
title: MediaWiki talk:Monobook.css
title: User:DO'Neil/Images Uploaded/Art
title: User:DO'Neil/Images Uploaded/Propaganda Material
title: User:Impi/sandbox
title: User:MacGyverMagic/Editing
title: Template:Infobox Album
title: Template talk:Infobox SuperBowl
title: User:Harajuku
title: User:Pikachu9000
title: Talk:List of The X-Files episodes
title: Portal:Test
title: User:Chochopk
title: User:Reisio/Sandbox
title: User:McPhail
title: User talk:Derex/1
title: Template talk:FFIX character
title: Help talk:Pipe trick
title: Template:ITC Productions Page Layout
title: Template talk:Infobox Television episode
title: User:Gakon5/Infoboxes
title: User:Redwolf24/Bootcamp heading
title: User:Redwolf24/Bootcamp
title: User:CDA/thumb
title: Wikipedia:Help desk/Archive 27
title: User:Paul Klenk/Sandbox
title: User talk:AHEMSLTD
title: User:Duncharris/playbox
title: User:NoSeptember/Main
title: User talk:Wikisoft*
title: Template talk:Album infobox soundtrack
title: User:Darren Jowalsen/workpad
title: User:SoM/Blink/test
title: User talk:CedarSavage
title: User:Jogers/Sandbox
title: Template:EU Agency
title: User:Jtmichcock/Sandbox
title: Template:MedalTop
title: Template:MedalSport
title: Template:MedalGold
title: Template:MedalSilver
title: Template:MedalBronze
title: Template:MedalBottom
title: Template talk:MedalTop
title: Template:MedalDisqualified
Added tag for Wikipedia:Images
No tag for User:Phil Boswell/Sandbox
Added tag for Talk:List of English monarchs
No tag for Wikipedia:WikiProject Albums
Added tag for User talk:Masssiveego
Added tag for MediaWiki talk:Monobook.css
No tag for Talk:Banjo-Kazooie
Added tag for User:DO'Neil/Images Uploaded/Propaganda Material
Added tag for User:DO'Neil/Images Uploaded/Art
Added tag for User:MacGyverMagic/Editing
Added tag for User:Harajuku
Added tag for User:Pikachu9000
Added tag for Template talk:Infobox SuperBowl
No tag for Portal:Test
Added tag for Talk:List of The X-Files episodes
No tag for User:Reisio/Sandbox
No tag for User:Chochopk
No tag for User:McPhail
No tag for Template talk:FFIX character
Added tag for Template:ITC Productions Page Layout
No tag for Help talk:Pipe trick
Added tag for User talk:Derex/1
Added tag for User:Redwolf24/Bootcamp heading
No tag for User:Redwolf24/Bootcamp
No tag for User:Gakon5/Infoboxes
Added tag for Template talk:Infobox Television episode
No tag for User:Impi/sandbox
No tag for User:CDA/thumb
No tag for User:Duncharris/playbox
No tag for User:NoSeptember/Main
No tag for User talk:AHEMSLTD
Added tag for User:Paul Klenk/Sandbox
No tag for User talk:Wikisoft*
No tag for Template talk:Album infobox soundtrack
Added tag for User:SoM/Blink/test
No tag for User:Darren Jowalsen/workpad
No tag for User:Jogers/Sandbox
No tag for Template:EU Agency
No tag for User:Jtmichcock/Sandbox
No tag for Wikipedia:Help desk/Archive 27
No tag for Template:MedalSport
No tag for Template:MedalTop
No tag for Template:MedalGold
No tag for Template:MedalSilver
No tag for Template:MedalBronze
No tag for Template:MedalBottom
No tag for Template:MedalDisqualified
No tag for Template talk:MedalTop
No tag for Template:Infobox Album
Added tag for User talk:CedarSavage

perhaps it might work also if for each iteration, to add a reference to the object in an array.

Comment 1

[:Mook]

I /think/ this was what's happening:

var xhr = new XMLHttpRequest();
xhr.foo = something;
xhr.onload = function () {
  /* use |this.foo| here since |this| is the XHR */
}
xhr.send();
delete xhr;
// GC happens before the onload gets called

My suspicion from IRC was that GC caused the wrapper for the XHR to go away, losing the expando properties.  onload then can't find the expando properties it expects.

I'm not sure how to force a GC from JS... I'd need that to write a testcase :(

(This is probably a core bug, but I have no idea where to put it...)

Comment 2

[Adam Guthrie]

Moving to Core: General for now.

Comment 3

[Carl Fürstenberg]

Perhaps my confusion when I encountered this problem was that I couldn't see the implicit delete each iteration, as my idea was mostly like:
for(var i in foo) {
    var xhr = new XMLHttpRequest();
    xhr.foo = something;
    xhr.onload = function () {
      /* use |this.foo| here since |this| is the XHR */
    }
    xhr.send();
}
I know Javascript wasn't meant from the beginning to be used asynchronous, and that it's still single threaded (isn't it?). Perhaps I'm trying to use it beyond it's limitation, I don't know. But I can't see why the GC would clean up an active object. It's still receiving data I assume.

Comment 4

[:Mook]

You didn't have a delete, you had an (equivalent for our purposes)
xhr = null;
(well, xhr = something-else anyway).  delete just drops the reference anyway.

Comment 5

[Carl Fürstenberg]

Well, I was gleaning through the The XMLHttpRequest Object, W3C Working Draft at http://www.w3.org/TR/XMLHttpRequest/ I read this:

"When the constructor is invoked a pointer to the Window object which initially had XMLHttpRequest as an attribute of which the constructor was invoked must be stored on the newly created object, called the Window pointer. This pointer must persist even if the browsing context in which the Window is located is destroyed (by removing it from a parent browsing context, for instance)."

Comment 6

[Boris Zbarsky [:bzbarsky]]

smaug, you want to look at this?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: mochitest

Perhaps preserving the wrapper when XHR is created and releasing it after
Abort/Error/Load (unless XHR is re-opened) or in Unlink could work.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Hmm, can't release after Abort/Error/Load...
And I don't think we want to let CC to handle releasing always - lots of garbage
if some page creates many XHR objects.
I wish I could easily detect when expando properties are set.

Comment 9

[Boris Zbarsky [:bzbarsky]]

You can using classinfo, no?  The AddProperty hook?

That's what we do for Nodes, no?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Preserve wrapper if expandos are used. Mochitest for xhr and upload

Adds ::AddProperty to nsEventTargetSH (which is used by XHR and XHR.upload).
Moves mScriptContext and mOwner up to nsXHREventTarget so that both XHR and 
XHR.upload can use those. Wrapper is kept alive the same way as DOM nodes -
in the sort-of-owner document.

Jst, since you reviewed some of the latest XHR changes and are familiar with
DOMClassInfo... ;)

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 334932 [details] [diff] [review]
Preserve wrapper if expandos are used. Mochitest for xhr and upload

- In nsEventTargetSH::AddProperty():

+  nsISupports* native = wrapper->Native();
+  nsCOMPtr<nsPIDOMEventTarget> target = do_QueryInterface(native);
+  if (target) {
+    nsCOMPtr<nsIScriptContext> scriptContext;
+    target->GetContextForEventHandlers(getter_AddRefs(scriptContext));
+    if (scriptContext) {
+      nsCOMPtr<nsPIDOMWindow> window =
+        do_QueryInterface(scriptContext->GetGlobalObject());
+      if (window) {
+        nsCOMPtr<nsIDocument> doc =
+          do_QueryInterface(window->GetExtantDocument());
+        if (doc) {
+          doc->AddReference(native, wrapper);

This will add the reference to the document in the current inner window, which may not always be what you want. It would be more correct to get the global JS object from obj and get the document out of there instead.

r- based on that. The rest looks good.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> This will add the reference to the document in the current inner window, which
> may not always be what you want. It would be more correct to get the global JS
> object from obj and get the document out of there instead.
Indeed. But should we change DOM node's wrapper handling too (in a different bug).
I wanted XHR to have similar behavior as nodes, so I decided to use the "ownerDocument".

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> This will add the reference to the document in the current inner window
And no, the patch doesn't add to the document in the current inner window, but 
to the document of the owner window of XHR

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 334932 [details] [diff] [review]
Preserve wrapper if expandos are used. Mochitest for xhr and upload

re-asking review (and sr). If we want to change how we store wrappers, the same should happen to DOM nodes. I'm just trying to stay consistent how the platform works.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 334932 [details] [diff] [review]
Preserve wrapper if expandos are used. Mochitest for xhr and upload

Fair enough. Wanna file a followup bug to clean this up?