83573 - XMLHttpRequest should use load groups

Status: RESOLVED FIXED

(Core :: XML, defect, P3)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

We should use the document's load group in XMLHttpRequest so that things will
get cleaned up properly when we go to another page.

If we are called from JS we should be able to get it following the practices in
nsHTMLImageElement.cpp.

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Johnny, you meantioned I can get the loadgroup from JS stack or something if
XMLHttpRequest is used from JS. How do I do that?

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: work-in-process

This patch seems to work, but I don't like the side effects. 

In IE, the throbber will not spin, and pressing STOP button has no effect. With
this patch, the trobber will spin when XMLHttpRequest is active, and pressing
STOP will stop request. Also, in Mozilla the throbber NEVER stops, even after
succesful request, or after pressing STOP.

Darin, do you have any ideas on how to achieve the functionality that
XMLHttpRequest stops automatically when user leaves page, but will NOT make
throbber active and will NOT respond to STOP button?

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

URL is a testcase for both IE & Mozilla. There is a 5 second delay on the
server, so you have time to observe behavior, hit stop, leave page etc.

Comment 4

[Darin Fisher]

try setting the LOAD_BACKGROUND load flag on the channel being added to the load
group.

Comment 5

[Darin Fisher]

LOAD_BACKGROUND should take care of the throbber business, but the STOP button
should still kill off the request if it lives in a load group.  however, iirc
the STOP button will go inactive once the throbber goes inactive.

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Proposed fix

Thanks, that did the trick :)

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Greg & other users of XMLHttpRequest: once this fix gets checked in, you should
no longer need to call XMLHttpRequest.abort() on the page's onunload handler.
This is a significant change, so it would be great if you could test your
applications as soon as possible after this has been fixed.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 95485 [details] [diff] [review]
Proposed fix

>+nsresult 
>+nsXMLHttpRequest::GetLoadGroup(nsILoadGroup **aLoadGroup)
>+{
>+  if (!mScriptContext) {
>+    GetCurrentContext(getter_AddRefs(mScriptContext));
>+  }
>+
>+  if (mScriptContext) {
>+    nsCOMPtr<nsIScriptGlobalObject> global;
>+    mScriptContext->GetGlobalObject(getter_AddRefs(global));
>+    nsCOMPtr<nsIDOMWindow> window = do_QueryInterface(global);
>+    if (window) {
>+      nsCOMPtr<nsIDOMDocument> domdoc;
>+      window->GetDocument(getter_AddRefs(domdoc));
>+      nsCOMPtr<nsIDocument> doc = do_QueryInterface(domdoc);
>+      if (doc) {
>+        doc->GetDocumentLoadGroup(aLoadGroup);
>+      }
>+    }
>+  }
>+
>+  return NS_OK;
>+}

Could you use an
if (!foo) {
  return NS_OK;
}
...

pattern here instead of

if (foo) {
  ...
}
return NS_OK;

That should avoid the deep nesting. You could even use NS_ENSURE_TRUE on the
two last since thay should never fail.

with that, r=sicking

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

oh, and you need to set *aLoadGroup to nsnull at the top of the function in case
the doc->GetDocumentLoadGroup(aLoadGroup) line isn't reached

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix v2

I usually prefer less return points from functions unless the indentation
becomes really deep, so I did not change that.

I did add the nulling out of out param, good catch!

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 96725 [details] [diff] [review]
Fix v2

r=sicking

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 96725 [details] [diff] [review]
Fix v2

sr=bzbarsky

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Checked in.