1336351 - canvas.toBlob callback should be async when height or width is 0

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Vincent Liu[:vliu]]

This is a following bug based on [1]. In bug 1331925, sync callback was invoked when canvas.toBlob was called in null case. But from comment in [1], the callback should be async by following the spec.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1331925#c14

Comment 1

[Vincent Liu[:vliu]]

Attachment: 0001-Bug-1336351-The-null-blob-callback-of-canvas.toBlob-.patch

Hi bz, smaug,

The proposed patch lets the callback of toBlob be async in null blob case. Could you please have a review? Also, I am not sure where to add web platform test for this issue. Can you please have info about this? Really thanks.

Comment 2

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

Comment on attachment 8833710 [details] [diff] [review]
0001-Bug-1336351-The-null-blob-callback-of-canvas.toBlob-.patch

Tests could go somewhere here
http://searchfox.org/mozilla-central/source/testing/web-platform/tests/html/dom

Looks like there aren't really tests for this kind of stuff yet.

Comment 3

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

Apparently http://searchfox.org/mozilla-central/source/testing/web-platform/tests/html/semantics/embedded-content/the-canvas-element might be better place

Comment 4

[Vincent Liu[:vliu]]

Attachment: 0001-Bug-1336351-Add-web-platform-test.-r-smaug.patch

Hi :smaug,
The attached patch intends to add web platform test under testing/web-platform/tests/html/semantics/embedded-content/the-canvas-element/. Could you please have a review? Thanks

Comment 5

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

Comment on attachment 8833879 [details] [diff] [review]
0001-Bug-1336351-Add-web-platform-test.-r-smaug.patch

Could you possible add a check that the callbacks to toBlob aren't called synchronously. Something like

var toBlob1Called = false;
canvas.toBlob(function(blob){
  toBlob1Called = true;
  _assertSame(blob, null, "blob", "null");
}, 'image/jpeg');

assert_false(toBlob1Called, "toBlob callback shouldn't be called synchronously");

Comment 6

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

Comment on attachment 8833879 [details] [diff] [review]
0001-Bug-1336351-Add-web-platform-test.-r-smaug.patch

Er, how does the async handling work with _addTest?
Doesn't that just call t.done() after running the callback passed to it?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8833710 [details] [diff] [review]
0001-Bug-1336351-The-null-blob-callback-of-canvas.toBlob-.patch

Please dispatch on the owner document of the canvas, not directly on "current thread" (which in practice is main thread anyway, right?).  See also https://wiki.mozilla.org/Quantum/DOM#Dispatching

Comment 8

[Vincent Liu[:vliu]]

Attachment: 0001-Bug-1336351-The-null-blob-callback-of-canvas.toBlob-.patch

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> Comment on attachment 8833710 [details] [diff] [review]
> 0001-Bug-1336351-The-null-blob-callback-of-canvas.toBlob-.patch
> 
> Please dispatch on the owner document of the canvas, not directly on
> "current thread" (which in practice is main thread anyway, right?).  See
> also https://wiki.mozilla.org/Quantum/DOM#Dispatching

Thanks for the comment. Can you please have a review?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8834298 [details] [diff] [review]
0001-Bug-1336351-The-null-blob-callback-of-canvas.toBlob-.patch

>+    if (!doc) {

OwnerDoc never returns null, which is why it's not called GetOwnerDoc.

>+                  NewRunnableMethod<BlobCallback*>(

So this will hold a strong ref to the BlobCallback?

Is there a reason you couldn't do:

  NewRunnableMethod<Blob*>(&aCallback, &BlobCallback::Call, nullptr)?

and skip the extra FireNullBlobEvent method?  Does that not compile because of the various BlobCallback::Call overloads or something?  Or some other reason it doesn't work

>+HTMLCanvasElement::FireNullBlobEvent(BlobCallback* aCallback)
>+  return;

No need for that return.

r=me with the above addressed.

Comment 10

[Vincent Liu[:vliu]]

Attachment: 0002-Bug-1336351-Add-web-platform-test.-r-smaug.patch

(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8833879 [details] [diff] [review]
> 0001-Bug-1336351-Add-web-platform-test.-r-smaug.patch
> 
> Er, how does the async handling work with _addTest?

I didn't look into too much details about _addTest but just refers to the coding style in toBlob.jpeg.html.

Comment 11

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

Comment on attachment 8834317 [details] [diff] [review]
0002-Bug-1336351-Add-web-platform-test.-r-smaug.patch

>+var t = async_test("toBlob with zero dimension returns a null Blob");
>+var toBlob1Called = false;
since you test just one case, drop '1' from the name

>+c.toBlob(function(blob){
>+  _assertSame(blob, null, "blob", "null");
>+  c.width = 0;
>+  c.height = 100;
>+  c.toBlob(function(blob){
>+    toBlob1Called = true;
assign to true in the first callback, not here in the second one

>+    _assertSame(blob, null, "blob", "null");
>+  }, 'image/jpeg');
>+}, 'image/jpeg');
>+
>+assert_false(toBlob1Called, "toBlob callback shouldn't be called synchronously");
>+t.done();

Shouldn't you call t.done() in the second callback to toBlob?

And some t.step() should be used here before the asserts I think.
I know wpt tests are messy.

After chatting in #whatwg about toBlob.jpeg.html, it became clear that it doesn't actually test anything, since
the callback to toBlob is called after done();
wpt framework just has a bug where it doesn't warn about calling asserts after done(), nor does it warn about having tests without
any asserts.

Comment 12

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

FYI, https://github.com/w3c/web-platform-tests/pull/4753/files

Comment 13

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

Comment on attachment 8834298 [details] [diff] [review]
0001-Bug-1336351-The-null-blob-callback-of-canvas.toBlob-.patch

(no other comments than what bz had)

Comment 14

[Vincent Liu[:vliu]]

Attachment: 0001-Bug-1336351-Add-web-platform-test.-r-smaug.patch

(In reply to Olli Pettay [:smaug] from comment #12)
> FYI, https://github.com/w3c/web-platform-tests/pull/4753/files

Thanks for example code. Can you please review the attached patch. Thanks.

Comment 15

[Vincent Liu[:vliu]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)
> Comment on attachment 8834298 [details] [diff] [review]
> 0001-Bug-1336351-The-null-blob-callback-of-canvas.toBlob-.patch
> 
> >+    if (!doc) {
> 
> OwnerDoc never returns null, which is why it's not called GetOwnerDoc.

Ok. I will call OwnerDoc->OwnerDoc()->Dispatch(...) in next version of patch. 

> 
> >+                  NewRunnableMethod<BlobCallback*>(
> 
> So this will hold a strong ref to the BlobCallback?
> 

Yes.

> Is there a reason you couldn't do:
> 
>   NewRunnableMethod<Blob*>(&aCallback, &BlobCallback::Call, nullptr)?
> 
> and skip the extra FireNullBlobEvent method?  Does that not compile because
> of the various BlobCallback::Call overloads or something?  Or some other
> reason it doesn't work
> 

Met the compile error below.

In file included from /Volumes/firefoxos/gecko-dev/obj-x86_64-apple-darwin15.6.0/dom/html/Unified_cpp_dom_html0.cpp:65:
 0:07.64 /Volumes/firefoxos/gecko-dev/dom/html/HTMLCanvasElement.cpp:847:26: error: no matching function for call to 'NewRunnableMethod'
 0:07.64                          NewRunnableMethod<Blob*>(
 0:07.64                          ^~~~~~~~~~~~~~~~~~~~~~~~
 0:07.64 /Volumes/firefoxos/gecko-dev/obj-x86_64-apple-darwin15.6.0/dist/include/nsThreadUtils.h:948:1: note: candidate template ignored: couldn't infer template argument 'Method'
 0:07.64 NewRunnableMethod(PtrType&& aPtr, Method aMethod, Args&&... aArgs)
 0:07.64 ^
 0:07.64 /Volumes/firefoxos/gecko-dev/obj-x86_64-apple-darwin15.6.0/dist/include/nsThreadUtils.h:896:1: note: candidate function template not viable: requires 2 arguments, but 3 were provided
 0:07.64 NewRunnableMethod(PtrType&& aPtr, Method aMethod)
 0:07.64 ^
 0:08.37 1 error generated.


> >+HTMLCanvasElement::FireNullBlobEvent(BlobCallback* aCallback)
> >+  return;
> 
> No need for that return.
>

Agree

Comment 16

[Vincent Liu[:vliu]]

(In reply to Vincent Liu[:vliu] from comment #15)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)
> > Comment on attachment 8834298 [details] [diff] [review]
> > 0001-Bug-1336351-The-null-blob-callback-of-canvas.toBlob-.patch
> > 
> > >+    if (!doc) {
> > 
> > OwnerDoc never returns null, which is why it's not called GetOwnerDoc.
> 
> Ok. I will call OwnerDoc->OwnerDoc()->Dispatch(...) in next version of
> patch. 
> 

Sorry, it should be OwnerDoc()->Dispatch(...)

Comment 17

[Boris Zbarsky [:bzbarsky]]

> candidate template ignored: couldn't infer template argument 'Method'

Yes, so sounds like the multiple overloads of Call are the problem there.

Comment 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #17)
> > candidate template ignored: couldn't infer template argument 'Method'
> 
> Yes, so sounds like the multiple overloads of Call are the problem there.

I think you can choose overload by using static_cast. So that this should pass the compiler.

NewRunnableMethod<Blob*, const char*>(&aCallback, static_cast<void(BlobCallback::*)(Blob*, const char*)>(&BlobCallback::Call), nullptr, nullptr));

Comment 19

[Vincent Liu[:vliu]]

Attachment: 0002-Bug-1336351-The-null-blob-callback-of-canvas.toBlob-.patch

(In reply to Morris Tseng [:mtseng] [:Morris] from comment #18)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #17)
> > > candidate template ignored: couldn't infer template argument 'Method'
> > 
> > Yes, so sounds like the multiple overloads of Call are the problem there.
> 
> I think you can choose overload by using static_cast. So that this should
> pass the compiler.
> 
> NewRunnableMethod<Blob*, const char*>(&aCallback,
> static_cast<void(BlobCallback::*)(Blob*, const char*)>(&BlobCallback::Call),
> nullptr, nullptr));

Here the patch was attached casting to the correct overloaded function. r=me

Comment 20

[Vincent Liu[:vliu]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6019216a85fb27611b3ab2f3a52e620a228fc88b

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d98b2c08929
The null blob callback of canvas.toBlob should be async. r=bz, smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f57cb9c4a846
Add web platform test. r=smaug

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5d98b2c08929
https://hg.mozilla.org/mozilla-central/rev/f57cb9c4a846