1210330 - non-e10s TCPSocket "data" event is a Uint8Array, should be an ArrayBuffer (WebIDL-ification regression)

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

In TCPSocket::OnDataAvailable, the implementation diverges:

- If mSocketBridgeParent (e10s), we FireArrayBufferDataEvent.  After bridging This eventually results in a call to TCPSocket::FireDataArrayEvent where we use DeserializeArrayBuffer at https://dxr.mozilla.org/mozilla-central/source/dom/network/TCPSocket.cpp?offset=100#514
  bool ok = IPC::DeserializeArrayBuffer(cx, buffer, &val);
  if (ok) {
    return FireDataEvent(cx, aType, val);
  }

- But if we're not e10s, then we do https://dxr.mozilla.org/mozilla-central/source/dom/network/TCPSocket.cpp?offset=0#1043
    if (!ToJSValue(cx, TypedArrayCreator<Uint8Array>(buffer), &value)) {
      return NS_ERROR_FAILURE;
    }
    FireDataEvent(cx, NS_LITERAL_STRING("data"), value);

I believe the unit tests don't care because in the ondata implementation we immediately coerce to a Uint8array.  We probably want to add a line like so to the top of the ondata implementation in the test:
  ok(event.data instanceof ArrayBuffer, 'payload is ArrayBuffer');


I encountered this because I was being a luddite using (desktop) nightly under non-e10s and my postMessage transferable stuff broke.  (Via the very opaque DataCloneError.  Grr!)  FxOS email hasn't cared yet because the devices are obviously e10s and the automated tests using b2g-desktop don't run by default for reasons we won't go into.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Use the ArrayBuffer deserializer, have a test that fails without the fix on non-e10s, passes with it v1

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Use the ArrayBuffer deserializer, have a test that fails without the fix on non-e10s, passes with it v1

(bad mq usage resulted in an empty patch)

Comment 3

[Josh Matthews [:jdm]]

Comment on attachment 8673404 [details] [diff] [review]
Use the ArrayBuffer deserializer, have a test that fails without the fix on non-e10s, passes with it v1

Does it work if we do TypedArrayCreator<ArrayBuffer>? Calling DeserializeArrayBuffer looks kind of weird here.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Change the typed array type from Uint8Array to ArrayBuffer, with test.

(In reply to Josh Matthews [:jdm] from comment #3)
> Comment on attachment 8673404 [details] [diff] [review]
> Does it work if we do TypedArrayCreator<ArrayBuffer>? Calling
> DeserializeArrayBuffer looks kind of weird here.

Yes, in addition to making sense, it also works.  Carrying forward r=jdm and will land shortly.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81b6dbef4fb155079a9f250272a323f67319dc20
Bug 1210330 - TCPSocket data event should be an ArrayBuffer in non-e10s case too. r=jdm

Comment 6

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e7dbcd77219c - turns out there's a mochitest-chrome test called test_tcpsocket_jsm.html which would really prefer you not do that on Android and b2g, https://treeherder.mozilla.org/logviewer.html#?job_id=15590577&repo=mozilla-inbound

Comment 7

[Phil Ringnalda (:philor)]

Oh, busted on desktop too, it just takes them longer to run mochitest-chrome since it's in mochitest-other with other crap.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Apologies about the bustage.  The instanceof check of course fails for TCPSocket instances created in a JSM since it will have logically distinct globals from a window context, even with B2G's JSM shenanigans.  I didn't pay enough attention to the JSM test path recently added in bug 1207090 and definitely played it too fast and loose with testing before landing.  I have a fix and will push full try and ask for re-review since the method I'm using for delegating the instanceof check may be too ugly.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Attachment: hey, look, you can tell ./mach mochitest a whole directory of tests to run!

Try is at:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d95fea3cc097
(And because I only do extremes, it's a very thorough try run!)

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14bbbb348f21b830faf0211a339688277fdc3e74
Bug 1210330 - TCPSocket data event should be an ArrayBuffer in non-e10s case too. r=jdm

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14bbbb348f21

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Since the original WebIDL conversion landed in 43, could we uplift this fix too, so there is a consistent API across releases?

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> Since the original WebIDL conversion landed in 43, could we uplift this fix
> too, so there is a consistent API across releases?

Yes.  This bug's test depends on bug 1207090 being there too, but it's already there, so feel free to request uplift.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8673457 [details] [diff] [review]
hey, look, you can tell ./mach mochitest a whole directory of tests to run!

Approval Request Comment
[Feature/regressing bug #]: Bug 889582 converted TCPSocket to WebIDL
[User impact if declined]: If declined, there will be a different API in 43 vs. 44, increasing the maintenance burden for add-on authors and other users of the API.
[Describe test coverage new/current, TreeHerder]: Test added on m-c.
[Risks and why]: Seems low, repairing an API regression.
[String/UUID change made/needed]: None

Comment 15

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

I can't tell what happened to cause the regression in bug 889582 (which is a duplicate of a bug marked RESOLVED INVALID). Is that the right bug number? 
Tracking since this is a regression in 43+.

Comment 16

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8673457 [details] [diff] [review]
hey, look, you can tell ./mach mochitest a whole directory of tests to run!

Fix for a regression in 43; includes tests. OK to uplift to aurora.

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> I can't tell what happened to cause the regression in bug 889582 (which is a
> duplicate of a bug marked RESOLVED INVALID). Is that the right bug number? 
> Tracking since this is a regression in 43+.

Sorry, it's meant to be 885982.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7cbf57fae6c5