Closed Bug 1239300 Opened 5 years ago Closed 5 years ago

createImageBitmap ends up throwing wrong exceptions

Categories

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

36 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: smaug, Assigned: kaku)

References

Details

Attachments

(1 file, 1 obsolete file)

Testcase:
createImageBitmap(new Blob()).catch(function(a) { console.log(a); });
in web console

I see NS_ERROR_NOT_AVAILABLE, but if I read the spec correctly, the Promise should be
resolved with null.
Flags: needinfo?(tkuo)
(In reply to Olli Pettay [:smaug] from comment #0)
> Testcase:
> createImageBitmap(new Blob()).catch(function(a) { console.log(a); });
> in web console
> 
> I see NS_ERROR_NOT_AVAILABLE, but if I read the spec correctly, the Promise
> should be resolved with null.
Should it be "rejected" with null, instead of "resolved" with null?
(Spec here: https://html.spec.whatwg.org/multipage/webappapis.html#images)
Assignee: nobody → tkuo
Flags: needinfo?(tkuo) → needinfo?(bugs)
er, yes, rejected with null.
Flags: needinfo?(bugs)
So, we do reject the promise in this case but with an error message. 

However, what exactly "reject the promise with null" is? In our code base, we all reject promise with an error code. The most similar thing that I can recall is "resolve the promise with void" which we call Promise::MaybeResolve(JS::UndefinedHandleValue). 

Should we instead reject the promise with JS::UndefinedHandleValue?
Flags: needinfo?(bugs)
Status: NEW → ASSIGNED
or, should it be "JS::NullHandleValue"?
JS::NullHandleValue sounds right.
Flags: needinfo?(bugs)
Thanks, then I will go this way!
Comment on attachment 8709273 [details] [diff] [review]
Bug1239300 - reject prmoise with null while creating imagebitmap from empty blob; r?smaug

># Parent  a77b73c7723e1060993045fb31eb2f0a30473486
>Bug1239300 - reject prmoise with null while creating imagebitmap from empty blob; r?smaug
s/prmoise/promise/

>@@ -0,0 +1,19 @@
>+function testBug1239300() {
>+  return new Promise(function(resolve, reject) {
>+    createImageBitmap(new Blob()).then(
>+      function() {
>+        ok(false, "The promise should be reject with null.");
should reject or should be rejected

>+        reject();
>+      },
>+      function(result) {
>+        if (result == null) {
>+          ok(true, "The promise should be reject with null.");
>+          resolve();
>+        } else {
>+          ok(false, "The promise should be reject with null.");
with these too


> [test_imagebitmap.html]
>+tags = imagebitmap
> [test_imagebitmap_close.html]
>+tags = imagebitmap
> [test_imagebitmap_cropping.html]
>+tags = imagebitmap
> [test_imagebitmap_on_worker.html]
>+tags = imagebitmap
> [test_imagebitmap_structuredclone.html]
>+tags = imagebitmap
> [test_imagebitmap_structuredclone_iframe.html]
>+tags = imagebitmap
> [test_imagebitmap_structuredclone_window.html]
>+tags = imagebitmap
> [test_imagebitmap_transfer.html]
>+tags = imagebitmap
I'm not familiar with 'tags'. I assume it is something useful :)
Attachment #8709273 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (expect slower than usual review time for couple of days) from comment #8)
Thanks for your review. 
I should pay attention to my typos more..., sorry about that.

> > [test_imagebitmap_transfer.html]
> >+tags = imagebitmap
> I'm not familiar with 'tags'. I assume it is something useful :)
Yes, so that you can use "./mach mochitest <tag name>" to invoke all test files tagged with the <tag name>.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/060438e9046e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Where do you see the spec saying to reject with null?  That's a _really_ weird thing to be doing, and I don't see it anywhere in the spec...
Flags: needinfo?(kaku)
Flags: needinfo?(bugs)
Neither do I see it now, the spec might has been modified. I am happy to update the code if :smaug also agree on it.
Flags: needinfo?(kaku)
I'm pretty sure the spec talked about null. Nothing weird about that. 
But since the spec has changed, ok to change the implementation.
Flags: needinfo?(bugs)
> Nothing weird about that.

Rejecting with null is just as weird as throwing null would be.  Can you imagine a spec ever saying to throw null?
rejecting with exceptions is the weird thing. Rejecting with null would be just fine, IMHO - some resource just isn't available so you get null.
Blocks: 1293878
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.