Closed Bug 766661 Opened 9 years ago Closed 6 months ago

Taking screenshots of long pages silently fails

Categories

(DevTools :: General, defect, P2)

defect

Tracking

(firefox77 fixed)

RESOLVED FIXED
Firefox 77
Tracking Status
firefox77 --- fixed

People

(Reporter: RNicoletto, Assigned: nchevobbe, NeedInfo)

References

(Depends on 1 open bug, )

Details

(Keywords: testcase, Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(2 files, 3 obsolete files)

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.
OS: Windows XP → All
Priority: -- → P2
Hardware: x86 → All
Target Milestone: --- → Firefox 17
Can this be related to bug 591822?
Component: Developer Tools → Developer Tools: Graphic Commandline and Toolbar
My toolbar doesnt accept "screenshot image 0 true" it only recognizes "screenshot image"
(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"
I think comment 1 is correct. It is a D2D issue.
Joe, what should the approach be ?
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.
(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.
(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"
We could take partial pictures and concatenate them.
(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
This bug was not fixed with Firefox 17.
Target Milestone: Firefox 17 → Future
@Jonathan Vieyra : Are you still planning to fix this bug ?
@Girish Sharma
Not at the moment, Ive chosen a different bug for my project.
Okay cool.
Assignee: nobody → scrapmachines
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.
(I meant 7000px not 700px)
(dammit , the actual stats are 10485px width/height on 96dpi is the failing size.)

Sorry for the spam.
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
btw, the image encoder throws this exception for large pixel data:

'InternalError: allocation size overflow'
(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/
(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.
Any thoughts on this, Joe ?
PS: refer comment 14 and comment 17
Flags: needinfo?(jwalker)
(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?
Flags: needinfo?(jwalker)
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.
(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.
Any suggestions here ?
Flags: needinfo?
(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?
Flags: needinfo?
(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 .
(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.
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).
i get this issue when trying to capture busy threads on image boards and IP.Board based communities.
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 :)
Assignee: scrapmachines → nobody
Whiteboard: [good-first-bugs][lang=js][mentor=Optimizer]
What exact values for max height/width must I use?
Also, please suggest a message.
Assignee: nobody → sachinhosmani2
Attachment #748428 - Flags: review?(scrapmachines)
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
Attachment #748428 - Flags: review?(scrapmachines) → feedback+
Attachment #748428 - Attachment is obsolete: true
Attachment #750005 - Flags: review?(scrapmachines)
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.
Attachment #750005 - Flags: review?(scrapmachines)
(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 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.
Attached patch Addresses the comments. (obsolete) — Splinter Review
I had to extend the test timeout as taking screenshots of pages this big seems to take some time and the test failed otherwise.
Attachment #750005 - Attachment is obsolete: true
Attachment #753936 - Flags: review?(scrapmachines)
(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 ?
Flags: needinfo?(jwalker)
(longer timeout try push with OOM exception) : https://tbpl.mozilla.org/?tree=Try&rev=7efc741790bd
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.
Attachment #753936 - Flags: review?(scrapmachines)
Flags: needinfo?(jwalker)
(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.
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 ?
Flags: needinfo?(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.
Flags: needinfo?(jwalker)
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 .
Whiteboard: [good-first-bugs][lang=js][mentor=Optimizer] → [good first bug][lang=js][mentor=Optimizer]
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]
Status: NEW → ASSIGNED
Resetting priorities because these P* levels are very out of date.
Sorry for the bug spam. Filter on Lobster Thermidor
Priority: P2 → --
(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.
Mentor: scrapmachines
Whiteboard: [good first bug][lang=js][mentor=Optimizer] → [good first bug][lang=js]
Mentor: scrapmachines
Duplicate of this bug: 1031579
We have related rendering problems in bug 1032498. Maybe workarounds that we consider here could also do for that bug?
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.
Duplicate of this bug: 993956
Duplicate of this bug: 965878
Assignee: sachinhosmani2 → nobody
Status: ASSIGNED → NEW
Flags: a11y-review+
relnote-firefox: ? → ---
Flags: a11y-review+
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.
The issue also happens when "Screenshot Node" (in Inspector) for an oversized element.
Component: Developer Tools: Graphic Commandline and Toolbar → Developer Tools
Summary: [devtb] "screenshot" command fails with long pages → screenshot will silent failure for oversized area
Whiteboard: [good first bug][lang=js]
Let's keep this in GCLI.  Under the covers, I believe the inspector's version also invokes GCLI as well.
Component: Developer Tools → Developer Tools: Graphic Commandline and Toolbar
Whiteboard: [polish-backlog][difficulty=medium]
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...
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
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
By "program still exists" I mean "*problem* still exists".
Product: Firefox → DevTools
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.
(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.
Duplicate of this bug: 1488449
Duplicate of this bug: 1342919
Component: Graphic Commandline and Toolbar → Shared Components
Summary: screenshot will silent failure for oversized area → Taking screenshots of long pages silently fails
Version: Trunk → unspecified

(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.

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

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.

Duplicate of this bug: 1599096
See Also: → 1615025
Duplicate of this bug: 1615010

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.

Component: Shared Components → General
Priority: -- → P2

(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.

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.

Assignee: nobody → pbrosset
Status: NEW → ASSIGNED

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.

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.

(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.

Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f216619d394a
Attempt to prevent screenshot failures and warn user on errors r=nchevobbe

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 - async
refresh@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.

Flags: needinfo?(pbrosset)

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

Honza

Assignee: patrickbrosset+bugzilla → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(patrickbrosset+bugzilla) → needinfo?(nchevobbe)

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

Flags: needinfo?(nchevobbe)
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Attachment #753936 - Attachment is obsolete: true
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/75df4b6a087f
Attempt to prevent screenshot failures and warn user on errors r=nchevobbe
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED

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.

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

QA Whiteboard: [qa-77b-p2]
Duplicate of this bug: 1615025
Regressions: 1635360

(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.

Flags: needinfo?(patrickbrosset+bugzilla)
Regressions: 1645284
Target Milestone: Future → Firefox 77
You need to log in before you can comment on or make changes to this bug.