Closed
Bug 1210330
Opened 5 years ago
Closed 5 years ago
non-e10s TCPSocket "data" event is a Uint8Array, should be an ArrayBuffer (WebIDL-ification regression)
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: asuth, Assigned: asuth)
References
Details
(Keywords: regression)
Attachments
(1 file, 3 obsolete files)
3.96 KB,
patch
|
jdm
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Updated•5 years ago
|
tracking-e10s:
--- → -
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•5 years ago
|
||
Attachment #8673403 -
Flags: review?(josh)
Assignee | ||
Comment 2•5 years ago
|
||
(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 3•5 years ago
|
||
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+
Assignee | ||
Comment 4•5 years ago
|
||
(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+
Assignee | ||
Comment 5•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
Oh, busted on desktop too, it just takes them longer to run mochitest-chrome since it's in mochitest-other with other crap.
Assignee | ||
Comment 8•5 years ago
|
||
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.
Assignee | ||
Comment 9•5 years ago
|
||
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)
Updated•5 years ago
|
Attachment #8673457 -
Flags: review?(josh) → review+
Assignee | ||
Comment 10•5 years ago
|
||
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•5 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/14bbbb348f21
Status: ASSIGNED → RESOLVED
Closed: 5 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: 1215141
Assignee | ||
Comment 13•5 years ago
|
||
(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+.
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•