Taking screenshots of long pages silently fails
Categories
(DevTools :: General, defect, P2)
Tracking
(firefox77 fixed)
Tracking | Status | |
---|---|---|
firefox77 | --- | fixed |
People
(Reporter: RNicoletto, Assigned: nchevobbe)
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.
Updated•12 years ago
|
Reporter | ||
Comment 1•12 years ago
|
||
Can this be related to bug 591822?
Updated•12 years ago
|
Comment 2•11 years ago
|
||
My toolbar doesnt accept "screenshot image 0 true" it only recognizes "screenshot image"
Comment 3•11 years ago
|
||
(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•11 years ago
|
||
I think comment 1 is correct. It is a D2D issue. Joe, what should the approach be ?
Comment 5•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
We could take partial pictures and concatenate them.
Comment 9•11 years ago
|
||
(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•11 years ago
|
||
This bug was not fixed with Firefox 17.
Comment 11•11 years ago
|
||
@Jonathan Vieyra : Are you still planning to fix this bug ?
Comment 12•11 years ago
|
||
@Girish Sharma Not at the moment, Ive chosen a different bug for my project.
Comment 14•11 years ago
|
||
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•11 years ago
|
||
(I meant 7000px not 700px)
Comment 16•11 years ago
|
||
(dammit , the actual stats are 10485px width/height on 96dpi is the failing size.) Sorry for the spam.
Comment 17•11 years ago
|
||
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•11 years ago
|
||
btw, the image encoder throws this exception for large pixel data: 'InternalError: allocation size overflow'
Comment 19•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
Any thoughts on this, Joe ? PS: refer comment 14 and comment 17
Comment 22•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
(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 26•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
i get this issue when trying to capture busy threads on image boards and IP.Board based communities.
Comment 32•11 years ago
|
||
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•11 years ago
|
||
What exact values for max height/width must I use? Also, please suggest a message.
Comment 34•11 years ago
|
||
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•11 years ago
|
||
Comment 36•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
(longer timeout try push with OOM exception) : https://tbpl.mozilla.org/?tree=Try&rev=7efc741790bd
Comment 42•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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 ?
Updated•11 years ago
|
Comment 45•11 years ago
|
||
(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•11 years ago
|
||
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 .
Updated•11 years ago
|
Comment 47•11 years ago
|
||
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]
Updated•10 years ago
|
Comment 48•10 years ago
|
||
Resetting priorities because these P* levels are very out of date. Sorry for the bug spam. Filter on Lobster Thermidor
Comment 49•10 years ago
|
||
(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.
Updated•10 years ago
|
Updated•10 years ago
|
Comment 51•10 years ago
|
||
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•10 years ago
|
||
We have related rendering problems in bug 1032498. Maybe workarounds that we consider here could also do for that bug?
Comment 53•10 years ago
|
||
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.
Updated•9 years ago
|
Comment hidden (spam) |
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•9 years ago
|
||
The issue also happens when "Screenshot Node" (in Inspector) for an oversized element.
Comment 61•9 years ago
|
||
Let's keep this in GCLI. Under the covers, I believe the inspector's version also invokes GCLI as well.
Comment 64•9 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
By "program still exists" I mean "*problem* still exists".
Updated•6 years ago
|
Comment 74•6 years ago
|
||
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•6 years ago
|
||
(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.
Updated•6 years ago
|
Comment 78•5 years ago
|
||
(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•5 years ago
•
|
||
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
- Open in a new tab:
data:text/html,<body style="background:pink"></body>
- Open Responsive Design Mode, and set the RDM viewport to width=1000, height=50
- In Console, run
:screenshot --dpr 32.767
- 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.
- Open in a new tab:
data:text/html,<body style="background:pink"></body>
- Open Responsive Design Mode, and set the RDM viewport to width=1000, height=1000
- Run
:screenshot --dpr 11.18
- 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
Assignee | ||
Comment 80•5 years ago
|
||
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•4 years ago
|
||
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.
Updated•4 years ago
|
Comment 84•4 years ago
•
|
||
(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•4 years ago
|
||
The changes made here are meant to make the screenshot code in DevTools
closer to how the Firefox Screenshots addon works.
- It cuts off large images to 10000x10000
- 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.
Updated•4 years ago
|
Comment 86•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
•
|
||
(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•4 years ago
|
||
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
Comment 90•4 years ago
|
||
Backed out for failures on browser_jsterm_screenshot_command_warnings.js
backout: https://hg.mozilla.org/integration/autoland/rev/c79d03d2790b674b4e07aee2b95e20bd350e0bbc
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•4 years ago
|
||
Nicolas, do you think we could fix the test failure?
Honza
Assignee | ||
Comment 92•4 years ago
|
||
yes, we should try to move that over the finish line.
I'm going to have a look
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Comment 93•4 years ago
|
||
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
Comment 94•4 years ago
|
||
bugherder |
Comment 95•4 years ago
|
||
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•4 years ago
|
||
Thanks a lot Patrick for starting this patch. One less papercut!
Updated•4 years ago
|
(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.
Updated•4 years ago
|
Updated•3 years ago
|
Description
•