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

Status: RESOLVED FIXED

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

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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...

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Just noting that this affects the Response object as well.

Comment 4

[Andrea Marchesini [:baku]]

I think this bug is fixed by bug 1047483. Can you try again?

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Allow Blobs in Fetch BodyInit

Thanks! That works.

Comment 6

[Andrea Marchesini [:baku]]

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.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I actually can't land this until the Response + Split InternalHeaders patch is reviewed :)

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/caa56adec4f5

Comment 10

[Andrea Marchesini [:baku]]

> 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.