Closed Bug 1200524 Opened 4 years ago Closed 3 years ago

Using "Save Image as" on a large canvas doesn't work

Categories

(Core :: Canvas: 2D, defect)

40 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox50 --- verified

People

(Reporter: pere.jobs, Assigned: mconley)

References

Details

(Keywords: crash, reproducible)

Attachments

(4 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150826185640

Steps to reproduce:

Open the attached file.
Right-click anywhere on the canvas.
Choose "Save Image As".


Actual results:

Firefox freezed for a long time.
Then it became responsive again. Without saving the image.


Expected results:

The browser should not freeze and the image should be saved.
This bug has first been reported here: https://github.com/lovasoa/dezoomify/issues/24
I juste created a simple test case.
Component: Untriaged → RSS Discovery and Preview
Component: RSS Discovery and Preview → Canvas: 2D
Product: Firefox → Core
I'm getting a crash on OSX, so it's plausible that it doesn't save a snapshot on another platform.

Can you please attach your about:support info? Navigate to 'about:support', hit one of the top left copy buttons and paste it here (add attachment, hit text, paste preferably). This will let us known what configuration is running.

Ideally this should probably be checked in as a crash-test.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, reproducible
Attached file about:support.json
Here are my `about:support` informations.
That smells typical OOM crash with an heavy image loaded in the browser.
On Win 7 with FF40, the memory peak is huge when FF is loading the image (and fails to display).
It's probably this one for windows:
https://hg.mozilla.org/mozilla-central/rev/46d9ffb97fe3

It's likely different for linux.
Blocks: 899785
I now get a different behavior on OSX. Now we just silently fail and output this to the browser console:
NS_ERROR_FAILURE:  content.js:827:0

We should get a proper error message for this.
Assignee: nobody → bgirard
He's a (probably wrong) solution to throw a better error message. There's a few more toDataURL() that can be checked in this file for video as well.
Comment on attachment 8717122 [details]
MozReview Request: Bug 1200524 - Warn when the canvas is too large to save. r=mconley

https://reviewboard.mozilla.org/r/34053/#review30715

::: browser/base/content/content.js:730
(Diff revision 1)
> -  let dataURL = message.objects.target.toDataURL();
> +    let dataURL = message.objects.target.toDataURL();

Is there a way we can detect whether or not the canvas is too large before attempting to do this conversion?

::: browser/base/content/content.js:739
(Diff revision 1)
> +      const promptSvc = Cc["@mozilla.org/embedcomp/prompt-service;1"].
> +                        getService(Ci.nsIPromptService);
> +      promptSvc.alert(content.window, title, msg);

We should probably be avoiding all prompt usage in what might be a content process at all times.

Instead, in this catch, I think we should send a message up to the parent passing along some kind of error code or state, and have the parent display the message.

In the parent, we might want to use [notifications](https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/notificationbox) instead of the prompt, as they're less intrusive, in general.

::: browser/base/content/content.js:742
(Diff revision 1)
> +    } catch (e) {}

Why the extra try/catch, out of curiosity?

`catch(e) {}` always freaks me out in general.
Attachment #8717122 - Flags: review?(mconley)
Rather than adding try/catch, would it be possible to use something other than an inefficient and unnecessary dataURL as a intermediary representation when saving a canvas to disk ?

I initially opened this bug because I'm the author of [a little opensource tool](https://github.com/lovasoa/dezoomify) that uses the <canvas> element, and regularly get bug reports from users who encounter this firefox bug. Getting an error message after the browser has freezed won't help them a lot...
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #10)
> Is there a way we can detect whether or not the canvas is too large before
> attempting to do this conversion?
> 

Not really, we could maybe have a way to see if it will likely work but we can't know for sure.
So we have to handle the failure anyways.

> ::: browser/base/content/content.js:742
> (Diff revision 1)
> > +    } catch (e) {}
> 
> Why the extra try/catch, out of curiosity?
> 
> `catch(e) {}` always freaks me out in general.

I copied it from the other example. I think the idea is not to bubble up an exception if the string
bundle can't be loaded but I think throwing another exception here would also be fine.

(In reply to pere.jobs from comment #11)
> Rather than adding try/catch, would it be possible to use something other
> than an inefficient and unnecessary dataURL as a intermediary representation
> when saving a canvas to disk ?

It's not ideal but whatever method we use we're going to need to handle the failure and I think it's important
to cover that correctness before doing any further optimization.

The freeze up can and should be optimized. In the mean time it would also be possible for you to encode the image yourself and provide your own download prompt instead of relying on the browser to handle 'Save Image As...' for large canvas since you're dealing with a really large canvas. However you might not be able to stream it out to disk and be restricted to needing to fit the entire encoded image in memory, I'm not sure.
Great! I didn't mean to say that this patch won't be useful, I was only hoping that the freeze would be fixed.

> it would also be possible for you to encode the image yourself
Unfortunately it isn't, because of the same-origin policy. My tool is an image downloader, and my canvas contains data from domains I don't own, so I don't have read access to the canvas content.
Ahh yes you're right. I guess 'Save Image As...' is a good way to provide your functionality across origin within the security policy. That's neat.
Is there anything new on this?
No. I haven't had time to make the necessary modification. If someone has time then feel free to take this from me. Otherwise I'll get back to it at some unknown time.
I've had users complaining about this bug again, so I've started to look at the code.

The issue doesn't seem very hard to solve.
It looks like in content.js (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#657) :

addMessageListener("ContextMenu:Canvas:ToDataURL", (message) => {
   let dataURL = message.objects.target.toDataURL();
   sendAsyncMessage("ContextMenu:Canvas:ToDataURL:Result", { dataURL });
});

could be replaced by 

addMessageListener("ContextMenu:Canvas:ToDataURL", (message) => {
   let dataURL = message.objects.target.toBlob((blob) => {
      let dataURL = URL.createObjectURL(blob);
      sendAsyncMessage("ContextMenu:Canvas:ToDataURL:Result", { dataURL });
   });
});


I'm pretty new to firefox hacking, so I might be very wrong. But if fixing the bug were that simple, I would love this to be integrated. It would help a lot of people!
(In reply to pere.jobs from comment #17)
> I've had users complaining about this bug again, so I've started to look at
> the code.
> 
> The issue doesn't seem very hard to solve.
> It looks like in content.js
> (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.
> js#657) :
> 
> addMessageListener("ContextMenu:Canvas:ToDataURL", (message) => {
>    let dataURL = message.objects.target.toDataURL();
>    sendAsyncMessage("ContextMenu:Canvas:ToDataURL:Result", { dataURL });
> });
> 
> could be replaced by 
> 
> addMessageListener("ContextMenu:Canvas:ToDataURL", (message) => {
>    let dataURL = message.objects.target.toBlob((blob) => {
>       let dataURL = URL.createObjectURL(blob);
>       sendAsyncMessage("ContextMenu:Canvas:ToDataURL:Result", { dataURL });
>    });
> });
> 
> 
> I'm pretty new to firefox hacking, so I might be very wrong. But if fixing
> the bug were that simple, I would love this to be integrated. It would help
> a lot of people!

That looks like a brilliant solution! Are you willing to put the patch together? I'll happily review it.
Flags: needinfo?(pere.jobs)
I've never done this kind of things, and I don't have a firefox compiling environment set up...
Is the `window.URL` object available in the context of this file (content.js) ?
Flags: needinfo?(pere.jobs)
(In reply to pere.jobs from comment #19)
> I've never done this kind of things, and I don't have a firefox compiling
> environment set up...
> Is the `window.URL` object available in the context of this file
> (content.js) ?

It can be made available, yes. I'll try putting together a quick patch.
Flags: needinfo?(mconley)
It turns out that Blob URLs can't be shared across processes, so I'm afraid this route will not work. :/ At least not now.

Hey mrbkap - is there a long-term goal of making Blob URLs work cross-process, or at least, from content to parent? Or is that unlikely to happen?
Flags: needinfo?(mconley) → needinfo?(mrbkap)
Oh, too bad :( Can these processes share typed array? That would still perform better than a base64 string.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #21)
> Hey mrbkap - is there a long-term goal of making Blob URLs work
> cross-process, or at least, from content to parent? Or is that unlikely to
> happen?

I thought blassey filed a bug on this, but I can't find it. I think it's unlikely to happen (IIRC, khuey had objections). I think the way to go here would be to send the blob itself over via structured cloning and then the parent can use the blob without having to transform it to base64.

Mike, can you try that out?
Flags: needinfo?(mrbkap) → needinfo?(mconley)
Assignee: bgirard → nobody
If this is still an issue, Peter, let's see if we can resolve it.
Flags: needinfo?(howareyou322)
Yes, this is still an issue. Thank you so much for your involvement!
I get regular bug reports to one of my projets that uses large canvases (dezoomify), that are in fact due to this bug.
(In reply to Milan Sreckovic [:milan] (June 10 PTO) from comment #24)
> If this is still an issue, Peter, let's see if we can resolve it.

I think I can reproduce it on my local build on OSX.

[GFX3-]: Surface size too large (exceeds allocation limit)!
[GFX3-]: Surface size too large (exceeds extent limit)!
Crash Annotation GraphicsCriticalError: |[0][GFX1-]: Failed to allocate a surface due to invalid size (CDT) Size(20480,20480) (t=719.79) [GFX1-]: Failed to allocate a surface due to invalid size (CDT) Size(20480,20480)
[GFX3-]: Surface size too large (exceeds allocation limit)!
[GFX3-]: Surface size too large (exceeds extent limit)!
Crash Annotation GraphicsCriticalError: |[0][GFX1-]: Failed to allocate a surface due to invalid size (CDT) Size(20480,20480) (t=719.79) |[1][GFX1-]: Failed to allocate a surface due to invalid size (CDT) Size(20480,20480) (t=719.79) [GFX1-]: Failed to allocate a surface due to invalid size (CDT) Size(20480,20480)
[GFX3-]: Surface size too large (exceeds allocation limit)!
[GFX3-]: Surface size too large (exceeds extent limit)!
Crash Annotation GraphicsCriticalError: |[0][GFX1-]: Failed to allocate a surface due to invalid size (CDT) Size(20480,20480) (t=719.79) |[1][GFX1-]: Failed to allocate a surface due to invalid size (CDT) Size(20480,20480) (t=719.79) |[2][GFX1-]: Failed to allocate a surface due to invalid size (CDT) Size(20480,20480) (t=719.79) [GFX1-]: Failed to allocate a surface due to invalid size (CDT) Size(20480,20480)

I can take it to look into more detail to see if it can be resolved.
Assignee: nobody → vliu
Clear ni since vicent is checking on this.
Flags: needinfo?(howareyou322)
Here are some investigations below:

Canvas 2d fails to create SkiaGL DrawTarget for some reason, so it fell back to software.

Crash Annotation GraphicsCriticalError: |[0][GFX1-]: Failed to create a SkiaGL DrawTarget, falling back to software
 (t=68.4423) [GFX1-]: Failed to create a SkiaGL DrawTarget, falling back to software


(In reply to pere.jobs from comment #17)
> I've had users complaining about this bug again, so I've started to look at
> the code.
> 
> The issue doesn't seem very hard to solve.
> It looks like in content.js
> (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.
> js#657) :
> 
> addMessageListener("ContextMenu:Canvas:ToDataURL", (message) => {
>    let dataURL = message.objects.target.toDataURL();
>    sendAsyncMessage("ContextMenu:Canvas:ToDataURL:Result", { dataURL });
> });
> 
> could be replaced by 
> 
> addMessageListener("ContextMenu:Canvas:ToDataURL", (message) => {
>    let dataURL = message.objects.target.toBlob((blob) => {
>       let dataURL = URL.createObjectURL(blob);
>       sendAsyncMessage("ContextMenu:Canvas:ToDataURL:Result", { dataURL });
>    });
> });
> 
> 

No matter toDataURL() or toBlob((blob) were called, they finally hit the Surface check error in [1]

[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/Factory.cpp#296

It is reasonable that the error was hit by over large canvas size.
The first fix is not to crash, but fail to save, because the canvas is too large.  That's this bug.  We should open a follow up "feature" bug to see if we can chop up the image and save it in pieces, or something equivalent, avoiding running out of memory.

(In reply to Milan Sreckovic [:milan] from comment #29)
> The first fix is not to crash, but fail to save, because the canvas is too
> large.  That's this bug.  

When I tried to reproduce this issue, I never meet Firefox freezed for a long time. The case I saw just a JavaScript error.

JavaScript error: chrome://browser/content/content.js, line 767: NS_ERROR_FAILURE:

For the canvas is too large case, do you think we just through error message like the current status or 
need to fire a *feature* bug to adjust canvas size when it happens?

> We should open a follow up "feature" bug to see if
> we can chop up the image and save it in pieces, or something equivalent,
> avoiding running out of memory.

I'd tried to set the canvas size is smaller than the size of image. The thing is, the saved image file would dynamically fit to canvas size. Based on this, I think we don't need to worry about memory issue.
Flags: needinfo?(milan)
pere.jobs - do you still see this problem in the latest Firefox nightly release?
Flags: needinfo?(milan) → needinfo?(pere.jobs)
Comment on attachment 8655302 [details]
firefox-canvas-bug.html

<canvas width="20480" height="20480"></canvas>
<img src="http://alanbetts.com/image/1/1200/0/uploads/earth-1278686566.jpg" style="display:none"/>
<script>
var i = document.querySelector("img"),
    c = document.querySelector("canvas"),
    ctx = c.getContext("2d");
i.onload = function() {
  ctx.drawImage(i, 0,0, c.width, c.height);
}
</script>
I had to update the image URL because the old one isn't valid anymore.

I tested on latest stable and on latest nightly.
On the latest stable, the things are exactly the same as when the bug was reported: image is drawn on the canvas, but "save image as" triggers a long freeze, and image is not saved.

On nightly, things are worse: the image is not even drawn on the canvas! Firefox prints on its standard output:
> [GFX1-]: Failed to allocate a surface due to invalid size (CDT) Size(20480,20480)
as was reported before. A right-click and "save image as" doesn't do anything, it does not even print a message in the console.
See Also: → 1278819
(In reply to pere.jobs from comment #33)
> I had to update the image URL because the old one isn't valid anymore.
> 

> 
> On nightly, things are worse: the image is not even drawn on the canvas!
> Firefox prints on its standard output:

> > [GFX1-]: Failed to allocate a surface due to invalid size (CDT) Size(20480,20480)

On mac, the image wasn't even drawn on canvas on nightly as well. After looks into it, I had a WIP locally and can draw on canvas.

It seems that there are some discussions here. 

We had a canvas size check in [1] before BufferProvider was created. 
   [1]: https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#1452

In which, gfxPrefs::MaxCanvasSize() was defined as 0x7fff (32767). The canvas size in the attached example didn't exceeds this value.
Suppose we defined canvas size to maximum value like 0x7fff, we should set gfx.max-alloc-size to 4294967296 to satisfy the 
condition in [2]. It is not since the current gfx.max-alloc-size set to 500000000 which is smaller than 4294967296.

   [2]: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/Factory.cpp#295

It seems that there was comment in [3] to mention how to decide gfx.max-alloc-size but I still even don't understand what's the proper value
we should define.

   [3]: https://bugzilla.mozilla.org/show_bug.cgi?id=1224254#c5

:nical, could you please have comment here? My local WIP set gfx.max-alloc-size to 4294967296. Temporary remove some code below in 
Factory::Init(const Config& aConfig) for not affecting sConfig->mMaxAllocSize. After that, I can draw on canvas. I think we should 
reconsider the gfx.max-alloc-size to satisfy the maximum canvas size. Or adjust the canvas size based on the maximum alloc-size.  

-  const int32_t kMinAllocPref = 10000000;
-  const int32_t kMinSizePref = 2048;
-  if (sConfig->mMaxAllocSize < kMinAllocPref) {
-    sConfig->mMaxAllocSize = kMinAllocPref;
-  }
-  if (sConfig->mMaxTextureSize < kMinSizePref) {
-    sConfig->mMaxTextureSize = kMinSizePref;
-  }
Flags: needinfo?(nical.bugzilla)
I haven't had time to look into this thoroughly today so I'll come back to it (maybe next week), but allocating huge canvases is not a good idea. Especially if this is going to cause us to try to allocate a surface of the same size of the GPU behind the scenes, I've ran into cases where the computer would freeze totally because the driver says it can handle the size and end up doing terrible things, And it's hard to know which driver is going to be okay with silly sizes. Even with the current maximum size, we can't reliably expect the allocation(s) to succeed and it puts us into a state where we are more likely to crash somewhere else because due to running out of memory.

So I don't think that increasing the maximum size would be a good thing (I would prefer it to be even smaller to be honest). We would go from never succeeding at rendering silly huge canvases to sometimes succeeding but not always without a reliable way to predict it, and we would loose in stability.
Flags: needinfo?(nical.bugzilla)
As I said earlier, these sizes are far from being "silly". And I'm the author of an application that uses such canvases, and that works with current Firefox (except for the current bug, that is about saving the canvas to disk). It works in Google chrome too. There is simply a regression in the nightly.
And aren't 2d canvases handled on the CPU ?
(In reply to pere.jobs from comment #37)
> And aren't 2d canvases handled on the CPU ?

Yes - once we go beyond a certain size.
(In reply to pere.jobs from comment #36)
> As I said earlier, these sizes are far from being "silly". And I'm the
> author of an application that uses such canvases, and that works with
> current Firefox (except for the current bug, that is about saving the canvas
> to disk). It works in Google chrome too. There is simply a regression in the
> nightly.

I tried to run your application on Comment 32 on Google chrome in my Ubuntu. The thing is I can't even see any
image drawn onto Canvas. Right-click and "save image as" can save a file to disk. But the file shows 0 bytes in it.
Did you see the same with me? I am not sure the part you said "it works in Google chrome too" means? Thanks
A question. What is the result when you loaded firefox-canvas-bug.html on Comment 32 in Google Chrome? Can you see image drawn on Canvas? As I said earlier, I'd tried and can't see image drawn in Google Chrome. It seems that Google Chrome also has a limitation so the canvas size are also limited to use.
I observe the same thing as you on google chrome, for file in comment 32. It displays in firefox (except nightly) and not in chrome.
Maybe we should discuss that in another bug ?
We are talking about the new issue with large canvases on firefox nightly, and not about this bug.
(In reply to pere.jobs from comment #36)
> As I said earlier, these sizes are far from being "silly". And I'm the
> author of an application that uses such canvases,

I apologize if the term silly sounded offensive, but from the perspective of a browser that needs to work reliably for most of our users (much lower hardware specs than us devs on average), these sizes are definitely unreasonable. My computer can certainly stomach huge canvases (if I disable GPU compositing), but I am not representative of our users demography and I still need to disable what makes scrolling smooth in order to not crash my driver on big canvases on some of my computers.

> Maybe we should discuss that in another bug ?

I filed bug 1282074 in which I give more details about why I think that moving the allocation limits is more work than tweaking a setting. I am not against supporting it, but I want to make it clear that it is a project of its own.
I'm on my turn sorry if I overreacted. I was afraid of seeing that these canvases that work on current Firefox stopped working in nightly, because all the service I provide is based on large canvases. I really don't know what I'll do if Firefox is to drop support for them!
I will un-take this bug since there are some other bugs to discuss big canvas issue.
Assignee: vliu → nobody
Now that bug 1200524 is fixed, we should be able to use Blob URLs as mrbkap first suggested in comment 23.
Depends on: 1279186
Flags: needinfo?(mconley)
Sorry, credit where it's due - this idea was from pere.jobs in comment 17!
Flags: needinfo?(pere.jobs)
Assignee: nobody → mconley
Data URLs were fine for smaller pieces of media, but for large media, we'd quickly
max out the message size limit, and that'd result in a failure to even show the
save dialog.

With Blob URLs, we can refer to large media using a unique identifier that
is easy to pass around, and works from content to parent process.

Review commit: https://reviewboard.mozilla.org/r/67474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67474/
Attachment #8775304 - Flags: review?(felipc)
Comment on attachment 8775304 [details]
Bug 1200524 - Use Blob URLs instead of Data URLs for saving media from the content process.

https://reviewboard.mozilla.org/r/67474/#review64620

neat, i didn't know this worked across processes out of the box. do you know if this is multi-e10s safe?

there were some bugs in the past for image drag'n'drop that used shmem to do things like this, and now I wonder if this would have been easier.
Attachment #8775304 - Flags: review?(felipc) → review+
https://reviewboard.mozilla.org/r/67474/#review64620

I think it's only just recently started to work out of the box since bug 1200524 was fixed. I believe it should be multi-e10s safe.
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22756e737bd1
Use Blob URLs instead of Data URLs for saving media from the content process. r=Felipe
Thank you very much! I have been waiting for this for one year!
(In reply to pere.jobs from comment #54)
> Thank you very much! I have been waiting for this for one year!

\o/ Should be in Nightly either tomorrow or the day after if you'd like to help test.
https://hg.mozilla.org/mozilla-central/rev/22756e737bd1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Flags: qe-verify+
This issue is VERIFIED FIXED on:
- Ubuntu 14.04 x64
- Windows 10 x64
- Mac OSX 10.11

using Firefox Beta 50.0b1 (21.09.2016)
Status: RESOLVED → VERIFIED
Flags: qe-verify+
This is actually not fixed. The situation has improved, and larger canvases can now be exported, but the exact same bug still occurs when the exported PNG size exceeds a certain limit.

How to reproduce:
1. Visit: http://ophir.alwaysdata.net/dezoomify/dezoomify.html#http://nla.gov.au/nla.obj-152794741/view
2. Click submit
3. Wait for the image to be fully loaded (this can take a few minutes)
4. Right-click the generated canvas, and choose 'Save as'
5. The bug described in this bug report still occurs

Tested on firefox nightly.
Pere, can you clone a new bug report from this bug with your STR, please.
https://bugzilla.mozilla.org/enter_bug.cgi?format=__default__&cloned_bug_id=1200524
Flags: needinfo?(pere.jobs)
Flags: needinfo?(pere.jobs)
You need to log in before you can comment on or make changes to this bug.