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

RESOLVED FIXED in Firefox 54

Status

()

Core
Canvas: 2D
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: vliu, Assigned: vliu)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

9 months ago
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
(Assignee)

Updated

9 months ago
Assignee: nobody → vliu
(Assignee)

Comment 1

9 months ago
Created attachment 8833710 [details] [diff] [review]
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.
Attachment #8833710 - Flags: review?(bzbarsky)
Attachment #8833710 - Flags: review?(bugs)
Attachment #8833710 - Flags: review?(bzbarsky)
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.
Attachment #8833710 - Flags: review?(bugs) → review+
Apparently http://searchfox.org/mozilla-central/source/testing/web-platform/tests/html/semantics/embedded-content/the-canvas-element might be better place
(Assignee)

Comment 4

9 months ago
Created attachment 8833879 [details] [diff] [review]
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
Attachment #8833879 - Flags: review?(bugs)
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");
Attachment #8833879 - Flags: review?(bugs) → review+
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?
Attachment #8833879 - Flags: review+ → review-
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
Attachment #8833710 - Flags: review-
(Assignee)

Comment 8

9 months ago
Created attachment 8834298 [details] [diff] [review]
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?
Attachment #8833710 - Attachment is obsolete: true
Attachment #8834298 - Flags: review?(bzbarsky)
Attachment #8834298 - Flags: review?(bugs)
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.
Attachment #8834298 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 10

9 months ago
Created attachment 8834317 [details] [diff] [review]
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.
Attachment #8833879 - Attachment is obsolete: true
Attachment #8834317 - Flags: review?(bugs)
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.
Attachment #8834317 - Flags: review?(bugs) → review-
FYI, https://github.com/w3c/web-platform-tests/pull/4753/files
Attachment #8834298 - Flags: review?(bugs) → review+
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)
(Assignee)

Comment 14

9 months ago
Created attachment 8834762 [details] [diff] [review]
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.
Attachment #8834317 - Attachment is obsolete: true
Attachment #8834762 - Flags: review?(bugs)
(Assignee)

Comment 15

9 months ago
(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
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 16

9 months ago
(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(...)
Attachment #8834762 - Flags: review?(bugs) → review+
> candidate template ignored: couldn't infer template argument 'Method'

Yes, so sounds like the multiple overloads of Call are the problem there.
Flags: needinfo?(bzbarsky)
(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));
(Assignee)

Comment 19

9 months ago
Created attachment 8835259 [details] [diff] [review]
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
Attachment #8834298 - Attachment is obsolete: true
(Assignee)

Comment 20

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

Comment 21

8 months ago
Pushed by vliu@mozilla.com:
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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5d98b2c08929
https://hg.mozilla.org/mozilla-central/rev/f57cb9c4a846
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.