237319 - Add support for server push using multipart/x-mixed-replace with XMLHttpRequest.

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Johnny Stenback (:jst)]

If we'd have support for multipart/x-mixed-replace in XMLHttpRequest, it would
enable web app developers to push data from the server in stead of always having
the client poll data off of the server. While multipart/x-mixed-replace isn't
standardized, it's been around for a long time, and in stead of re-inventing the
wheel we (me and darin) decided to go with using what we have.

Patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Implement multipart/x-mixed-replace for XMLHttpRequest.

Comment 2

[Johnny Stenback (:jst)]

Duh, ignore the addition of the aContinue argument in the prototype of
RequestCompleted(). Leftovers from some testing...

Comment 3

[Darin Fisher]

Comment on attachment 143758 [details] [diff] [review]
Implement multipart/x-mixed-replace for XMLHttpRequest.

>Index: extensions/xmlextras/base/src/nsXMLHttpRequest.cpp

>+    nsLoadListenerProxy* proxy = new nsLoadListenerProxy(requestWeak);
>+    if (!proxy) return NS_ERROR_OUT_OF_MEMORY;
>+
>+    // This will addref the proxy
>+    rv = target->AddEventListenerByIID(NS_STATIC_CAST(nsIDOMEventListener*, 
>+                                                      proxy), 
>+                                       NS_GET_IID(nsIDOMLoadListener));
>+    if (NS_FAILED(rv)) return NS_ERROR_FAILURE;

so if AddEventListener should return a failure, then won't we leak
proxy?


>+NS_IMETHODIMP
>+nsMultipartProxyListener::OnStartRequest(nsIRequest *aRequest,
...
>+  if (!contentType.Equals(NS_LITERAL_CSTRING("multipart/x-mixed-replace"))) {
>+    return NS_ERROR_INVALID_ARG;
>+  }

so this nsMultipartProxyListener only really exists to enforce
this content type rule, right?	because otherwise, you could have
just created the stream converter up-front.


>Index: extensions/xmlextras/base/src/nsXMLHttpRequest.h

>+class nsMultipartProxyListener : public nsIStreamListener
...
>+  virtual ~nsMultipartProxyListener();

no need for a virtual dtor i think.  our own Release is going
to call delete.


r=darin (though i'm not that familiar with some of the state changes in this
code)

Comment 4

[Johnny Stenback (:jst)]

Attachment: Set load flags to enable multiple simultanous XML streams.

This sets the loadflags nsIRequest::LOAD_BYPASS_CACHE and
nsIRequest::INHIBIT_CACHING on multipart requests to let more than one be open
at the same time. This also fixes the leak that darin pointed out (which was in
code I moved from one place to another). I left the nsMultipartProxyListener
destructor  virtual to keep the compilers from warning about virtual methods
but a non-virtual dtor.

Comment 5

[Darin Fisher]

FYI: i had to backout support for the INHIBIT_CACHING load flag since its
implementation was causing some regressions.  it shouldn't be a concern here,
since the LOAD_BYPASS_CACHE flag will prevent subsequent requests from getting
locked up behind an existing request to the same URL.  see bug 232385 for details.
INHIBIT_CACHING is still something you'd want since there is no reason to cache
this data.

Comment 6

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

Comment on attachment 143976 [details] [diff] [review]
Set load flags to enable multiple simultanous XML streams.

>Index: extensions/xmlextras/base/public/nsIXMLHttpRequest.idl
>===================================================================
>+  attribute boolean multipartResponse;

Could this be just "multipart", or could that cause confusion?

>Index: extensions/xmlextras/base/src/nsXMLHttpRequest.cpp
>===================================================================
> nsXMLHttpRequest::GetAllResponseHeaders(char **_retval)
> {
>   NS_ENSURE_ARG_POINTER(_retval);
>   *_retval = nsnull;
>-  nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(mChannel));
>+
>+  // Note that this will return all the headers from the *current*
>+  // part of a multipart request, not from the original channel.

Please move that comment to the IDL, as it really documents the interface.

> nsXMLHttpRequest::OpenRequest(const nsACString& method, 
>   if (mState & XML_HTTP_REQUEST_ABORTED) {
>-    // Proceed as normal
>+    // Unset our aborted state, and proceed as normal
>+
>+    mState &= ~XML_HTTP_REQUEST_ABORTED;

Please mention in the comment where/how abort is really handled.

Does hitting ESC still stop XMLHttpRequest, both multipart and normal?

>@@ -1069,55 +1140,75 @@ nsXMLHttpRequest::OnStopRequest(nsIReque
>   mReadRequest = nsnull;
>+    mReadRequest = nsnull;

I don't think you need to do this again, since it was done earlier in the
function.

>+    mScriptContext->GC();

Yes! This is really cool. I think there might still be an open bug somewhere
about XMLHttpRequest "leaking" memory, while in reality it is just that the GC
was not triggered. I offered a workaround in that bug to create some 20 dummy
JS objects per each XMLHttpRequest load. Now that won't be necessary. I can't
find that bug now, but if someone notices it it you can mark it fixed after
this.

>-    // This will addref the proxy
>-    rv = target->AddEventListenerByIID(NS_STATIC_CAST(nsIDOMEventListener*, 
>-                                                      proxy), 
>-                                       NS_GET_IID(nsIDOMLoadListener));

Just a generic comment on all this reference stuff; it is pretty easy to make
XMLHttpRequest objects and its dependants have circular references and cause
memory leaks. I think we got all fixed, so just be extra careful when you touch
this stuff...

>+    listener = new nsMultipartProxyListener(this);

Just worried about the circular refs, so be careful...

>+    mChannel->GetLoadFlags(&flags);
>+    flags |= nsIRequest::LOAD_BYPASS_CACHE | nsIRequest::INHIBIT_CACHING;

Which reminds me: currently XMLHttpRequest might have a bug where it always
uses cached document, see bug 200706. I think we should fix that bug at the
same time as this for consistency.

convServ->AsyncConvertData(NS_LITERAL_STRING("multipart/x-mixed-replace").get()
,
>+                                    NS_LITERAL_STRING("*/*").get(),
>+                                    toListener,
>+                                    nsnull,
>+                                    getter_AddRefs(fromListener));

This goes into area I know nothing about, so I trust you & Darin to know how
this should work.

>Index: extensions/xmlextras/base/src/nsXMLHttpRequest.h
>===================================================================

>+// Helper proxy class to be used when expecting an
>+// multipart/x-mixed-replace stream of XML documents.
>+
>+class nsMultipartProxyListener : public nsIStreamListener

I have a slight preference to place this class completely in the .cpp file
because it does not show outside in any way.

If you get this in time for 1.7b, you should inform Asa to include this in the
what's new notes. I think this is pretty big...

Comment 7

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

Hmm, I think you should mention in the multipart members IDL comment which are
all of the attributes and methods affected. Easier than adding a comment to
every affected property... But in any case it should be mentioned somewhere what
will be different.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 143976 [details] [diff] [review]
Set load flags to enable multiple simultanous XML streams.

I'll fix all of the above issues raised by Heikki (thanks for the sr!),
requesting for approval since this would be a cool new feature for 1.7, but I
wouldn't want to ship it w/o including it in a beta first.

Comment 9

[chris hofmann]

Comment on attachment 143976 [details] [diff] [review]
Set load flags to enable multiple simultanous XML streams.

a=chofmann for 1.7b

Comment 10

[Johnny Stenback (:jst)]

Attachment: Patch with all the fixes.

This is what was checked in.

Comment 11

[Johnny Stenback (:jst)]

Fix checked in.

Comment 12

[Eric Murphy]

If there was a testcase for this bug, could you please post it. Thanks.

Comment 13

[Johnny Stenback (:jst)]

See URL for a trivial testcase.

Comment 14

[Johnny Stenback (:jst)]

Um, never mind. I apparently deleted part of that from the server. I'll see if I
can recreate it tomorrow.

Comment 15

[Eric Murphy]

Hi Johnny. Did you get a chance to get the testcase? Thanks, I really appreciate it.

Comment 16

[Johnny Stenback (:jst)]

Eric, yes, I got it up again a few days ago. Forgot to update this bug :-(

See http://www.jstenback.com/mozilla/bugs/237319.html for a trivial testcase
(server pushes an XML document with the current date/time in it every 5 seconds
while running).

Comment 17

[Eric Murphy]

Thanks! This is cool stuff.

Comment 18

[Dario Palumbo]

Why don't you implement this amazing stuff also for Chrome?
After 9 years, nobody did .... ^__^