1898679 - canvas.toDataURL creates a string that does not work as src for an image element

Status: NEW

(Core :: Networking, defect, P3)

Description

[Martin Husemann]

This is a tiny (ugly) reproducer extracted out of a largish application. The issue boils down to a canvas creating a huge PNG from its content with canvas.toDataURL() where the string looks ok at quick glance in the debugger, but assigning that string to the src property of an image element does not make that image element load the correct graphics.

The demo shows that the images "onload" method is not invoked in the failure case.

How to reproduce:

1. open the reproducer web page, which shows two largish pictures.
2. open the dev tools and show the javascript console
3. click on the upper (smaller) image and watch the reports on the console showing the image is loaded and "onload" is running (giving the size of the image)
4. scroll down to the second image and click it - notice that the console shows the image being clicked but "onload" is never run

Comment 1

[Timothy Nikkel (:tnikkel)]

Thanks for the reduced testcase!

The problem is that we hit the max uri size

https://searchfox.org/mozilla-central/rev/f5251f16058e6307d92da02eed07ee91b9ee6243/netwerk/base/nsNetUtil.cpp#1854

which is set to 32mb

https://searchfox.org/mozilla-central/rev/f5251f16058e6307d92da02eed07ee91b9ee6243/modules/libpref/init/StaticPrefList.yaml#13027

You can modify the pref network.url.max-length in about:config to get around this.

It'd also be good if we had some kind of warning, even if just in debug builds about this.

Interestingly, Chrome works on the testcase, but Safari does not and outputs an error in the console.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1721448

:valentin, since you are the author of the regressor, bug 1721448, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 3

[Martin Husemann]

Is there a way to query max uri size from the DOM?

Then the application could work around that limit or easily recognize the issue at least.
Another option would be to make canvas.toDataURL() respect the limit too.

Even better would be a way to do the "cloning" w/o going via a URI (and other browsers offering that too :-})

Comment 4

[Timothy Nikkel (:tnikkel)]

(In reply to Martin Husemann from comment #3)

Even better would be a way to do the "cloning" w/o going via a URI (and other browsers offering that too :-})

Can you do what you need with blob urls?

https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/toBlob

Comment 5

[Martin Husemann]

(In reply to Timothy Nikkel (:tnikkel) from comment #4)

Can you do what you need with blob urls?

Yes, that is a great solution (and two line change in the real app, yay!) - and it seems to work on quick testing.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Could we add a console warning to toDataURL when the dataURL creation fails due to size limits?

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #6)

Could we add a console warning to toDataURL when the dataURL creation fails due to size limits?

toDataURL does not fail, it uses a plain string (not an nsIURI object) and is not subject to a limit.

The failure happens in HTMLImageElement when we try to create a uri object from the string in src attribute and it fails.

Comment 8

[Tom S]

I also opened an issue to improve the MDN documentation for toDataURL.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

We should add a warning in the console when this fails:
https://searchfox.org/comm-central/rev/29cc9f4e0e23700f3f2232bd828ff3a80c78eea5/mozilla/dom/html/HTMLImageElement.cpp#305

StringToURI(attrVal.String(), OwnerDoc(), getter_AddRefs(mSrcURI));

It should be something like:

nsresult rv = StringToURI(attrVal.String(), OwnerDoc(), getter_AddRefs(mSrcURI));
if (NS_FAILED(rv)) {

nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, "DOM"_ns,
                                      OwnerDoc(), nsContentUtils::eDOM_PROPERTIES,
                                      "ImageSRCInvalidURL");

}

You'll also need to add the error message to dom.properties to be something like:

ImageSRCInvalidURL=Setting the src attribute of the image element failed because the string is not a valid URL.

Comment 10

[Mayank Bansal]

This is working as of bug 1911300, which increased the limit to 512MB.

Comment 11

[Akash B]

Hello! I'm new to contributing and I'd love to help with this issue. I've identified where in the codebase to add the error check & message, although I'm not sure if the limit change to 512MB that Mayank mentioned above makes this a less pressing fix.

I see that the warning :evilpie mentioned is up on the MDN website, in my personal opinion the console error log could still be beneficial to developers so I'm happy to implement it!

Comment 12

[Valentin Gosu [:valentin] (he/him)]

I think this is still useful, not only for the URL size limit, but there are other reasons why the URL could be invalid, in which case showing the warning in devtools would still be a useful hint to developers.