Last Comment Bug 237319 - Add support for server push using multipart/x-mixed-replace with XMLHttpRequest.
: Add support for server push using multipart/x-mixed-replace with XMLHttpRequest.
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Johnny Stenback (:jst,
: Ashish Bhatt
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2004-03-12 16:36 PST by Johnny Stenback (:jst,
Modified: 2013-01-25 09:37 PST (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Implement multipart/x-mixed-replace for XMLHttpRequest. (29.98 KB, patch)
2004-03-12 16:42 PST, Johnny Stenback (:jst,
darin.moz: review+
Details | Diff | Splinter Review
Set load flags to enable multiple simultanous XML streams. (29.81 KB, patch)
2004-03-15 12:57 PST, Johnny Stenback (:jst,
jst: review+
hjtoi-bugzilla: superreview+
chofmann: approval1.7b+
Details | Diff | Splinter Review
Patch with all the fixes. (34.05 KB, patch)
2004-03-15 22:34 PST, Johnny Stenback (:jst,
no flags Details | Diff | Splinter Review

Description Johnny Stenback (:jst, 2004-03-12 16:36:49 PST
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, 2004-03-12 16:42:29 PST
Created attachment 143758 [details] [diff] [review]
Implement multipart/x-mixed-replace for XMLHttpRequest.
Comment 2 Johnny Stenback (:jst, 2004-03-12 17:11:54 PST
Duh, ignore the addition of the aContinue argument in the prototype of
RequestCompleted(). Leftovers from some testing...
Comment 3 Darin Fisher 2004-03-12 18:30:28 PST
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

>+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
Comment 4 Johnny Stenback (:jst, 2004-03-15 12:57:38 PST
Created attachment 143976 [details] [diff] [review]
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 2004-03-15 13:21:50 PST
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) 2004-03-15 21:38:28 PST
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)
> {
>   *_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

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

>+    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 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.

>+                                    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) 2004-03-15 21:49:43 PST
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, 2004-03-15 22:04:47 PST
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 2004-03-15 22:07:49 PST
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, 2004-03-15 22:34:46 PST
Created attachment 144021 [details] [diff] [review]
Patch with all the fixes.

This is what was checked in.
Comment 11 Johnny Stenback (:jst, 2004-03-15 23:03:30 PST
Fix checked in.
Comment 12 Eric Murphy 2004-04-15 20:45:03 PDT
If there was a testcase for this bug, could you please post it. Thanks.
Comment 13 Johnny Stenback (:jst, 2004-04-15 23:17:01 PDT
See URL for a trivial testcase.
Comment 14 Johnny Stenback (:jst, 2004-04-15 23:21:32 PDT
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 2004-04-19 19:35:30 PDT
Hi Johnny. Did you get a chance to get the testcase? Thanks, I really appreciate it.
Comment 16 Johnny Stenback (:jst, 2004-04-19 20:54:01 PDT
Eric, yes, I got it up again a few days ago. Forgot to update this bug :-(

See 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 2004-04-20 19:58:52 PDT
Thanks! This is cool stuff.
Comment 18 Dario Palumbo 2013-01-25 09:37:53 PST
Why don't you implement this amazing stuff also for Chrome?
After 9 years, nobody did .... ^__^

Note You need to log in before you can comment on or make changes to this bug.