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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Shian-Yow Wu [:swu]]

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.

Comment 1

[Shian-Yow Wu [:swu]]

Attachment: Patch: Alway transfer for array buffers when cloning.

bent, could you review this change in worker XHR?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 3

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

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

Comment 4

[Shian-Yow Wu [:swu]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 5

[Shian-Yow Wu [:swu]]

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

Comment 6

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

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.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 8

[Shian-Yow Wu [:swu]]

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.

Comment 9

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

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

Comment 10

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

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.

Comment 11

[Shian-Yow Wu [:swu]]

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.

Comment 12

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

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.

Comment 13

[Shian-Yow Wu [:swu]]

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

Comment 14

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

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 :)

Comment 15

[Shian-Yow Wu [:swu]]

Attachment: (WIP) Patch v2: Alway transfer for array buffers when cloning.

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.

Comment 16

[Shian-Yow Wu [:swu]]

Attachment: Patch: Test XHR reuse.

Test case for XHR reuse.

Comment 17

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

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.

Comment 18

[Shian-Yow Wu [:swu]]

Attachment: Patch: Test case for XHR reuse.

Added reusing async XHR.

Comment 19

[Shian-Yow Wu [:swu]]

Attachment: Part 1(v3): Always transfer array buffers when cloning.

Comment 20

[Shian-Yow Wu [:swu]]

Attachment: Part 2: Test cases.

Test cases for:
1. Chunked array buffer.
2. Sync/async xhr and reuse.

Comment 21

[Shian-Yow Wu [:swu]]

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.

Comment 22

[Shian-Yow Wu [:swu]]

Attachment: Part 2: Test cases.

Comment 23

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

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.

Comment 24

[Shian-Yow Wu [:swu]]

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

Comment 25

[Shian-Yow Wu [:swu]]

Attachment: Part 1(v4): Always transfer array buffers when cloning.

Thanks for previous comments, the patch should have addressed them and please review again.

Comment 26

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

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.

Comment 27

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

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.

Comment 28

[Shian-Yow Wu [:swu]]

Attachment: Part 1(v5): Always transfer array buffers when cloning. (r=bent)

Addressed previous review comments.

Comment 29

[Shian-Yow Wu [:swu]]

Attachment: Part 2(v2): Test cases.

The patch addressed previous review comments and added multiple events case.

Comment 30

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

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");

Comment 31

[Shian-Yow Wu [:swu]]

(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();
  }

Comment 32

[Shian-Yow Wu [:swu]]

Thanks for elaborate review from bent, and comments from khuey & smaug.

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=c89aa2b3f06f

https://hg.mozilla.org/integration/mozilla-inbound/rev/63eb4a7d5a3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/04bfd88f4fb8

Comment 34

[Ed Morley [:emorley]]

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 :-)

Comment 35

[Ed Morley [:emorley]]

Relanded:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5ea529f39d44
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/85aa4fe910e7

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5ea529f39d44
https://hg.mozilla.org/mozilla-central/rev/85aa4fe910e7