Closed Bug 1210330 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
e10s - ---
firefox43 + fixed
firefox44 + fixed

People

(Reporter: asuth, Assigned: asuth)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
(bad mq usage resulted in an empty patch)
Attachment #8673403 - Attachment is obsolete: true
Attachment #8673403 - Flags: review?(josh)
Attachment #8673404 - Flags: review?(josh)
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.
Attachment #8673404 - Flags: review?(josh) → review+
(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.
Attachment #8673404 - Attachment is obsolete: true
Attachment #8673422 - Flags: review+
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
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
Oh, busted on desktop too, it just takes them longer to run mochitest-chrome since it's in mochitest-other with other crap.
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.
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!)
Attachment #8673422 - Attachment is obsolete: true
Attachment #8673457 - Flags: review?(josh)
Attachment #8673457 - Flags: review?(josh) → review+
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
https://hg.mozilla.org/mozilla-central/rev/14bbbb348f21
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Since the original WebIDL conversion landed in 43, could we uplift this fix too, so there is a consistent API across releases?
Blocks: 889582
Flags: needinfo?(bugmail)
(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.
Flags: needinfo?(bugmail)
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
Attachment #8673457 - Flags: approval-mozilla-aurora?
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+.
Flags: needinfo?(jryans)
Keywords: regression
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.
Attachment #8673457 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Blocks: 885982
No longer blocks: 889582
Flags: needinfo?(jryans)
You need to log in before you can comment on or make changes to this bug.