766661 - Taking screenshots of long pages silently fails

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[RNicoletto]

Trying to capture long web pages via Developer Toolbar "screenshot" command, Firefox throws the following error:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMCanvasRenderingContext2D.drawWindow]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource:///modules/devtools/GcliCommands.jsm :: Command_screenshotGrabScreen :: line 128"  data: no]

Step to reproduce:
1. visit Planet Mozilla (http://planet.mozilla.org/)
2. start Developer Toolbar
3. execute "screenshot image 0 true"

Expected result: a file named image.png with the full page screenshot.
Actual result: exception error above.

NOTE: the "true" parameter in command line is mandatory to reproduce the error. Capturing only the visible portion of the web page ("false" parameter) works fine.

Comment 1

[RNicoletto]

Can this be related to bug 591822?

Comment 2

[Jonathan Vieyra]

My toolbar doesnt accept "screenshot image 0 true" it only recognizes "screenshot image"

Comment 3

[Joe Walker [:jwalker]]

(In reply to Jonathan Vieyra from comment #2)
> My toolbar doesnt accept "screenshot image 0 true" it only recognizes
> "screenshot image"

I think Jonathan's issue, and part of this issue, is logged in bug 799769.

The syntax for step 2 above is "screenshot --fullpage"

Comment 4

[Girish Sharma [:Optimizer]]

I think comment 1 is correct. It is a D2D issue.
Joe, what should the approach be ?

Comment 5

[Jonathan Vieyra]

Hi, Im interested in taking on this bug for a class project, and this is my first bug fix so I would appreciate assistance while working on this bug.

Comment 6

[Joe Walker [:jwalker]]

(In reply to Jonathan Vieyra from comment #5)
> Hi, Im interested in taking on this bug for a class project, and this is my
> first bug fix so I would appreciate assistance while working on this bug.

This bug is really just a manifestation of bug 591822, so you'd need to solve that one first. I'd ask on that bug.

Comment 7

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #6)
> (In reply to Jonathan Vieyra from comment #5)
> > Hi, Im interested in taking on this bug for a class project, and this is my
> > first bug fix so I would appreciate assistance while working on this bug.
> 
> This bug is really just a manifestation of bug 591822, so you'd need to
> solve that one first. I'd ask on that bug.

That bug is too big of a scope for first time bug. I suggest that we handle that case from the command side, displaying a message when height of the page exceeds limit. Something like "Web page too long. See bug bug 591822"

Comment 8

[Paul Rouget [:paul]]

We could take partial pictures and concatenate them.

Comment 9

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #8)
> We could take partial pictures and concatenate them.

Awesome idea. Lets do it.

@Jonathan Vieyra
This[1] should be a very good place for all the information you need for building Firefox. For any doubts, join the irc channel #devtools and ping anyone.

[1] https://wiki.mozilla.org/DevTools/GetInvolved

Comment 10

[Joe Walker [:jwalker]]

This bug was not fixed with Firefox 17.

Comment 11

[Girish Sharma [:Optimizer]]

@Jonathan Vieyra : Are you still planning to fix this bug ?

Comment 12

[Jonathan Vieyra]

@Girish Sharma
Not at the moment, Ive chosen a different bug for my project.

Comment 13

[Girish Sharma [:Optimizer]]

Okay cool.

Comment 14

[Girish Sharma [:Optimizer]]

I have already fixed this. But as soon as you fix this bug, you ought to run into the limitation of canvas.toDataURL , so basically the actual issue was a exception at around 700px width/height on a 96dpi system. But as soon as you stitch 3-4 images of such size, your toDataURL will fail and throw. I am trying to find a work around.

Comment 15

[Girish Sharma [:Optimizer]]

(I meant 7000px not 700px)

Comment 16

[Girish Sharma [:Optimizer]]

(dammit , the actual stats are 10485px width/height on 96dpi is the failing size.)

Sorry for the spam.

Comment 17

[Girish Sharma [:Optimizer]]

Progress so far:

1) At first I tried of concatenating partial pictures. So I was looping on the height and doing ctx.drawWindow for at max 7000px only from a temporary canvas. Then drawing that canvas using drawImage method at the correct position on the main canvas. This worked perfectly for any height window (I tried till 93K px height). But as soon as I did canvas.toDataURL, to retrieve the base64 string for the image, to copy or to save to file , it gave the exact same exception. So internally it was failing due to the exact same reason. Which means that this approach is useless now.

Then after a very extensive discussion with bz and joe , they gave me another way of doing this, avoiding the canvas.

2) I loop through the height of the window, taking certain height tiles at a time. Draw them on a canvas, get the pixel data via getImageData().data for each tile. Then finally combine all the pixel data into an image encoder to get the output image stream. But that also gives almost the same exception at around the similar pixel range.

So the only way possible now is that:

Since I already have all the pixel data for the whole big page, I can manually try to convert that into dataURI using something like this[1] . But that would mean an addition of so much code for this functionality.

[1] http://www.bytestrom.eu/blog/2009/1120a_jpeg_encoder_for_javascript

Comment 18

[Girish Sharma [:Optimizer]]

btw, the image encoder throws this exception for large pixel data:

'InternalError: allocation size overflow'

Comment 19

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #17)
> Progress so far:
> 
> 1) At first I tried of concatenating partial pictures. So I was looping on
> the height and doing ctx.drawWindow for at max 7000px only from a temporary

You really shouldn't hard code such values. They differ greatly from hardware to hardware, depending on the graphics card and driver capabilities. On fairly capable hardware, the limitation is 2^13 pixels width/height, as given by a gl.getParameter(gl.MAX_TEXTURE_SIZE), where gl is a webgl context on a canvas.

Since manually creating a canvas and webgl context just to query this information is a bit overkill (and may even fail on some machines), consider the fact that 2^13 is available to 61.1% of users (according to [0]), and 2^12 to 90.0%. The safest bet would be going for chunks of 2^11 pixels maximum width and height (remember that width is also important!).

[0] http://webglstats.com/

Comment 20

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vp] from comment #19)
However, if the limitation is only because of canvas.toDataURL being silly and not drawWindow being able to handle large widths/heights, then disregard comment #19.

Comment 21

[Girish Sharma [:Optimizer]]

Any thoughts on this, Joe ?
PS: refer comment 14 and comment 17

Comment 22

[Paul Rouget [:paul]]

(In reply to Girish Sharma [:Optimizer] from comment #17)
> 2) I loop through the height of the window, taking certain height tiles at a
> time. Draw them on a canvas, get the pixel data via getImageData().data for
> each tile. Then finally combine all the pixel data into an image encoder to
> get the output image stream. But that also gives almost the same exception
> at around the similar pixel range.

Once you have the canvas, can you do canvas.mozGetAsFile() or canvas.toBlob? And then save this file?

Comment 23

[Girish Sharma [:Optimizer]]

Let me try that, but I doubt that it will work as the core method is the same that is failing due to size limitation.

Comment 24

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #22)
> Once you have the canvas, can you do canvas.mozGetAsFile() or canvas.toBlob?
> And then save this file?

Yup, both fail giving similar exceptions.

Comment 25

[Girish Sharma [:Optimizer]]

Any suggestions here ?

Comment 26

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #25)
> Any suggestions here ?

I think that silence means that we don't have any suggestions, which means that your guesses about what we do next are probably as good as ours. What do you think we should do?

Comment 27

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #26)
> I think that silence means that we don't have any suggestions, which means
> that your guesses about what we do next are probably as good as ours. What
> do you think we should do?

Two things can be done here:

1) Get help from Joe Drew or other gfx people to fix the exception 'InternalError: allocation size overflow' that the image encoder throws.

or

2) Show an error message in the command that "Height/Width greater than xxxx px are not supported" and do not fix anything in the code .

Comment 28

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #27)
> (In reply to Joe Walker [:jwalker] from comment #26)
> > I think that silence means that we don't have any suggestions, which means
> > that your guesses about what we do next are probably as good as ours. What
> > do you think we should do?
> 
> Two things can be done here:
> 
> 1) Get help from Joe Drew or other gfx people to fix the exception
> 'InternalError: allocation size overflow' that the image encoder throws.
> 
> or
> 
> 2) Show an error message in the command that "Height/Width greater than xxxx
> px are not supported" and do not fix anything in the code .

I think you're right. Joe is CCed on this bug, so for now we just do 2. We could also consider trimming the output size so there is *something* output.
Thanks.

Comment 29

[Kemcy BEST]

I was about to submit a duplicate, thanks for the search similar feature.

I was thinking if this bug is related to lazy-loading images (images which don't load until you scroll to certain part of a page). Now I know that the problem lies in the page height.

FYI: this bug breaks a lot of screenshot extensions, like Screengrab! (fixed version).

Comment 31

[mandarke]

i get this issue when trying to capture busy threads on image boards and IP.Board based communities.

Comment 32

[Girish Sharma [:Optimizer]]

Clearing this from my list as the solution to this is now very straight forward and this could be a really good good first bug .

Please refer to comment 28 for what to do exactly. I am willing to be the mentor for this one :)

Comment 33

[Sachin Hosmani [:sachin]]

Attachment: Shows a message and trims the screenshot.

What exact values for max height/width must I use?
Also, please suggest a message.

Comment 34

[Girish Sharma [:Optimizer]]

Comment on attachment 748428 [details] [diff] [review]
Shows a message and trims the screenshot.

Review of attachment 748428 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Sachin, thanks for the patch. A couple more revisions and a test and this will be ready to go!

Please add a test for this use case. Please ping me on IRC on #devtools if you have any doubt. I am Optimizer, I mean my nick.

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +1832,5 @@
>          width = window.innerWidth + window.scrollMaxX;
>          height = window.innerHeight + window.scrollMaxY;
>        }
> +
> +      if(height > 7000) {

Space after the if.

@@ +1833,5 @@
>          height = window.innerHeight + window.scrollMaxY;
>        }
> +
> +      if(height > 7000) {
> +        height = 7000;

I am not exactly sure that the number is 7000, but I think its a good bet.

@@ +1837,5 @@
> +        height = 7000;
> +        trimmed = true;
> +      }
> +
> +      if(width > 7000) {

(space after the if)

@@ +1881,5 @@
>            let clip = Cc["@mozilla.org/widget/clipboard;1"].getService(clipid);
>            clip.setData(trans, null, clipid.kGlobalClipboard);
> +
> +          div.textContent = trimmed ? gcli.lookup("screenshotTrimmedAndCopied") :
> +            gcli.lookup("screenshotCopied");

':' in next line, vertically below the '?' like

trimmer ? gcli.look...
        : gcli.look...

@@ +1941,5 @@
>        let source = ioService.newURI(data, "UTF8", null);
>        persist.saveURI(source, null, null, null, null, file, loadContext);
>  
> +      div.textContent = (trimmed ? gcli.lookup("screenshotTrimmedAndSavedToFile") :
> +        gcli.lookup("screenshotSavedToFile")) + " \"" + filename +

ditto

@@ +1946,1 @@
>                          "\"";

this should not remain on a third line now.

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +126,5 @@
>  screenshotSavedToFile=Saved to
>  
> +# LOCALIZATION NOTE (screenshotSavedToFile) Text displayed to user when the
> +# screenshot is successfully saved to the file specified.
> +screenshotTrimmedAndSavedToFile=The screenshot was trimmed due to size limitations. Saved to

since this is already a long line, it would be better to keep it continuous :
limitations and saved to

@@ +138,5 @@
>  screenshotCopied=Copied to clipboard.
>  
> +# LOCALIZATION NOTE (screenshotCopied) Text displayed to user when the
> +# screenshot is successfully copied to the clipboard.
> +screenshotTrimmedAndCopied=The screenshot was trimmed due to size limitations. Copied to clipboard.

ditto

Comment 35

[Sachin Hosmani [:sachin]]

Attachment: Addresses the comments and includes a test case.

Comment 36

[Sachin Hosmani [:sachin]]

Comment on attachment 750005 [details] [diff] [review]
Addresses the comments and includes a test case.

Review of attachment 750005 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, this is on mozilla-central.
I'll submit another patch on fx-team.

Comment 37

[Girish Sharma [:Optimizer]]

(In reply to Sachin Hosmani from comment #36)
> Comment on attachment 750005 [details] [diff] [review]
> Addresses the comments and includes a test case.
> 
> Review of attachment 750005 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, this is on mozilla-central.
> I'll submit another patch on fx-team.

There is not really a need. But if you want to, wait for the review, so that you can address some minor comments that I have.

Comment 38

[Girish Sharma [:Optimizer]]

Comment on attachment 750005 [details] [diff] [review]
Addresses the comments and includes a test case.

Review of attachment 750005 [details] [diff] [review]:
-----------------------------------------------------------------

I tried the screenshot command on a 31K height page and it worked. I guess 7000 is not the max limit. Can you confirm that on your side and make the max number to be 32767 (as explained in bug 591822)

r+ with this and below changes.

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +1840,5 @@
> +      if (width > 7000) {
> +        width = 7000;
> +        trimmed = true;
> +      }
> +

make the 7000 (or 32767) a constant and put at around the top of the file.

@@ +1945,1 @@
>        div.addEventListener("click", function openFile() {

please constraint yourself to 80 columns.

::: browser/devtools/commandline/test/browser_cmd_screenshot_long.html
@@ +1,4 @@
> +<html>
> +  <head></head>
> +  <body>
> +    <div style="background-color: orange; height: 8000px;"></div>

This might require to be changed.

::: browser/devtools/commandline/test/browser_cmd_screenshot_long.js
@@ +38,5 @@
> +            chrome: { value: false },
> +          },
> +        },
> +        exec: {
> +          output: new RegExp("^The screenshot was trimmed due to size limitations and saved to "),

This will cause the tests to fail locally on profiles with non english locales.
Can you instead obtain the string here from the properties file and check for a non ^ regecp match, since some locales might have the file name before the rest of the text.

@@ +71,5 @@
> +            chrome: { value: false },
> +          },
> +        },
> +        exec: {
> +          output: new RegExp("^The screenshot was trimmed due to size limitations and copied to clipboard.$"),

dutto.
Also, 80 column.

Comment 39

[Sachin Hosmani [:sachin]]

Attachment: Addresses the comments.

I had to extend the test timeout as taking screenshots of pages this big seems to take some time and the test failed otherwise.

Comment 40

[Girish Sharma [:Optimizer]]

(In reply to Sachin Hosmani from comment #39)
> Created attachment 753936 [details] [diff] [review]
> Addresses the comments.
> 
> I had to extend the test timeout as taking screenshots of pages this big
> seems to take some time and the test failed otherwise.

The patch looks fine .. but when I pushed the patch as is, it was timing out on ubuntu and fedora : https://tbpl.mozilla.org/?tree=Try&rev=752c91422d07 and when I requested even longer timeout (3), Ubuntu and Fedora worked fine. The issue now is that Windows build are giving OOM exceptions. I guess, having this long raw image data is too much for our Windows slaves..

@Joe, any suggestions ?

Comment 41

[Girish Sharma [:Optimizer]]

(longer timeout try push with OOM exception) : https://tbpl.mozilla.org/?tree=Try&rev=7efc741790bd

Comment 42

[Joe Walker [:jwalker]]

Comment on attachment 753936 [details] [diff] [review]
Addresses the comments.

Review of attachment 753936 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work. A few things to tidy-up but this is looking good.

::: browser/devtools/commandline/test/browser_cmd_screenshot_long.js
@@ +98,5 @@
> +
> +            // Recent PB changes to the test I'm modifying removed the 'pb'
> +            // variable, but left this line in tact. This seems so obviously
> +            // wrong that I'm leaving this in in case the analysis is wrong
> +            // pb.privateBrowsingEnabled = true;

Not sure we need this cut and paste section. Also could you remove it from the original?

@@ +107,5 @@
> +  },
> +};
> +
> +function test() {
> +  requestLongerTimeout(2);

This does worry me a bit. Do we know what is causing the slowdown exactly? How can we be sure that we're not going to be creating an intermittent orange which fails whenever disk activity on linux conflicts with our test for example?
Please could you add a comment that explains why we need this - and I think we need more than "got timeouts, this seems to have fixed it"
Thanks

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +126,5 @@
>  screenshotSavedToFile=Saved to
>  
> +# LOCALIZATION NOTE (screenshotTrimmedAndSavedToFile) Text displayed to user when the
> +# screenshot is successfully trimmed and saved to the file specified.
> +screenshotTrimmedAndSavedToFile=The screenshot was trimmed due to size limitations and saved to

Really we should have "... and save to %1$S" to allow localization into languages where it isn't correct to have the filename as the last parameter.

If you're feeling helpful, you could also fix the same problem in the previous 2 strings, however we'd need to change the key from screenshotSavedToFile to screenshotSavedToFile2 or similar so that localizers know the string as changed.

Comment 43

[Sachin Hosmani [:sachin]]

(In reply to Joe Walker [:jwalker] from comment #42)

> This does worry me a bit. Do we know what is causing the slowdown exactly?
> How can we be sure that we're not going to be creating an intermittent
> orange which fails whenever disk activity on linux conflicts with our test
> for example?

I'm not sure I understood what you meant, but if you meant that it happened by chance, I must say it happened all the times I ran the tests and is also happening in the try server.

> Please could you add a comment that explains why we need this - and I think
> we need more than "got timeouts, this seems to have fixed it"

Please tell me what comment I must put. I don't know what is causing it either.

Comment 44

[Girish Sharma [:Optimizer]]

dammit. I don't know how I missed the above two comments. Anyways. Joe, the tests concern me as they give OOM exceptions, any comment on that ?

Comment 45

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #44)
> dammit. I don't know how I missed the above two comments. Anyways. Joe, the
> tests concern me as they give OOM exceptions, any comment on that ?

I'm not sure why the tests give OOM exceptions, but I am sure that we can't commit code that causes oranges.

Comment 46

[Girish Sharma [:Optimizer]]

The test give OOM because we have to store the color values of 33000 * 1000+ pixel in memory. That seems to be too much on the windows machines where we have a 2 GB limit per process .

Comment 47

[JH]

Got this error code with a long thread of "Tinyboard v0.9.6-dev-16"


[Exception... "Failure" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource://app/modules/devtools/BuiltinCommands.jsm :: <TOP_LEVEL> :: line 1765" data: no]

Comment 48

[Joe Walker [:jwalker]]

Resetting priorities because these P* levels are very out of date.
Sorry for the bug spam. Filter on Lobster Thermidor

Comment 49

[Elbart]

(In reply to Girish Sharma [:Optimizer] from comment #46)
> The test give OOM because we have to store the color values of 33000 * 1000+
> pixel in memory. That seems to be too much on the windows machines where we
> have a 2 GB limit per process .

That would a) be only about 130MB (when using 4bytes per pixel), and b) 32bit-Firefox can use 4GB of memory on the more and more dominant Windows-64bit-systems.

It's probably because of VM-fragmentation, see bug 941823.

Comment 51

[Girish Sharma [:Optimizer]]

Now happening from the screenshot button too . Uservoice :
http://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/6107918-fix-full-page-screenshot-fails-on-large-pages

Comment 52

[Joe Walker [:jwalker]]

We have related rendering problems in bug 1032498. Maybe workarounds that we consider here could also do for that bug?

Comment 53

[Girish Sharma [:Optimizer]]

That is a bad bug. Also, as per comment 49, I don't have any idea about the OOM's I was getting in the try runs.

Also, I am pretty sure that users won't like the solution presented in this bug (trimming the screenshot)

I think we need help of gfx/layout people to fix the rendering and size limitation issues here.

Comment 59

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Including some ideas from duplicate / similar bugs:

From bug 1187311 comment 0:
> Two possibilities:
> 
> 1) Trigger a notice: Show a notice why no screenshot can be made (too large
> canvas)
> 2) Capture it anyways in slices.

Jeff Griffiths (:canuckistani) in bug 1032498 comment 6:
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > From duplicate bug 1187311:
> > 
> > Two possibilities:
> > 
> > 1) Trigger a notice: Show a notice why no screenshot can be made (too large
> > canvas)
> 
> We should at least do this, ASAP.
> 
> > 2) Capture it anyways in slices.
> 
> IMO this is pretty interesting, especially if we replace the message in 1)
> with a message about what we did.

Michael Ratcliffe [:miker] [:mratcliffe] in bug 1032498 comment 7:
> In my opinion when an image is too large we should use multiple canvases to
> capture slices and then manually reassemble the image from canvas data...
> that way we only need to output a single file.

Comment 60

[YF (Yang)]

The issue also happens when "Screenshot Node" (in Inspector) for an oversized element.

Comment 61

[YF (Yang)]

Attachment: oversized element (testcase)

Comment 62

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Let's keep this in GCLI.  Under the covers, I believe the inspector's version also invokes GCLI as well.

Comment 64

[Mostafa.S]

Hello
I not familiar with fixing bugs ..I just report a similar bug here.
https://bugzilla.mozilla.org/show_bug.cgi?id=1232225 (I Think)

maybe help fix this bug...

Comment 67

[Maxim Degtyarev]

Still reproducible on Firefox 54.0 (32-bit). 
Tried execute "screenshot --clipboard --fullpage" on https://planet.mozilla.org/ and got following in the browser console:


Exception { message: "", result: 2147500037, name: "NS_ERROR_FAILURE", filename: "resource://gre/modules/commonjs/too…", lineNumber: 308, columnNumber: 0, data: null, stack: "createScreenshotData@resource://gre…", location: XPCWrappedNative_NoHelper }  cli.js:2056
Object { isTypedData: true, data: Object, type: "error" }  cli.js:2056
Object { columnNumber: 0, lineNumber: 308, message: "", stack: "createScreenshotData@resource://gre…" }  cli.js:2056

Comment 68

[Márton Salomváry]

The screenshot command seems to be gone (as of Firefox 57 macOS the dev tools Console does not accept a screenshot command) but the program still exists. New steps to reproduce:

- Open dev tools
- Open settings (cog wheel icon)
- Go to "Available Toolbox Buttons"
- Make sure "Take a screenshot of the entire page" is enabled
- Open https://en.wikipedia.org/wiki/2017_in_paleontology
- Open dev tools
- Click the camera icon on the dev tools toolbar

Expected: a screenshot is saved to the downloads folder (plus shutter sound, download animation on the main toolbar)

Actual: nothing happens

Comment 69

[Márton Salomváry]

By "program still exists" I mean "*problem* still exists".

Comment 74

[Peter Bengtsson [:peterbe]]

What didn't used to work now works. Yay!
It *used* to fail silently on this monster of a page: https://www.peterbe.com/plog/blogitem-040601-1
Now, it works. FYI, it makes a 3346x28676 PNG. This didn't work 2 weeks ago. 
I'm using Nightly as of 2018-07-06.

Admittedly, that page (and mentioned in my dupe https://bugzilla.mozilla.org/show_bug.cgi?id=1471329) has changed slightly in the last 2 weeks so perhaps I'm *just* on the cusp of it working now.

Comment 75

[:glob ✱]

(In reply to Peter Bengtsson [:peterbe] from comment #74)
> It *used* to fail silently on this monster of a page:
> https://www.peterbe.com/plog/blogitem-040601-1
> Now, it works. FYI, it makes a 3346x28676 PNG. This didn't work 2 weeks ago. 
> Admittedly, that page has changed slightly in the last 2 weeks so perhaps
> I'm *just* on the cusp of it working now.

it still fails on longer pages (eg. about:license).
28676 _is_ suspiciously just short of 32768 (although i don't know if 2**15 is the limiting factor here).


showing some kind of error message is likely to go a long way towards reducing duplicates.

Comment 78

[BhenChod]

(In reply to Maxim Degtyarev from comment #67)

Still reproducible on Firefox 54.0 (32-bit).
Tried execute "screenshot --clipboard --fullpage" on
https://time4buying.com/wella-toners and got following in the browser console:

Exception { message: "", result: 2147500037, name: "NS_ERROR_FAILURE",
filename: "resource://gre/modules/commonjs/too…", lineNumber: 308,
columnNumber: 0, data: null, stack: "createScreenshotData@resource://gre…",
location: XPCWrappedNative_NoHelper } cli.js:2056
Object { isTypedData: true, data: Object, type: "error" } cli.js:2056
Object { columnNumber: 0, lineNumber: 308, message: "", stack:
"createScreenshotData@resource://gre…" } cli.js:2056

I have already fixed this. But as soon as you fix this bug, you ought to run into the limitation of canvas.toDataURL , so basically the actual issue was a exception at around 700px width/height on a 96dpi system. But as soon as you stitch 3-4 images of such size, your toDataURL will fail and throw. I am trying to find a work around.

Comment 79

[Florens Verschelde :fvsch]

We're having issues producing large images with the :screenshot command on very long pages and/or when using high resolution DPR values (e.g. :screenshot --fullpage --dpr 2 and above).

When stressing screenshots in that way, I guess there are multiple things that may fail. Here are some failures I could reproduce with a blank document. Note: I'm using screenshots of the visible content only, so this may not include classes of problems that involve the --fullpage option.

A. Screenshot fails if one dimension is larger than 32767

1. Open in a new tab: data:text/html,<body style="background:pink"></body>
2. Open Responsive Design Mode, and set the RDM viewport to width=1000, height=50
3. In Console, run :screenshot --dpr 32.767
4. In Console, run :screenshot --dpr 32.768

In my tests, step 3 produces a 32767×1638 image, and step 4 fails silently.

B. Fails if area is larger than X?

With a square viewport, we reach a limit earlier than edge>32767.

1. Open in a new tab: data:text/html,<body style="background:pink"></body>
2. Open Responsive Design Mode, and set the RDM viewport to width=1000, height=1000
3. Run :screenshot --dpr 11.18
4. Run :screenshot --dpr 11.181

#3 works and gives me a 11180×11180 image.
#4 fails silently (expected: a 11181×11181 image).

So what's the difference between #3 and #4?

Expected image dimensions	Number of pixels	Profile
11180 × 11180	124,992,400	https://perfht.ml/2nHVATP
11181 × 11181	125,014,761	https://perfht.ml/2m8b6Iu

It looks like we have a limit at 125,000,000 pixels (or 500MB if 4 bytes per RGBA pixel?).
The profiles also show that #3 takes 900+MB of memory, while #4 is flat, so it's probably failing early on purpose.

If we do this test again with a 1250×1000 viewport, we can run:

• :screenshot --dpr 9.999 -> 12498×9999 image
• :screenshot --dpr 10 -> 12500×10000 image (Profile: https://perfht.ml/2mPytGV)
• :screenshot --dpr 10.001 -> fails silently

UX considerations

If we don't want or can't change those limits, we should log a message in the Console saying why we can't make a screenshot. Something like:

Screenshot image is too large. Firefox cannot currently make screenshot images taller than 32767 pixels, or with a pixel count higher than 125 megapixels. Try using the --dpr option with a lower resolution:
:screenshot --fullpage --dpr 1

Comment 80

[Nicolas Chevobbe [:nchevobbe]]

For the error message, you can make something very quickly like:

diff --git a/devtools/server/actors/webconsole/utils.js b/devtools/server/actors/webconsole/utils.js
--- a/devtools/server/actors/webconsole/utils.js
+++ b/devtools/server/actors/webconsole/utils.js
@@ -700,15 +700,23 @@ WebConsoleCommands._registerOriginal("sc
   owner.helperResult = (async () => {
     // creates data for saving the screenshot
     // help is handled on the client side
-    const value = await captureScreenshot(args, owner.window.document);
-    return {
-      type: "screenshotOutput",
-      value,
-      // pass args through to the client, so that the client can take care of copying
-      // and saving the screenshot data on the client machine instead of on the
-      // remote machine
-      args,
-    };
+    try {
+      const value = await captureScreenshot(args, owner.window.document);
+      return {
+        type: "screenshotOutput",
+        value,
+        // pass args through to the client, so that the client can take care of copying
+        // and saving the screenshot data on the client machine instead of on the
+        // remote machine
+        args,
+      };
+    } catch (e) {
+      console.error(e);
+      return {
+        type: "error",
+        message: "Firefox wasn't able to take a screenshot.",
+      };
+    }
   })();
 });

I passed a plain string, but we should pass the key of a localized string in webconsole.properties.
Not sure we can assume it failed because of the image size though.

Also, I remember someone talking about a replacement for the current screenshot code (for Fission): drawSnapshot (dom/ipc/WindowGlobalParent.cpp#372).
I'm not sure this is completely production ready, but we might play with this in a near future.

Comment 83

[Patrick Brosset <:pbro>]

This is an old bug with many duplicates and a high vote count, we should really think of adding an error message when this happens. The most frustrating for people is that nothing happens.
A better solution would be to use the code that Firefox Screenshot uses as the tool does not seem subject to the same limitation.

Comment 84

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #83)

A better solution would be to use the code that Firefox Screenshot uses as the tool does not seem subject to the same limitation.

Actually bug 1576210 and bug 1570231 seem to disagree with this. But at least the tool does not fail silently. A part of the webpage still gets turned into the image.

Firefox Screenshot does this by capping the max height and width of the image with these values used here.

This diff shows when and how the capping was added. Note that the tool displays a warning when a full page screenshot was cut-off.

Comment 85

[Patrick Brosset <:pbro>]

Attachment: Bug 766661 - Attempt to prevent screenshot failures and warn user on errors

The changes made here are meant to make the screenshot code in DevTools
closer to how the Firefox Screenshots addon works.

1. It cuts off large images to 10000x10000
2. It reduces drp to 1 for fullpage images

When those things happen, a warning is logged in the content console so
the user is aware that they did happen.

Finally, because there are still cases when taking a screenshot could fail,
an error is logged in the content console when this happens.

Comment 86

[Patrick Brosset <:pbro>]

This is an attempt at fixing this. The UI of it is based on the console. I posted a message in DevTools' public Slack (in #ux), so this is not final, but I believe this is our best option right now.

Comment 87

[Florens Verschelde :fvsch]

It reduces dpr to 1 for fullpage images

This could regress the --fullpage --dpr <number> use case advertised here:
https://meyerweb.com/eric/thoughts/2015/10/22/firefoxs-screenshot-command/

Two ways to avoid that:

• Don't automatically change the DPR; when failing, the error message could suggest taking a screenshot with :screenshot --fullpage --dpr 1.
• Or automatically change the DPR only if --dpr <number> is not specified.

Comment 88

[Patrick Brosset <:pbro>]

(In reply to Florens Verschelde :fvsch from comment #87)

• Or automatically change the DPR only if --dpr <number> is not specified.

This is how I have implemented it. DPR will be decreased to 1 only if --fullpage is defined and --dpr is not specified.

Comment 89

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f216619d394a
Attempt to prevent screenshot failures and warn user on errors r=nchevobbe

Comment 90

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on browser_jsterm_screenshot_command_warnings.js

backout: https://hg.mozilla.org/integration/autoland/rev/c79d03d2790b674b4e07aee2b95e20bd350e0bbc

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=f216619d394a4efc01865652a7a8e8db7ff6f03e&selectedJob=290989815

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=290989815&repo=autoland&lineNumber=185854

[task 2020-02-28T15:55:29.696Z] 15:55:29 INFO - Details: error creating Audio decoder" {file: "chrome://devtools/content/webconsole/index.html" line: 0}]
[task 2020-02-28T15:55:29.696Z] 15:55:29 INFO - Buffered messages logged at 15:53:39
[task 2020-02-28T15:55:29.697Z] 15:55:29 INFO - Console message: [JavaScript Error: "getScreenshot(http://example.com/browser/devtools/client/webconsole/test/browser/test_jsterm_screenshot_command.html) failed: TypeError: NetworkError when attempting to fetch resource." {file: "resource://activity-stream/lib/Screenshots.jsm" line: 59}]
[task 2020-02-28T15:55:29.697Z] 15:55:29 INFO - getScreenshotForURL@resource://activity-stream/lib/Screenshots.jsm:59:10
[task 2020-02-28T15:55:29.698Z] 15:55:29 INFO - asyncmaybeCacheScreenshot@resource://activity-stream/lib/Screenshots.jsm:112:37
[task 2020-02-28T15:55:29.698Z] 15:55:29 INFO - _fetchScreenshot@resource://activity-stream/lib/TopSitesFeed.jsm:527:23
[task 2020-02-28T15:55:29.698Z] 15:55:29 INFO - _fetchIcon@resource://activity-stream/lib/TopSitesFeed.jsm:515:16
[task 2020-02-28T15:55:29.698Z] 15:55:29 INFO - getLinksWithDefaults@resource://activity-stream/lib/TopSitesFeed.jsm:408:16
[task 2020-02-28T15:55:29.698Z] 15:55:29 INFO - asyncrefresh@resource://activity-stream/lib/TopSitesFeed.jsm:431:30
[task 2020-02-28T15:55:29.698Z] 15:55:29 INFO - onAction@resource://activity-stream/lib/TopSitesFeed.jsm:771:14
[task 2020-02-28T15:55:29.698Z] 15:55:29 INFO - _middleware/</<@resource://activity-stream/lib/Store.jsm:63:17
[task 2020-02-28T15:55:29.698Z] 15:55:29 INFO - Store/this[method]@resource://activity-stream/lib/Store.jsm:39:54
[task 2020-02-28T15:55:29.698Z] 15:55:29 INFO - init/this.intervalId<@resource://activity-stream/lib/SystemTickFeed.jsm:27:24
[task 2020-02-28T15:55:29.698Z] 15:55:29 INFO - notify@resource://gre/modules/Timer.jsm:62:17
[task 2020-02-28T15:55:29.698Z] 15:55:29 INFO -
[task 2020-02-28T15:55:29.698Z] 15:55:29 INFO - Buffered messages finished
[task 2020-02-28T15:55:29.698Z] 15:55:29 INFO - TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser/browser_jsterm_screenshot_command_warnings.js | Test timed out -
[task 2020-02-28T15:55:30.693Z] 15:55:30 INFO - Removing tab.

Comment 91

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Nicolas, do you think we could fix the test failure?

Honza

Comment 92

[Nicolas Chevobbe [:nchevobbe]]

yes, we should try to move that over the finish line.
I'm going to have a look

Comment 93

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75df4b6a087f
Attempt to prevent screenshot failures and warn user on errors r=nchevobbe

Comment 94

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/75df4b6a087f

Comment 95

[Patrick Brosset <:pbro>]

Thanks for pushing it over the finish line for me Nicolas! Really appreciate it. This is going to save some users from a lot of frustration I hope.

Comment 96

[:Harald Kirschner :digitarald]

Thanks a lot Patrick for starting this patch. One less papercut!

Comment 98

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Patrick Brosset <:pbro> from comment #88)

This is how I have implemented it. DPR will be decreased to 1 only if --fullpage is defined and --dpr is not specified.

Is that how it was implemented? Because it looks like if --fullpage is specified, it will always use a dpr value of 1.