450452 - Implement XHR ("minus X") for worker threads

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Implement XHR ("minus X") for worker threads.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Here we go!

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Jonas, could you look over the changes I made to XHR?

Comment 3

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

Comment on attachment 338759 [details] [diff] [review]
Patch, v1

>diff --git a/content/base/public/nsIXMLHttpRequest.idl b/content/base/public/nsIXMLHttpRequest.idl
>--- a/content/base/public/nsIXMLHttpRequest.idl
>+++ b/content/base/public/nsIXMLHttpRequest.idl
>@@ -334,78 +334,11 @@ interface nsIXMLHttpRequest : nsISupport
>   [noscript] void init(in nsIPrincipal principal,
>                        in nsIScriptContext scriptContext,
>                        in nsPIDOMWindow ownerWindow);
>-};
> 
>-[scriptable, uuid(6e127bd2-b4c1-4a82-be0d-012bd24efb37)]
>-interface nsIXMLHttpRequestUploadGetter : nsISupports {
>   /**
>    * Upload process can be tracked by adding event listener to |upload|.
>    */
>   readonly attribute nsIXMLHttpRequestUpload upload;
>-};
>-
>-[scriptable, uuid(261676b4-d508-43bf-b099-74635a0ee2e9)]
>-interface nsIJSXMLHttpRequest : nsISupports {
>-  /**
>-   * Meant to be a script-only mechanism for setting a load event listener.
>-   * The attribute is expected to be JavaScript function object. When
>-   * the load event occurs, the function is invoked.
>-   * This attribute should not be used from native code!!
>-   *
>-   * After the initial response, all event listeners will be cleared.
>-   * // XXXbz what does that mean, exactly?   
>-   *
>-   * Call open() before setting an onload listener.
>-   *
>-   * Mozilla only.
>-   */
>-  attribute nsIDOMEventListener onload;
>-
>-  /**
>-   * Meant to be a script-only mechanism for setting an error event listener.
>-   * The attribute is expected to be JavaScript function object. When
>-   * the error event occurs, the function is invoked.
>-   * This attribute should not be used from native code!!
>-   *
>-   * After the initial response, all event listeners will be cleared.
>-   * // XXXbz what does that mean, exactly?   
>-   *
>-   * Call open() before setting an onerror listener.
>-   *
>-   * Mozilla only.
>-   */
>-  attribute nsIDOMEventListener onerror;
>-
>-  /**
>-   * Meant to be a script-only mechanism for setting a progress event listener.
>-   * The attribute is expected to be JavaScript function object. When
>-   * the error event occurs, the function is invoked.
>-   * This attribute should not be used from native code!!
>-   * This event listener may be called multiple times during the open request.
>-   *
>-   * After the initial response, all event listeners will be cleared.
>-   * // XXXbz what does that mean, exactly?
>-   *
>-   * This event listener must be set BEFORE calling open().
>-   *
>-   * Mozilla only.
>-   */
>-  attribute nsIDOMEventListener onprogress;
>-
>-  /**
>-   * Meant to be a script-only mechanism for setting an upload progress event
>-   * listener.
>-   * This attribute should not be used from native code!!
>-   * This event listener may be called multiple times during the upload..
>-   *
>-   * After the initial response, all event listeners will be cleared.
>-   * // XXXbz what does that mean, exactly?
>-   *
>-   * This event listener must be set BEFORE calling open().
>-   *
>-   * Mozilla only.
>-   */
>-  attribute nsIDOMEventListener onuploadprogress;
> 
>   /**
>    * Meant to be a script-only mechanism for setting a callback function.
>@@ -421,6 +354,27 @@ interface nsIJSXMLHttpRequest : nsISuppo
>   attribute nsIDOMEventListener onreadystatechange;
> };
> 
>+/**
>+ * DEPRECATED.
>+ */
>+[scriptable, uuid(261676b4-d508-43bf-b099-74635a0ee2e9)]
>+interface nsIJSXMLHttpRequest : nsISupports {
>+  /**
>+   * Meant to be a script-only mechanism for setting an upload progress event
>+   * listener.
>+   * This attribute should not be used from native code!!
>+   * This event listener may be called multiple times during the upload..
>+   *
>+   * After the initial response, all event listeners will be cleared.
>+   * // XXXbz what does that mean, exactly?
>+   *
>+   * This event listener must be set BEFORE calling open().
>+   *
>+   * Mozilla only.
>+   */
>+  attribute nsIDOMEventListener onuploadprogress;
>+};
>+
> %{ C++
> #define NS_XMLHTTPREQUEST_CID                       \
>  { /* d164e770-4157-11d4-9a42-000064657374 */       \
>diff --git a/content/base/src/nsXMLHttpRequest.cpp b/content/base/src/nsXMLHttpRequest.cpp
>--- a/content/base/src/nsXMLHttpRequest.cpp
>+++ b/content/base/src/nsXMLHttpRequest.cpp
>@@ -611,8 +611,9 @@ nsXMLHttpRequestUpload::GetContextForEve
> /////////////////////////////////////////////
> 
> nsXMLHttpRequest::nsXMLHttpRequest()
>-  : mState(XML_HTTP_REQUEST_UNINITIALIZED), mUploadTransferred(0),
>-    mUploadTotal(0), mUploadComplete(PR_TRUE), mErrorLoad(PR_FALSE)
>+  : mRequestObserver(nsnull), mState(XML_HTTP_REQUEST_UNINITIALIZED),
>+    mUploadTransferred(0), mUploadTotal(0), mUploadComplete(PR_TRUE),
>+    mErrorLoad(PR_FALSE), mFirstStartRequestSeen(PR_FALSE)
> {
>   nsLayoutStatics::AddRef();
> }
>@@ -727,6 +728,12 @@ nsXMLHttpRequest::Initialize(nsISupports
>   return NS_OK; 
> }
> 
>+void
>+nsXMLHttpRequest::SetRequestObserver(nsIRequestObserver* aObserver)
>+{
>+  mRequestObserver = aObserver;
>+}
>+
> NS_IMPL_CYCLE_COLLECTION_CLASS(nsXMLHttpRequest)
> 
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsXMLHttpRequest,
>@@ -776,7 +783,6 @@ NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(nsXMLHttpRequest)
>   NS_INTERFACE_MAP_ENTRY(nsIXMLHttpRequest)
>   NS_INTERFACE_MAP_ENTRY(nsIJSXMLHttpRequest)
>-  NS_INTERFACE_MAP_ENTRY(nsIXMLHttpRequestUploadGetter)
>   NS_INTERFACE_MAP_ENTRY(nsIDOMLoadListener)
>   NS_INTERFACE_MAP_ENTRY(nsIDOMEventListener)
>   NS_INTERFACE_MAP_ENTRY(nsIRequestObserver)
>@@ -1565,6 +1571,11 @@ NS_IMETHODIMP
> NS_IMETHODIMP
> nsXMLHttpRequest::OnStartRequest(nsIRequest *request, nsISupports *ctxt)
> {
>+  if (!mFirstStartRequestSeen && mRequestObserver) {
>+    mFirstStartRequestSeen = PR_TRUE;
>+    mRequestObserver->OnStartRequest(request, ctxt);
>+  }
>+
>   if (!IsSameOrBaseChannel(request, mChannel)) {
>     return NS_OK;
>   }
>@@ -1784,6 +1795,12 @@ nsXMLHttpRequest::OnStopRequest(nsIReque
>   }
> 
>   mState &= ~XML_HTTP_REQUEST_SYNCLOOPING;
>+
>+  if (mRequestObserver && mState & XML_HTTP_REQUEST_GOT_FINAL_STOP) {
>+    NS_ASSERTION(mFirstStartRequestSeen, "Inconsistent state!");
>+    mFirstStartRequestSeen = PR_FALSE;
>+    mRequestObserver->OnStopRequest(request, ctxt, status);
>+  }

What will happen if we never end up with mState & XML_HTTP_REQUEST_GOT_FINAL_STOP? Will something terrible happen? As I recall it this could sometimes happen with malformed multipart requests.

Feel free to just file a bug on making this more robust such that we for sure always shut down things properly, we need to do that anyway for user friendlyness reasons.

(In general, make sure to test multipart requests inside the worker)

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 338759 [details] [diff] [review]
Patch, v1

- In nsGlobalWindow::FreeInnerObjects():

   nsDOMThreadService* dts = nsDOMThreadService::get();
   if (dts) {
+    nsIScriptContext *scx = GetContextInternal();
+
+    JSContext *cx = scx ? (JSContext *)scx->GetNativeContext() : nsnull;
+
+    JSAutoSuspendRequest asr(cx);

Add a comment explaining why this suspend request is needed.

- In nsDOMWorkerPool::nsDOMWorkerPool():

   JSContext* cx = nsContentUtils::GetCurrentJSContext();
   NS_ENSURE_TRUE(cx, NS_ERROR_UNEXPECTED);
 
-  nsIScriptContext* scriptContext = GetScriptContextFromJSContext(cx);
-  NS_ENSURE_STATE(scriptContext);
+  mScriptContext = GetScriptContextFromJSContext(cx);
+  NS_ENSURE_STATE(mScriptContext);

This should get the context out of the document rather than the context we're currently running on, in which case you might not even need to cache it here as you can get to it from the document at any point (well, until the document is torn down etc).

- In nsDOMWorkerThread::CancelXHRs():

+  nsAutoTArray<nsDOMWorkerXHR*, 20> xhrs;
+
+  // Must call Cancel outside the lock!
+  {
+    nsAutoLock lock(mLock);
+    xhrs.AppendElements(mXHRs);
+  }
+
+  PRUint32 xhrCount = xhrs.Length();
+  for (PRUint32 index = 0; index < xhrCount; index++) {
+    xhrs[index]->Cancel();
+  }

This races with NewXMLHttpRequest and the xhr contructor which adds itself to mXHRs.

class nsErrorReportingRunnable : public nsRunnable

New name maybe? This isn't actually reporting an error... Not sure what a better name would be tho...

And maybe the response runnable isn't even needed, as it could just set mResult and mDone once it has the result, as long as the waiting thread can't get stuck waiting for a message?

+class nsDOMWorkerXHREvent : public nsRunnable,
+                            public nsIDOMEvent,
+                            public nsIClassInfo

File a followup bug on exposing the other types of events and their data as well.

- In nsDOMWorkerXHREvent::GetTarget():

+  return CallQueryInterface(mXHRProxy->mWorkerXHR, aTarget);

Here, and all other places where script can call into a stored event, we need to not dereference mXHRProxy as it could've already been nulled out.

- In nsDOMWorkerXHRProxy::InitInternal():

+  // Call QI manually here to avoid keeping up with the cast madness of
+  // nsXMLHttpRequest.
+  nsCOMPtr<nsIXMLHttpRequest> xhr;
+  rv = xhrConcrete->QueryInterface(NS_GET_IID(nsIXMLHttpRequest),
+                                   getter_AddRefs(xhr));
+  NS_ENSURE_SUCCESS(rv, rv);

You should be able to simply cast to nsIXMLHttpRequest here and use the nsCOMPtr helpers, safely...

+  // We now own upload.
+  upload.swap(mUpload);
+
+  mXHR = xhr;

mXHR is weak, so maybe document above that the upload object owns the XHR object.

- In nsDOMWorkerXHRProxy::AddEventListener():

+    // Silently fail on juk events.

Typo, copied a couple of times :)

- In nsDOMWorkerXHRProxy::OnStartRequest():

+  NS_ASSERTION(!mOwnedByXHR, "Inconsistent state!");
+
+  FlipOwnership();

This could race with the flip call in Abort() on the worker, right?
Same thing in nsDOMWorkerXHRProxy::OnStopRequest().

r- based on that, but over all this looks really good, even if non-trivial :)

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

Comments addressed.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Differences between v1 and v2

Here's the interdiff. It looks long but it's mainly because I sprinkled cancel checks all over the place to kill the XHR as soon as possible.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 339531 [details] [diff] [review]
Patch, v2

So you're changing nsIXMLHttpRequest interface without updating its uuid.
Is that on purpose? I guess the vtable is still compatible. Though, because
there are new things on the interface, the caller can't know if those are available.
When adding support for .upload I explicitly decided to not to change nsIXMLHttpRequest, 
because that is probably used in quite many extensions (also binary).

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 339531 [details] [diff] [review]
Patch, v2

nsDOMWorkerXHREventTarget::DispatchEvent():

+  nsRefPtr<nsDOMWorkerXHREventWrapper> wrapper =
+    new nsDOMWorkerXHREventWrapper(aEvent);

Do we need to worry about the case where the event already is an nsDOMWorkerXHREventWrapper? If not, script could theoretically create indefinitely long chains of these things and slow things down considerably, maybe?

r+sr=jst with that considered.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v3

This applies on top of attachment 339531 [details] [diff] [review] and addresses the event re-wrap.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Final patch for checkin

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #7)
> So you're changing nsIXMLHttpRequest interface without updating its uuid.

Oops, I'll bump that on checkin, it was not intentional. Discussed the modification of nsIXMLHttpRequest with jst and sicking and they both voted to change it rather than making a new interface.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Additional change needed for mochitest failure

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Final patch for checkin

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Changes to account for bug 382871

Slight changes were necessary after bug 382871 landed.

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed.