Closed Bug 1071290 Opened 5 years ago Closed 5 years ago

Blob as one of the types in a Union crashes in workers

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: nsm, Assigned: nsm)

References

Details

Attachments

(1 file)

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
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
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...
Just noting that this affects the Response object as well.
I think this bug is fixed by bug 1047483. Can you try again?
Flags: needinfo?(nsm.nikhil)
Thanks! That works.
Attachment #8502753 - Flags: review?(amarchesini)
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Flags: needinfo?(nsm.nikhil)
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+
I actually can't land this until the Response + Split InternalHeaders patch is reviewed :)
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)
https://hg.mozilla.org/mozilla-central/rev/caa56adec4f5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
> 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)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.