Closed Bug 237319 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: XML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jst, Assigned: jst)

References

()

Details

Attachments

(2 files, 1 obsolete file)

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: hjtoi-bugzilla → jst
Attachment #143758 - Flags: superreview?(hjtoi-bugzilla)
Attachment #143758 - Flags: review?(darin)
Duh, ignore the addition of the aContinue argument in the prototype of
RequestCompleted(). Leftovers from some testing...
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+
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.
Attachment #143758 - Attachment is obsolete: true
Attachment #143976 - Flags: superreview?(hjtoi-bugzilla)
Attachment #143976 - Flags: review+
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.
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 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+
This is what was checked in.
Fix checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
If there was a testcase for this bug, could you please post it. Thanks.
See URL for a trivial testcase.
Um, never mind. I apparently deleted part of that from the server. I'll see if I
can recreate it tomorrow.
Hi Johnny. Did you get a chance to get the testcase? Thanks, I really appreciate it.
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).
Thanks! This is cool stuff.
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.