838567 - URL.createObjectURL(new Object()); in Workers crashes debug builds reliably

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: Testcase

> Comment on attachment 710222 [details] [diff] [review]
> patch
> 
> >+  if (!blob) {
> >+    SetDOMStringToNull(aResult);
> >+    aRv.Throw(NS_ERROR_TYPE_ERR);
> >+    return;
> >+  }
> 
> Do you have a test about this? I'm surprised tests do not fail assertions by
> this change.
> https://mxr.mozilla.org/mozilla-central/source/dom/bindings/ErrorResult.h#45

Don't you hear me anymore?

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 710663 [details] [diff] [review]
patch

Review of attachment 710663 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/URL.cpp
@@ +203,5 @@
>  
>    nsCOMPtr<nsIDOMBlob> blob = file::GetDOMBlobFromJSObject(aBlob);
>    if (!blob) {
>      SetDOMStringToNull(aResult);
> +    aRv.ThrowTypeError(MSG_DOES_NOT_IMPLEMENT_INTERFACE, &NS_LITERAL_STRING("Blob"));

It is probably best to first check for !aBlob and throw MSG_NOT_OBJECT, then if it fails to convert to Blob throw MSG_DOES_NOT_IMPLEMENT_INTERFACE... But I'd double check that with the bindings folks to see what their codegen does.

And I'm nervous about doing an address-of-temprary here. How about:

  NS_NAMED_LITERAL_STRING(blobStr, "Blob");
  aRv.ThrowTypeError(MSG_DOES_NOT_IMPLEMENT_INTERFACE, &blobStr);

Comment 3

[Andrea Marchesini [:baku]]

Bz, should we check if aBlob is null? In the URL.webidl I see

  static DOMString? createObjectURL(Blob blob, optional objectURLOptions options);

so probably this check is not needed. Is it?

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 710676 [details] [diff] [review]
patch

Looks fine to me assuming we don't need the null check.

Comment 6

[Masatoshi Kimura [:emk]]

I first tried createObjectURL(null). It didn't crash and it threw TypeError correctly.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Bz, should we check if aBlob is null?

Given that IDL, no.  It can't be null.  Maybe we should be passing it as JSObject& instead... but once we fix up workers so we can actually pass interfaces correctly, that should stop being a problem!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2e0efdf7ade

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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