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

RESOLVED FIXED

Status

()

Core
XML
RESOLVED FIXED
14 years ago
5 years ago

People

(Reporter: jst, Assigned: jst)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

14 years ago
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.
(Assignee)

Updated

14 years ago
Assignee: hjtoi-bugzilla → jst
(Assignee)

Comment 1

14 years ago
Created attachment 143758 [details] [diff] [review]
Implement multipart/x-mixed-replace for XMLHttpRequest.
(Assignee)

Updated

14 years ago
Attachment #143758 - Flags: superreview?(hjtoi-bugzilla)
Attachment #143758 - Flags: review?(darin)
(Assignee)

Comment 2

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

Comment 3

14 years ago
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)
Attachment #143758 - Flags: review?(darin) → review+
(Assignee)

Comment 4

14 years ago
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.
(Assignee)

Updated

14 years ago
Attachment #143758 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #143976 - Flags: superreview?(hjtoi-bugzilla)
Attachment #143976 - Flags: review+

Comment 5

14 years ago
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 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...
Attachment #143976 - Flags: superreview?(hjtoi-bugzilla) → superreview+
Attachment #143758 - Flags: superreview?(hjtoi-bugzilla)
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.
(Assignee)

Comment 8

14 years ago
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.
Attachment #143976 - Flags: approval1.7b?

Comment 9

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

a=chofmann for 1.7b
Attachment #143976 - Flags: approval1.7b? → approval1.7b+
(Assignee)

Comment 10

14 years ago
Created attachment 144021 [details] [diff] [review]
Patch with all the fixes.

This is what was checked in.
(Assignee)

Comment 11

14 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 12

14 years ago
If there was a testcase for this bug, could you please post it. Thanks.
(Assignee)

Comment 13

14 years ago
See URL for a trivial testcase.
(Assignee)

Comment 14

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

Comment 15

14 years ago
Hi Johnny. Did you get a chance to get the testcase? Thanks, I really appreciate it.
(Assignee)

Comment 16

14 years ago
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

14 years ago
Thanks! This is cool stuff.

Comment 18

5 years ago
Why don't you implement this amazing stuff also for Chrome?
After 9 years, nobody did .... ^__^
You need to log in before you can comment on or make changes to this bug.