Array buffers should always use transfer when been cloned from main thread to worker XHR.

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: swu, Assigned: swu)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

Per discussion with smaug on IRC regarding bug 945152, not only mapped array buffer, but also normal array buffers should be transfered instead of copied when been cloned from main thread to worker XHR.  Because nothing in main thread should access the data for the worker XHR.
bent, could you review this change in worker XHR?
Assignee: nobody → swu
Attachment #8420041 - Flags: review?(bent.mozilla)
Depends on: 1008092
Comment on attachment 8420041 [details] [diff] [review]
Patch: Alway transfer for array buffers when cloning.

Review of attachment 8420041 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing the review.

::: dom/workers/XMLHttpRequest.cpp
@@ +1177,5 @@
> +        if (obj && JS_IsArrayBufferObject(obj)) {
> +          // Discard the response if the object was neutered.
> +          if (JS_IsNeuteredArrayBufferObject(obj)) {
> +            return false;
> +          }

Why would the response be neutered at this point?  Aren't we the first thing to read it?
Attachment #8420041 - Flags: review?(bent.mozilla) → review?(khuey)
Comment on attachment 8420041 [details] [diff] [review]
Patch: Alway transfer for array buffers when cloning.

Review of attachment 8420041 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks mostly right, but there are a few problems:

::: dom/workers/XMLHttpRequest.cpp
@@ +1171,5 @@
>            WorkerStructuredCloneCallbacks(true);
>  
>          nsTArray<nsCOMPtr<nsISupports> > clonedObjects;
>  
> +        JS::RootedValue transferable(aCx, JSVAL_NULL);

Nit: no need for JSVAL_NULL here I hope.

@@ +1175,5 @@
> +        JS::RootedValue transferable(aCx, JSVAL_NULL);
> +        JS::RootedObject obj(aCx, response.toObjectOrNull());
> +        if (obj && JS_IsArrayBufferObject(obj)) {
> +          // Discard the response if the object was neutered.
> +          if (JS_IsNeuteredArrayBufferObject(obj)) {

How is it possible to have a newly created array buffer that is also neutered? I think you should just be able to assert that this is always false.

@@ +1176,5 @@
> +        JS::RootedObject obj(aCx, response.toObjectOrNull());
> +        if (obj && JS_IsArrayBufferObject(obj)) {
> +          // Discard the response if the object was neutered.
> +          if (JS_IsNeuteredArrayBufferObject(obj)) {
> +            return false;

Returning false here is not correct, this won't send anything to the worker. If you can't assert that this case is impossible (see above) then you probably want to set mResponseResult to some suitable error code here and then return true.

@@ +1180,5 @@
> +            return false;
> +          }
> +          JS::AutoValueArray<1> argv(aCx);
> +          argv[0].set(response);
> +          transferable = OBJECT_TO_JSVAL(JS_NewArrayObject(aCx,

You're not handling the case where JS_NewArrayObject could fail here. You'll need to set mResponseResult to a suitable error code.

Also, you should just use |transferable.setObject()| rather than |OBJECT_TO_JSVAL|.
Attachment #8420041 - Flags: review?(khuey) → review-
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> Comment on attachment 8420041 [details] [diff] [review]
> Patch: Alway transfer for array buffers when cloning.
> 
> Review of attachment 8420041 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Stealing the review.
> 
> ::: dom/workers/XMLHttpRequest.cpp
> @@ +1177,5 @@
> > +        if (obj && JS_IsArrayBufferObject(obj)) {
> > +          // Discard the response if the object was neutered.
> > +          if (JS_IsNeuteredArrayBufferObject(obj)) {
> > +            return false;
> > +          }
> 
> Why would the response be neutered at this point?  Aren't we the first thing
> to read it?

There is one thing I noticed when cloning mapped array buffer by transfer, it was mentioned earlier in bug 945152 comment 181, and I am not sure whether it's a bug or not. 

When JS code making xhr request in worker, once the response ready, there will be totally 3 DispatchDOMEvent() sent from main thread to worker XHR.

Starting at nsXMLHttpRequest::ChangeStateToDone(), totally 3 events were sent.

The 1st event is sent by ChangeState() at
http://hg.mozilla.org/mozilla-central/file/a8602e656d86/content/base/src/nsXMLHttpRequest.cpp#l2219

The 2nd event sent by DispatchProgressEvent() at
http://hg.mozilla.org/mozilla-central/file/a8602e656d86/content/base/src/nsXMLHttpRequest.cpp#l2226

The 3rd event sent by DispatchProgressEvent() again at
http://hg.mozilla.org/mozilla-central/file/a8602e656d86/content/base/src/nsXMLHttpRequest.cpp#l1450

Each event triggers a new cloning in worker XHR.  When cloning by copy, it's not a big problem(though additional memory is allocated).  But when cloning by transfer, the 1st transfer will be successful, but the 2nd and 3rd transfer would fail because the array buffer from main thread already been neutered at the 1st transfer.  Therefore, only the 1st event produces a valid response.  When testing with mapped array buffer, it's ok for async XHR(because the first response is got), but sync XHR would fail.  I guess the result will be the same for normal array buffer.

That's the reason we have to check for neutered and return false here because we don't want invalid responses been delivered back.
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #3)
> @@ +1175,5 @@
> > +        JS::RootedValue transferable(aCx, JSVAL_NULL);
> > +        JS::RootedObject obj(aCx, response.toObjectOrNull());
> > +        if (obj && JS_IsArrayBufferObject(obj)) {
> > +          // Discard the response if the object was neutered.
> > +          if (JS_IsNeuteredArrayBufferObject(obj)) {
> 
> How is it possible to have a newly created array buffer that is also
> neutered? I think you should just be able to assert that this is always
> false.

As explained in comment 4.

> 
> @@ +1176,5 @@
> > +        JS::RootedObject obj(aCx, response.toObjectOrNull());
> > +        if (obj && JS_IsArrayBufferObject(obj)) {
> > +          // Discard the response if the object was neutered.
> > +          if (JS_IsNeuteredArrayBufferObject(obj)) {
> > +            return false;
> 
> Returning false here is not correct, this won't send anything to the worker.
> If you can't assert that this case is impossible (see above) then you
> probably want to set mResponseResult to some suitable error code here and
> then return true.

As explained in comment 4.  We silently discard it here instead of returning true, because redundant invalid responses would make sync XHR fail.
Oh... Right. This is not going to work.

The problem is that XHR generates lots of events, and at any time script could handle an event and ask for the response property on the worker thread. We don't know when (or if) that will happen so we have to grab the response property every time an event is generated on the main thread so that we can update the worker thread's value.
Ah, right.  It feels like we should be able to do something smarter here (because the ArrayBuffer is never read on the main thread) but just using the structured clone transferable mechanism is not going to work.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
Blocks: 1008512
If we can avoid sending more than one event when state changed to XML_HTTP_REQUEST_DONE, then it is possible by doing transfer only in this final state.
(In reply to Shian-Yow Wu [:shianyow] from comment #8)
> If we can avoid sending more than one event when state changed to
> XML_HTTP_REQUEST_DONE, then it is possible by doing transfer only in this
> final state.

We have to send all events unfortunately.
array buffer response is null until the request is DONE.
http://xhr.spec.whatwg.org/#the-response-attribute

So as far as I see we can transfer the array at that point.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
It seems we can transfer array buffer when readyState is DONE and status is 200.

How about the proposal below?

When receiving response which readyState is DONE,
1. If status != 200, copy array buffer response to worker.
  Note: If this was happened after a state==200 response, a neutered array buffer will be sent.
2. If status == 200,
  2-1. For the first state==200 resonse, transfer array buffer response to worker. 
  2-2. For further state==200 responses, don't send response to worker.
What further 200 responses? If we are in DONE state, we can transfer the array and store it on the
worker side.
Is someone then reuses XHR and calls open/send again, we aren't in DONE state anymore so the
cache should be cleared and once the new request is done, we transfer the new array.
(In reply to Olli Pettay [:smaug] from comment #12)
> What further 200 responses? If we are in DONE state, we can transfer the
> array and store it on the
> worker side.

For same XHR request, one response received at main thread, but three response events received at the worker side, all with readyState=DONE and status=200.  We should ignore the redundant responses.
The first time main thread XHR has state DONE we transfer array to worker, and cache it there.
So, ok, that is what "2-2. For further state==200 responses, don't send response to worker." is about :)
Hi bent, could you feedback whether this is going to work? :)

The WIP patch implemented what has been discussed since comment 10.  It uses previously stored response on worker side if array buffer already been transfered.

TODO:
1. Reuse XHR case.
Attachment #8420041 - Attachment is obsolete: true
Attachment #8423069 - Flags: feedback?(bent.mozilla)
Posted patch Patch: Test XHR reuse. (obsolete) — Splinter Review
Test case for XHR reuse.
Depends on: 1010784
Comment on attachment 8423069 [details] [diff] [review]
(WIP) Patch v2: Alway transfer for array buffers when cloning.

Review of attachment 8423069 [details] [diff] [review]:
-----------------------------------------------------------------

In general this looks like the right idea but I wouldn't worry with the readystate/status checks. You need to handle the chunked arraybuffer case too, so the mechanism needs to be a little more flexible. I also wouldn't call any additional DOM methods on the worker side.

You probably want to change XMLHttpRequest::UpdateState to take a bool 'reusePreviousResponse' flag or something. Then save the previous response on the stack before setting the new data.

On the main thread you need some more serious assertions to make sure that the value you're sending never changes after you send it the first time.
Attachment #8423069 - Flags: feedback?(bent.mozilla) → feedback+
Added reusing async XHR.
Attachment #8423075 - Attachment is obsolete: true
Depends on: 1014466
Attachment #8423069 - Attachment is obsolete: true
Posted patch Part 2: Test cases. (obsolete) — Splinter Review
Test cases for:
1. Chunked array buffer.
2. Sync/async xhr and reuse.
Attachment #8426872 - Attachment is obsolete: true
Attachment #8429855 - Flags: review?(bent.mozilla)
Comment on attachment 8429153 [details] [diff] [review]
Part 1(v3): Always transfer array buffers when cloning.

Previous comments addressed and root cause of dependent bugs found.
Now it's ready for review.
Attachment #8429153 - Attachment description: Patch v3: Alway transfer for array buffers when cloning. → Part 1(v3): Always transfer array buffers when cloning.
Attachment #8429153 - Flags: review?(bent.mozilla)
Posted patch Part 2: Test cases. (obsolete) — Splinter Review
Attachment #8429855 - Attachment is obsolete: true
Attachment #8429855 - Flags: review?(bent.mozilla)
Attachment #8430018 - Flags: review?(bent.mozilla)
Comment on attachment 8429153 [details] [diff] [review]
Part 1(v3): Always transfer array buffers when cloning.

Review of attachment 8429153 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/XMLHttpRequest.cpp
@@ +87,5 @@
>    // Read on multiple threads.
>    WorkerPrivate* mWorkerPrivate;
>    XMLHttpRequest* mXMLHttpRequestPrivate;
> +  bool mIsTransferring;
> +  bool mUseCachedResponse;

It looks like these flags are used on both threads currently and I'd prefer we separate them a bit. I think we can try the following:

|mIsTransfering| should only ever be touched on the main thread and should only be set to true after we've transfered a non-neutered array buffer, right? You would then be able to assert that any other arraybuffers that you receive later have been neutered.

|mUseCachedResponse| would actually be better as a flag on the EventRunnable object. You would set it when you're about to send an arraybuffer that has already been neutered.

Does that make sense?

@@ +1169,5 @@
>          mResponse = response;
>        }
>        else {
> +        bool doClone = true;
> +        JS::RootedValue transferable(aCx, JS::UndefinedValue());

Nit: I think we decided a while ago not to use |JS::RootedValue| or |JS::RootedObject| typedefs but instead to use explicit |JS::Rooted<JS::Value>| and |JS::Rooted<JSObject*>| types.

@@ +1170,5 @@
>        }
>        else {
> +        bool doClone = true;
> +        JS::RootedValue transferable(aCx, JS::UndefinedValue());
> +        JS::RootedObject obj(aCx, response.toObjectOrNull());

I don't think you can call |toObjectOrNull()| on anything that does not return true for |isObjectOrNull()|. Otherwise it will assert. So you have to do:

  JS::RootedObject obj(aCx,
    response.isObjectOrNull() ? response.toObjectOrNull() : nullptr);

@@ +2325,5 @@
> +    JSContext* cx = mWorkerPrivate->GetJSContext();
> +    JS::Rooted<JS::Value> response(cx);
> +    ErrorResult rv;
> +    // Save previous response from mStateData.
> +    response = GetResponse(cx, rv);

So let's definitely not call DOM methods in this case. The value you want is already stored in mStateData here, just save it on the stack before you replace mStateData with aStateData and then set it back.
Attachment #8429153 - Flags: review?(bent.mozilla) → review-
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #23)
> It looks like these flags are used on both threads currently and I'd prefer
> we separate them a bit. I think we can try the following:
> 
> |mIsTransfering| should only ever be touched on the main thread and should
> only be set to true after we've transfered a non-neutered array buffer,
> right? You would then be able to assert that any other arraybuffers that you
> receive later have been neutered.
> 
> |mUseCachedResponse| would actually be better as a flag on the EventRunnable
> object. You would set it when you're about to send an arraybuffer that has
> already been neutered.
> 
> Does that make sense?

If we put mUseCachedResponse on EventRunnable object, then it can shared by main thread and worker thread, right?  We need to have one place to set(i.e. main thread) mUseCachedResponse and the other to read(i.e. worker thread) the flag.

So it will be like this: check the mIsTransferred flag in main thread, if true, then assert that arraybuffer has been neutered and set mUseCachedResponse to be true.  In worker thread, check the mUseCachedResponse flag to determine whether to use cached response.
Thanks for previous comments, the patch should have addressed them and please review again.
Attachment #8429153 - Attachment is obsolete: true
Attachment #8430738 - Flags: review?(bent.mozilla)
Comment on attachment 8430738 [details] [diff] [review]
Part 1(v4): Always transfer array buffers when cloning.

Review of attachment 8430738 [details] [diff] [review]:
-----------------------------------------------------------------

This looks nearly perfect, thanks! I have a few more suggestions before I think this is done. Please address these and then we should be good to go.

::: dom/workers/XMLHttpRequest.cpp
@@ +117,5 @@
>    // Only touched on the main thread.
>    bool mUploadEventListenersAttached;
>    bool mMainThreadSeenLoadStart;
>    bool mInOpen;
> +  bool mIsTransferred;

Nit: Let's name this more specifically... Something like 'mArrayBufferResponseWasTransferred'... It's long...

@@ +423,5 @@
>    uint16_t mReadyState;
>    bool mUploadEvent;
>    bool mProgressEvent;
>    bool mLengthComputable;
> +  bool mUseCachedResponse;

Nit: And let's call this 'mUseCachedArrayBufferResponse'

@@ +1173,5 @@
>          mResponse = response;
>        }
>        else {
> +        bool doClone = true;
> +        JS::Rooted<JS::Value> transferable(aCx, JS::UndefinedValue());

Nit: The second arg is unnecessary here, it defaults to undefined.

@@ +1183,5 @@
> +            MOZ_ASSERT(JS_IsNeuteredArrayBufferObject(obj));
> +            mUseCachedResponse = true;
> +            doClone = false;
> +          }
> +          else {

Nit: go ahead and cuddle this |} else {|

@@ +1187,5 @@
> +          else {
> +            JS::AutoValueArray<1> argv(aCx);
> +            argv[0].set(response);
> +            obj = JS_NewArrayObject(aCx, argv);
> +            if (!obj) {

Nit: Since you're checking both cases (obj and !obj) it makes for easier reading to do the obj check rather than !obj. Please reverse the check and order of the blocks.

@@ +1199,2 @@
>          }
> +        if (doClone) {

Nit: Let's add an extra line before this if.

@@ +1820,5 @@
>    mWorkerPrivate->AssertIsOnWorkerThread();
>  
>    bool hasUploadListeners = mUpload ? mUpload->HasListeners() : false;
>  
> +  mProxy->mIsTransferred = false;

This shouldn't be set here but rather in SendRunnable::MainThreadRun. That way it is only ever touched on the main thread.

@@ +2340,4 @@
>  {
> +  if (aUseCachedResponse) {
> +    JS::Rooted<JS::Value> response(mWorkerPrivate->GetJSContext(),
> +                                   mStateData.mResponse);

Here can you assert that mStateData.mResponse is an arraybuffer?

@@ +2341,5 @@
> +  if (aUseCachedResponse) {
> +    JS::Rooted<JS::Value> response(mWorkerPrivate->GetJSContext(),
> +                                   mStateData.mResponse);
> +    mStateData = aStateData;
> +    if(!response.isPrimitive()) {

Hm, I don't think this check is worth doing. You should just always assign back if aUseCachedResponse is true.
Attachment #8430738 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8430018 [details] [diff] [review]
Part 2: Test cases.

Review of attachment 8430018 [details] [diff] [review]:
-----------------------------------------------------------------

These tests look ok but I don't see any tests around the crazy transferred/neutered behavior with multiple events.

Ideally I'd want to see an additional test that checks xhr.response is valid and identical during the last three events of a successful load (readystatechange, loadend, and load, i think?).

::: content/base/test/file_bug1008126_worker.js
@@ +1,1 @@
> +var gJar1 = "jar:http://example.org/tests/content/base/test/file_bug945152.jar!/data_1.txt";

This needs a CC license header, see http://mxr.mozilla.org/mozilla-central/source/dom/workers/test/closeOnGC_worker.js#1 for example.

@@ +34,5 @@
> +
> +  var xhr = new XMLHttpRequest({mozAnon: true, mozSystem: true});
> +
> +  xhr.onerror = function(e) {
> +    dump("Error: " + e.error + "\n");

Shouldn't this be |ok(false, e.error);|? Otherwise you'll never report xhr errors.

@@ +60,5 @@
> +    xhr.send();
> +  }
> +
> +  function test_sync_xhr_data1() {
> +    xhr = new XMLHttpRequest({mozAnon: true, mozSystem: true});

Hm, why do you need to make a new xhr here? You won't have your original error handler installed...

::: content/base/test/test_bug1008126.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

This needs a CC license header, see http://mxr.mozilla.org/mozilla-central/source/dom/workers/test/test_closeOnGC.html?force=1#1 for example.

@@ +31,5 @@
> +  };
> +
> +  worker.onerror = function(event) {
> +    is(event.target, worker);
> +    ok(false, "Worker had an error: " + event.data);

This should be |event.message|, error events don't have a data property.
Attachment #8430018 - Flags: review?(bent.mozilla)
Addressed previous review comments.
Attachment #8430738 - Attachment is obsolete: true
Attachment #8433274 - Flags: review+
The patch addressed previous review comments and added multiple events case.
Attachment #8430018 - Attachment is obsolete: true
Attachment #8433277 - Flags: review?(bent.mozilla)
Comment on attachment 8433277 [details] [diff] [review]
Part 2(v2): Test cases.

Review of attachment 8433277 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with this change:

::: content/base/test/file_bug1008126_worker.js
@@ +85,5 @@
> +      checkData(xhr, gData2, false, runTests);
> +    };
> +    xhr.open("GET", gJar2, true);
> +    xhr.responseType = "arraybuffer";
> +    xhr.send();

This is close, but your test would currently pass if we somehow didn't deliver the right number or type of events. I think it would be better to have this:

  xhr.abort();

  var eventCount = 0;
  xhr.onreadystatechange = function() {
    if (xhr.readyState == xhr.DONE) {
      eventCount++;
      checkData(xhr, gData2, false, function() {});
    }
  };
  xhr.onload = function() {
    eventCount++;
    checkData(xhr, gData2, false, function() {});
  };
  xhr.onloadend = function() {
    eventCount++;
    checkData(xhr, gData2, false, function() {});
  };
  xhr.open("GET", gJar2, true);
  xhr.responseType = "arraybuffer";
  xhr.send();

  is (eventCount, 3, "Saw all expected events");
Attachment #8433277 - Flags: review?(bent.mozilla) → review+
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #30)
> This is close, but your test would currently pass if we somehow didn't
> deliver the right number or type of events. I think it would be better to
> have this:
> 

Thank you for pointing this out.  I will add it and check the 3 event counts separately, to make sure each event was delivered just once, like below.

  var readystatechangeCount = 0;
  var loadCount = 0;
  var loadendCount = 0;

  function checkEventCount() {
    ok(readystatechangeCount == 1 && loadCount == 1 && loadendCount == 1,
       "Saw all expected events");
    runTests();
  }

  function test_multiple_events() {
    ok(true, "Test multiple events");
    xhr.abort();

    xhr.onreadystatechange = function() {
      if (xhr.readyState == xhr.DONE) {
        readystatechangeCount++;
        checkData(xhr, gData2, false, function() {} );
      }   
    };  
    xhr.onload = function() {
      loadCount++;
      checkData(xhr, gData2, false, function() {} );
    };  
    xhr.onloadend = function() {
      loadendCount++;
      checkData(xhr, gData2, false, function() {} );
    };  
    xhr.open("GET", gJar2, false);
    xhr.responseType = "arraybuffer";
    xhr.send();
    checkEventCount();
  }
Duplicate of this bug: 1008512
Tomcat backed this out on suspicion of causing failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0df4d901b8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/52d75fbe0280

...however after looking at retriggers, it turns out the test is broken & this landing just bumped it from one mochitest chunk to another causing it to fail (since that test must rely on previous state). The test will be disabled in bug 1021644, after which this can reland :-)
https://hg.mozilla.org/mozilla-central/rev/5ea529f39d44
https://hg.mozilla.org/mozilla-central/rev/85aa4fe910e7
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1022422
Depends on: 1022607
You need to log in before you can comment on or make changes to this bug.