1239300 - createImageBitmap ends up throwing wrong exceptions

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 1

[Tzuhao Kuo [:kaku]]

(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)

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

er, yes, rejected with null.

Comment 3

[Tzuhao Kuo [:kaku]]

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?

Comment 4

[Tzuhao Kuo [:kaku]]

or, should it be "JS::NullHandleValue"?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

JS::NullHandleValue sounds right.

Comment 6

[Tzuhao Kuo [:kaku]]

Thanks, then I will go this way!

Comment 7

[Tzuhao Kuo [:kaku]]

Attachment: Bug1239300 - reject prmoise with null while creating imagebitmap from empty blob; r?smaug

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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 :)

Comment 9

[Tzuhao Kuo [:kaku]]

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

Comment 10

[Tzuhao Kuo [:kaku]]

Attachment: Bug1239300 - reject promise with null while creating imagebitmap from empty blob; r=smaug

Carry r+.

Comment 11

[Tzuhao Kuo [:kaku]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6c5bad441fe

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/060438e9046e

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/060438e9046e

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/060438e9046e

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Tzuhao Kuo [:kaku]]

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.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

I'm pretty sure the spec talked about null. Nothing weird about that. 
But since the spec has changed, ok to change the implementation.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

https://github.com/whatwg/html/pull/884 changed the spec

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

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.