Closed
Bug 1071290
Opened 10 years ago
Closed 10 years ago
Blob as one of the types in a Union crashes in workers
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: nsm, Assigned: nsm)
References
Details
Attachments
(1 file)
13.24 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
The fetch spec's Request's body can be a blob [1]. Our codegen generates unwrapping code for main thread, but Blob has special handling on the worker to get a Blob from JS::Value, leading to crashes when a Blob is passed as the body. [1]: https://fetch.spec.whatwg.org/#bodyinit
Comment 1•10 years ago
|
||
See bug 809899. Sadly, it seems we'll compile stuff now, just incorrect stuff. I wonder whether we can do better here... For the case of Blob, the right answer is to land bug 1047483, right?
Depends on: 1047483
Comment 2•10 years ago
|
||
Maybe what we can do is have union ctors assert mainthread if the union contains any interface type which has different worker and non-worker descriptors? That would at least catch the issue early in debug builds...
Assignee | ||
Updated•10 years ago
|
Blocks: dom-fetch-api
Assignee | ||
Comment 3•10 years ago
|
||
Just noting that this affects the Response object as well.
Comment 4•10 years ago
|
||
I think this bug is fixed by bug 1047483. Can you try again?
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Comment 5•10 years ago
|
||
Thanks! That works.
Attachment #8502753 -
Flags: review?(amarchesini)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(nsm.nikhil)
Comment 6•10 years ago
|
||
Comment on attachment 8502753 [details] [diff] [review] Allow Blobs in Fetch BodyInit Review of attachment 8502753 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/fetch/Fetch.cpp @@ +114,5 @@ > } > } > > nsresult > +ExtractByteStreamFromBody(const OwningArrayBufferOrArrayBufferViewOrBlobOrScalarValueStringOrURLSearchParams& aBodyInit, wow. This is a really cool type name. @@ +122,5 @@ > MOZ_ASSERT(aStream); > > if (aBodyInit.IsArrayBuffer()) { > const ArrayBuffer& buf = aBodyInit.GetAsArrayBuffer(); > return ExtractFromArrayBuffer(buf, aStream); So actually, just because you are returning value, I think we should have this: if (aBodyInit.IsArrayBuffer()) { ... return ... } if ( ... ) { return ... } @@ +157,5 @@ > const ArrayBufferView& buf = aBodyInit.GetAsArrayBufferView(); > return ExtractFromArrayBufferView(buf, aStream); > + } else if (aBodyInit.IsBlob()) { > + const File& blob = aBodyInit.GetAsBlob(); > + return ExtractFromBlob(blob, aStream, aContentType); save comment here.
Attachment #8502753 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 7•10 years ago
|
||
I actually can't land this until the Response + Split InternalHeaders patch is reviewed :)
Assignee | ||
Comment 8•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/caa56adec4f5 Andrea, I forgot to make the else-if to if change you suggested :/ Will there be performance savings in converting? If so, i can push a followup
Flags: needinfo?(amarchesini)
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/caa56adec4f5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 10•10 years ago
|
||
> I forgot to make the else-if to if change you suggested :/ Will there be
> performance savings in converting? If so, i can push a followup
This code is changing quickly. Let's do it next time we touch this code.
Flags: needinfo?(amarchesini)
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•