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

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: emk, Assigned: baku)

Tracking

unspecified
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

7 years ago
Posted file 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?
Assignee

Updated

7 years ago
Assignee: nobody → amarchesini
Assignee

Comment 1

7 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #710663 - Flags: review?(bent.mozilla)
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);
Assignee

Comment 3

7 years ago
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?
Assignee

Comment 4

7 years ago
Posted patch patchSplinter Review
Attachment #710663 - Attachment is obsolete: true
Attachment #710663 - Flags: review?(bent.mozilla)
Attachment #710676 - Flags: review?(bent.mozilla)
Comment on attachment 710676 [details] [diff] [review]
patch

Looks fine to me assuming we don't need the null check.
Attachment #710676 - Flags: review?(bent.mozilla) → review+
Reporter

Comment 6

7 years ago
I first tried createObjectURL(null). It didn't crash and it threw TypeError correctly.
> 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!
Assignee

Updated

7 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d2e0efdf7ade
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.