Closed Bug 1378010 Opened 2 years ago Closed 2 years ago

Take a screenshot from the command line with headless

Categories

(Firefox :: Headless, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
relnote-firefox --- 57+
firefox57 --- fixed

People

(Reporter: bdahl, Assigned: ronoueb, NeedInfo)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

It'd be nice to easily take a screen shot of a web page from the command line using headless Firefox similar to what Chrome supports[1].

For example: `firefox --headless --screenshot http://google.com` would produce screenshot.png

A rough outline of what needs to be done:
 - check for the arg somewhere in nsAppRunner.cpp and set a flag (maybe in HeadlessWidget?) that we can later retrieve
 - then once the page is loaded (after first paint or some amount delay?) use drawWindow to save the png [3]
 - exit firefox

For finding spots where we could listen to the page loading I would follow firefox's handling of the URL command line argument and attach a listener somewhere along the way to the window.


[1] https://chromium.googlesource.com/chromium/src/+/lkgr/headless/app/headless_shell.cc#37

[2] http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/toolkit/xre/nsAppRunner.cpp

[3] http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/testing/marionette/capture.js#131
Not sure if it's of interest for headless, but you might get some ideas from DevTools' screenshot code[1] (triggered via inspector context menus, screenshot buttons, and GCLI commands).

Probably you can't use the code directly, but some features might be nice to borrow, such as:

1. Screenshot only specific elements using a CSS selector
2. Save to clipboard instead of file
3. Full page screenshot (entire height / width of the document, not just what is within the viewport)
4. Allows specifying a custom devicePixelRatio value to check the result for different monitor pixel densities
5. Allows specifying an arbitrary delay

[1]: http://searchfox.org/mozilla-central/source/devtools/shared/gcli/commands/screenshot.js
Some Chrome defaults:
- window size 800x600
- screenshot.png goes into working directory
- captures with scroll bars
There doesn't seem to be a way to make Chrome capture the whole page.

Our devtools will capture the whole page if you use the screenshot button from the toolbox and the screenshots will go in the download manager.
Comment on attachment 8887182 [details]
Bug 1378010 - screenshot from command line with headless;

https://reviewboard.mozilla.org/r/157930/#review163700

Looks really good overall. Just a few little things below (mainly making takeScreenshot more like synchronous js). Feel free to grab me if any of it's unclear! I also have a few slight feature changes after talking with Myk, but I'll leave that comment in the bug.

::: browser/components/nsBrowserContentHandler.js:454
(Diff revision 1)
>      let info =
>                "  --browser          Open a browser window.\n" +
>                "  --new-window <url> Open <url> in a new window.\n" +
>                "  --new-tab <url>    Open <url> in a new tab.\n" +
>                "  --private-window <url> Open <url> in a new private window.\n";
>      if (AppConstants.platform == "win") {

Let's add documentation for the new flags up here. We'll also need to wrap it an if and only show on linux/windows. We don't need to do this now, but we should expose a flag for if headless is supported (I'll file a bug).

::: browser/components/shell/HeadlessShell.jsm:49
(Diff revision 1)
> +    webProgress.addProgressListener(progressListener,
> +                                    Ci.nsIWebProgress.NOTIFY_LOCATION);
> +  });
> +}
> +
> +async function takeScreenshot(fullpage, contentWidth, contentHeight, path, url) {

We don't really use the result of this function below, but it is a bit odd that it resolves before the screenshot may actually be saved.

::: browser/components/shell/HeadlessShell.jsm:52
(Diff revision 1)
> +}
> +
> +async function takeScreenshot(fullpage, contentWidth, contentHeight, path, url) {
> +  // Don't quit even though we don't create a window.
> +  Services.startup.enterLastWindowClosingSurvivalArea();
> +

Start regular a try/catch/finally here.

::: browser/components/shell/HeadlessShell.jsm:68
(Diff revision 1)
> +                        : contentWindow.innerHeight;
> +  canvas.width = width;
> +  canvas.height = height;
> +  context.drawWindow(contentWindow, 0, 0, width, height, 'rgb(255, 255, 255)');
> +
> +  canvas.toBlob((b) => {

Do an await new Promise(...) here that resolves when the file is done writing.

::: browser/components/shell/HeadlessShell.jsm:75
(Diff revision 1)
> +    reader.onloadend = function() {
> +      OS.File.writeAtomic(path, new Uint8Array(reader.result), {flush:true})
> +      .then(() => dump('Screenshot saved to: ' + path + '\n'))
> +      .catch((reason) => dump('Failure saving screenshot: ' + reason + '\n'))
> +      .then(() => { // "finally"
> +        webNavigation.close();

Move .close into the finally and exitLastWindowClosingSurvivalArea after the finally.

::: browser/components/shell/HeadlessShell.jsm:113
(Diff revision 1)
> +        if (dimensions[0] > 0 && dimensions[1] > 0) {
> +          fullpage = false;
> +          contentWidth = dimensions[0];
> +          contentHeight = dimensions[1];
> +        } else {
> +          throw 0; // Ill-formed `window-size`

I recommend always using throw new Error(....) so a stack gets attached to it.

::: browser/components/shell/test/test_headless_screenshot.html:86
(Diff revision 1)
> +    let proc = await Subprocess.call({
> +      command: firefoxExe,
> +      arguments: firefoxArgs.concat(["--window-size=hello"]),
> +    });
> +    let str;
> +    let foundErr;

I originally had said let's test for the error message, but after thinking about it some more, we should really just check the behavior and make sure a screenshot doesn't exist. That way we don't have to hardcode string error messages in two places.
A few slight modifications after talking with Myk:

`firefox --screenshot` should imply headless, so let's check for it in nsAppRunner[1] and enable headless if present.

`firefox --screenshot --window-size=800` should set the width but do a fullpage screenshot

[1] http://searchfox.org/mozilla-central/rev/88180977d79654af025158d8ebeb8c2aa11940eb/toolkit/xre/nsAppRunner.cpp#3170
Assignee: nobody → ronoueb
Addressed review comments and added some more tests. Try is closed right now, so I don't know if changing about:mozilla to about:blank helps.
Here's a try submission with the most recent changes https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce5c76344bf8abbda29fcb6f08b3aa0124f343b9
Comment on attachment 8887182 [details]
Bug 1378010 - screenshot from command line with headless;

https://reviewboard.mozilla.org/r/157930/#review169460

::: browser/components/nsBrowserContentHandler.js:462
(Diff revision 3)
> +      info += "  --screenshot[=<path>]        Save screenshot of page to <path> or in working directory.\n";
> +      info += "  --window-size=width[,height] Width of full page screenshot or width and height of region to screenshot.\n";

To be consistent with above, let's show these options in the generic form e.g. --screenshot [<path>]

::: browser/components/nsBrowserContentHandler.js
(Diff revision 3)
>        if (URLlist.length) {
>          openWindow(null, gBrowserContentHandler.chromeURL, "_blank",
>                     "chrome,dialog=no,all" + gBrowserContentHandler.getFeatures(cmdLine),
>                     URLlist);
>        }
> -

Revert unchanged line

::: browser/components/shell/HeadlessShell.jsm:57
(Diff revision 3)
> +
> +  try {
> +    let windowlessBrowser = Services.appShell.createWindowlessBrowser(false);
> +    var webNavigation = windowlessBrowser.QueryInterface(Ci.nsIWebNavigation);
> +    let contentWindow = await loadContentWindow(webNavigation, url);
> +    contentWindow.resizeTo(contentWidth, contentHeight);

We don't need to fix this now, but can you file a follow up bug to add parameters to set the initial size of the browserless window so we don't have to resize?

::: browser/components/shell/test/chrome.ini:2
(Diff revision 3)
> +[test_headless_screenshot.html]
> +skip-if = (os != 'win' && os != 'linux')

Please file a follow up bug to enable this for macos.

::: browser/components/shell/test/test_headless_screenshot.html:26
(Diff revision 3)
> +  const mochiPrefsPath = mochiPrefsFile.path;
> +  const mochiPrefsName = mochiPrefsFile.leafName;
> +  const profilePath = OS.Path.join(OS.Constants.Path.tmpDir, "headless_test_screenshot_profile");
> +  const prefsPath = OS.Path.join(profilePath, mochiPrefsName);
> +  const screenshotPath = OS.Path.join(OS.Constants.Path.tmpDir, "headless_test_screenshot.png");
> +  const firefoxArgs = ["-profile", profilePath, "-no-remote", "-url", "about:blank", "-screenshot", screenshotPath];

Let's add a few more simple tests for those tricky edge cases we discussed.

e.g.
--screenshot about:blank (no --url and no path)
--screenshot (no --url and no optional url)
Attachment #8887182 - Flags: review?(bdahl) → review-
Comment on attachment 8887182 [details]
Bug 1378010 - screenshot from command line with headless;

https://reviewboard.mozilla.org/r/157930/#review169460

> We don't need to fix this now, but can you file a follow up bug to add parameters to set the initial size of the browserless window so we don't have to resize?

https://bugzilla.mozilla.org/show_bug.cgi?id=1387585
Comment on attachment 8887182 [details]
Bug 1378010 - screenshot from command line with headless;

https://reviewboard.mozilla.org/r/157930/#review169460

> Please file a follow up bug to enable this for macos.

https://bugzilla.mozilla.org/show_bug.cgi?id=1387587
Attachment #8887182 - Flags: review?(bdahl) → review?(dtownsend)
Comment on attachment 8887182 [details]
Bug 1378010 - screenshot from command line with headless;

https://reviewboard.mozilla.org/r/157930/#review177002

A few questions and comments here.

Does this (or should this) support taking screenshots with a currently running instance of Firefox?

::: browser/components/nsBrowserContentHandler.js:8
(Diff revision 4)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  Components.utils.import("resource://gre/modules/AppConstants.jsm");
> +Components.utils.import("resource:///modules/HeadlessShell.jsm");

Please import this lazily

::: browser/components/nsBrowserContentHandler.js:462
(Diff revision 4)
> +      info += "  --screenshot [<path>]        Save screenshot of page to <path> or in working directory.\n";
> +      info += "  --window-size width[,height] Width of full page screenshot or width and height of region to screenshot.\n";

Make these output lines less than 78 characters.

::: browser/components/nsBrowserGlue.js:75
(Diff revision 4)
>    ["DirectoryLinksProvider", "resource:///modules/DirectoryLinksProvider.jsm"],
>    ["ExtensionsUI", "resource:///modules/ExtensionsUI.jsm"],
>    ["Feeds", "resource:///modules/Feeds.jsm"],
>    ["FileUtils", "resource://gre/modules/FileUtils.jsm"],
>    ["FormValidationHandler", "resource:///modules/FormValidationHandler.jsm"],
> +  ["HeadlessShell", "resource:///modules/HeadlessShell.jsm"],

Why did you need to add this here?

::: browser/components/shell/HeadlessShell.jsm:32
(Diff revision 4)
> +        docShell = webNavigation.QueryInterface(Ci.nsIInterfaceRequestor)
> +                   .getInterface(Ci.nsIDocShell);

I don't think the docshell will have changed

::: browser/components/shell/HeadlessShell.jsm:35
(Diff revision 4)
> +          return;
> +        }
> +        docShell = webNavigation.QueryInterface(Ci.nsIInterfaceRequestor)
> +                   .getInterface(Ci.nsIDocShell);
> +        let contentWindow = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                            .getInterface(Ci.nsIDOMWindow);

Line up the periods please.

::: browser/components/shell/HeadlessShell.jsm:69
(Diff revision 4)
> +    await new Promise((resolve, reject) => {
> +      canvas.toBlob((blob) => {
> +        let reader = new FileReader();
> +        reader.onloadend = function() {
> +          OS.File.writeAtomic(path, new Uint8Array(reader.result), {flush: true})
> +          .then(() => dump("Screenshot saved to: " + path + "\n"))
> +          .catch((reason) => dump("Failure saving screenshot: " + reason + "\n"))
> +          .then(() => resolve()); // "finally"
> +        };
> +        reader.readAsArrayBuffer(blob);
> +      });
> +    });

We're in an async function so it would be nicer to make a few small functions that wrap the toBlob and reader in ways that return promises that you can just await on.

Something like:

    function promiseCanvasBlob(canvas) {
        return new Promise(resolve => canvas.toBlob(resolve);
    }
    
    function readBlob(blob) {
      return new Promise(resolve => {
        let reader = new FileReader();
        reader.onloadend = () => resolve(reader);
        reader.readAsArrayBuffer(blob);
      });
    }

::: browser/components/shell/HeadlessShell.jsm:97
(Diff revision 4)
> +    let contentWidth = 1366;
> +    let contentHeight = 768;

Where do these numbers come from?

::: browser/components/shell/HeadlessShell.jsm:105
(Diff revision 4)
> +    // Parse `window-size`
> +    try {
> +      var dimensionsStr = cmdLine.handleFlagWithParam("window-size", true);
> +    } catch (e) {
> +      dump("expected format: --window-size width[,height]\n");
> +      return;

Does this cause the process to exit? I'm not seeing how

::: browser/components/shell/HeadlessShell.jsm:138
(Diff revision 4)
> +    // Only command line argument left should be `screenshot`
> +    // There could still be URLs however
> +    try {
> +      var path = cmdLine.handleFlagWithParam("screenshot", true);
> +      if (!cmdLine.length && !URLlist.length) {
> +        URLlist.push(path); // Assume the user wanted to specify a URL

This is a bit weird and I wouldn't want to box us into a corner of supporting this in the future. In this case just log a message saying there was nothing to screenshot and exit.

::: toolkit/xre/nsAppRunner.cpp:574
(Diff revision 4)
>  
>    return ar;
>  }
>  
> +/**
> + * Check for a commandline flag. Ignore parameters of the form -arg <data> and -arg=<data>.

It doesn't seem to ignore these parameters. Rather it still matches even if there is a delimeter there regardless of the data past it.

::: toolkit/xre/nsAppRunner.cpp:3214
(Diff revision 4)
>  
>    if (ChaosMode::isActive(ChaosFeature::Any)) {
>      printf_stderr("*** You are running in chaos test mode. See ChaosMode.h. ***\n");
>    }
>  
> -  if (CheckArg("headless")) {
> +  if (CheckArg("headless") || CheckArgExists("screenshot")) {

Do we actually need to test this here? Should screenshot always imply headless? We could just require users to also pass the headless argument if they don't want to see the UI when taking a screenshot.
Attachment #8887182 - Flags: review?(dtownsend)
(In reply to Dave Townsend [:mossop] from comment #15)
> This is a bit weird and I wouldn't want to box us into a corner of
> supporting this in the future. In this case just log a message saying there
> was nothing to screenshot and exit.

If I understand it correctly, this code enables us to support both the optional path parameter to the --screenshot flag and the implicit URL parameter (i.e. the `[URL]` in `firefox [ options ... ] [URL]`).

From a product perspective, I'd like to continue supporting the implicit URL parameter, which is consistent with both existing Firefox and with the Chrome implementation of the --screenshot flag <https://developers.google.com/web/updates/2017/04/headless-chrome#screenshots>.

I also think the optional path parameter is a useful addition to the feature (and a nice differentiator vis-a-vis Chrome).  So I would prefer to support both, if that's possible.

If the heuristic differentiation between a URL and a path is too complex/brittle from an implementation standpoint, however, and we need to make the specification of the URL and/or path more explicit, then my preference would be to require explicit specification of the path, while continuing to allow implicit URLs.

Explicit specification of the path could be:

  --screenshot-path <path>

Or:

  --screenshot=path

Or another such format that makes it possible (but optional) to specify a path for the screenshot file.  I don't have a strong preference.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #16)
> (In reply to Dave Townsend [:mossop] from comment #15)
> > This is a bit weird and I wouldn't want to box us into a corner of
> > supporting this in the future. In this case just log a message saying there
> > was nothing to screenshot and exit.
> 
> If I understand it correctly, this code enables us to support both the
> optional path parameter to the --screenshot flag and the implicit URL
> parameter (i.e. the `[URL]` in `firefox [ options ... ] [URL]`).

That matches my understanding so:

    firefox --screenshot foo

will take a screenshot of site foo

    firefox --screenshot foo bar

will take a screenshot of site bar called foo.

My opinion is that the first case should be consistent with the second and gave an error about no url being given.

> From a product perspective, I'd like to continue supporting the implicit URL
> parameter, which is consistent with both existing Firefox and with the
> Chrome implementation of the --screenshot flag
> <https://developers.google.com/web/updates/2017/04/headless-
> chrome#screenshots>.

Ah, I didn't realise we're trying to clone a Chrome behaviour here.

> I also think the optional path parameter is a useful addition to the feature
> (and a nice differentiator vis-a-vis Chrome).  So I would prefer to support
> both, if that's possible.
> 
> If the heuristic differentiation between a URL and a path is too
> complex/brittle from an implementation standpoint, however, and we need to
> make the specification of the URL and/or path more explicit, then my
> preference would be to require explicit specification of the path, while
> continuing to allow implicit URLs.
> 
> Explicit specification of the path could be:
> 
>   --screenshot-path <path>
> 
> Or:
> 
>   --screenshot=path
> 
> Or another such format that makes it possible (but optional) to specify a
> path for the screenshot file.  I don't have a strong preference.

I defer to your judgement as this is really a product call.
Thanks for your input here, Myk.
(In reply to Dave Townsend [:mossop] from comment #15)
> Comment on attachment 8887182 [details]
> Bug 1378010 - screenshot from command line with headless;
> 
> https://reviewboard.mozilla.org/r/157930/#review177002
> 
> A few questions and comments here.
> 
> Does this (or should this) support taking screenshots with a currently
> running instance of Firefox?
> 
It doesn't. We always designed this feature to be like Chrome's (which also takes the screenshot and exits). As for if it should, maybe that's for Brendan or Myk to answer.
> ::: browser/components/shell/HeadlessShell.jsm:97
> (Diff revision 4)
> > +    let contentWidth = 1366;
> > +    let contentHeight = 768;
> 
> Where do these numbers come from?
> 
It's the most common resolution of Firefox users IIRC. I'll add a code comment.
> ::: browser/components/shell/HeadlessShell.jsm:105
> (Diff revision 4)
> > +    // Parse `window-size`
> > +    try {
> > +      var dimensionsStr = cmdLine.handleFlagWithParam("window-size", true);
> > +    } catch (e) {
> > +      dump("expected format: --window-size width[,height]\n");
> > +      return;
> 
> Does this cause the process to exit? I'm not seeing how
> 
Yes, but it's a little tricky. When handleCmdLineArgs returns, there is an early return in nsBrowserContentHandler.js:732. Since no browser windows exist and the early return in nsBrowserContentHandler prevents one from being created, the process exits.
> ::: toolkit/xre/nsAppRunner.cpp:574
> (Diff revision 4)
> >  
> >    return ar;
> >  }
> >  
> > +/**
> > + * Check for a commandline flag. Ignore parameters of the form -arg <data> and -arg=<data>.
> 
> It doesn't seem to ignore these parameters. Rather it still matches even if
> there is a delimeter there regardless of the data past it.
> 
This is possibly bad wording on my behalf. CheckArg and the other ways to check command line arguments in nsAppRunner, will say that --screenshot is not present if you do --screenshot=<data>, since they consider all of that as one command line argument. Instead of changing the behavior of those functions, CheckArgExists was introduced. That comment considers just the delimiter and the data that follows as parameters (and --screenshot as a flag). Maybe it should be reworded as "Ignore data that's passed in with the flag"?
> ::: toolkit/xre/nsAppRunner.cpp:3214
> (Diff revision 4)
> >  
> >    if (ChaosMode::isActive(ChaosFeature::Any)) {
> >      printf_stderr("*** You are running in chaos test mode. See ChaosMode.h. ***\n");
> >    }
> >  
> > -  if (CheckArg("headless")) {
> > +  if (CheckArg("headless") || CheckArgExists("screenshot")) {
> 
> Do we actually need to test this here? Should screenshot always imply
> headless? We could just require users to also pass the headless argument if
> they don't want to see the UI when taking a screenshot.

It doesn't actually work if not in headless mode. Again, I don't know if it should, but I think Brendan and Myk decided that screenshot should imply headless, so let's hear what they think :)
Flags: needinfo?(bdahl)
(In reply to B. Dahse from comment #18)
> Thanks for your input here, Myk.
> (In reply to Dave Townsend [:mossop] from comment #15)
> > Comment on attachment 8887182 [details]
> > Bug 1378010 - screenshot from command line with headless;
> > 
> > https://reviewboard.mozilla.org/r/157930/#review177002
> > 
> > A few questions and comments here.
> > 
> > Does this (or should this) support taking screenshots with a currently
> > running instance of Firefox?
> > 
> It doesn't. We always designed this feature to be like Chrome's (which also
> takes the screenshot and exits). As for if it should, maybe that's for
> Brendan or Myk to answer.

Have you tested to see what happens in that case? I think it will actually work since when you run firefox any command line arguments simply get passed along to a running instance if there is one. Unless headless disables remoting I guess.

> > ::: browser/components/shell/HeadlessShell.jsm:105
> > (Diff revision 4)
> > > +    // Parse `window-size`
> > > +    try {
> > > +      var dimensionsStr = cmdLine.handleFlagWithParam("window-size", true);
> > > +    } catch (e) {
> > > +      dump("expected format: --window-size width[,height]\n");
> > > +      return;
> > 
> > Does this cause the process to exit? I'm not seeing how
> > 
> Yes, but it's a little tricky. When handleCmdLineArgs returns, there is an
> early return in nsBrowserContentHandler.js:732. Since no browser windows
> exist and the early return in nsBrowserContentHandler prevents one from
> being created, the process exits.

Ok I thought it might be something like that but it's a little non-obvious. Given that this is an async function it might be tempting to await something before the return which presumably would cause the app to exit early. Can you wrap the whole function in a last window closing survival block to ensure that never happens, using a try...finally block should do it.

> > ::: toolkit/xre/nsAppRunner.cpp:574
> > (Diff revision 4)
> > >  
> > >    return ar;
> > >  }
> > >  
> > > +/**
> > > + * Check for a commandline flag. Ignore parameters of the form -arg <data> and -arg=<data>.
> > 
> > It doesn't seem to ignore these parameters. Rather it still matches even if
> > there is a delimeter there regardless of the data past it.
> > 
> This is possibly bad wording on my behalf. CheckArg and the other ways to
> check command line arguments in nsAppRunner, will say that --screenshot is
> not present if you do --screenshot=<data>, since they consider all of that
> as one command line argument. Instead of changing the behavior of those
> functions, CheckArgExists was introduced. That comment considers just the
> delimiter and the data that follows as parameters (and --screenshot as a
> flag). Maybe it should be reworded as "Ignore data that's passed in with the
> flag"?

Yeah that wording sounds better.
(In reply to Dave Townsend [:mossop] from comment #19)
> (In reply to B. Dahse from comment #18)
> > Thanks for your input here, Myk.
> > (In reply to Dave Townsend [:mossop] from comment #15)
> > > Comment on attachment 8887182 [details]
> > > Bug 1378010 - screenshot from command line with headless;
> > > 
> > > https://reviewboard.mozilla.org/r/157930/#review177002
> > > 
> > > A few questions and comments here.
> > > 
> > > Does this (or should this) support taking screenshots with a currently
> > > running instance of Firefox?
> > > 
> > It doesn't. We always designed this feature to be like Chrome's (which also
> > takes the screenshot and exits). As for if it should, maybe that's for
> > Brendan or Myk to answer.
> 
> Have you tested to see what happens in that case? I think it will actually
> work since when you run firefox any command line arguments simply get passed
> along to a running instance if there is one. Unless headless disables
> remoting I guess.
> 
I haven't tested it, in fact I didn't know it works like that. In any case, starting one instance with
/home/bdahse/src/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/bin/firefox -profile /home/bdahse/src/mozilla-unified/obj-x86_64-pc-linux-gnu/tmp/scratch_user --headless google.com
and another with
/home/bdahse/src/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/bin/firefox -profile /home/bdahse/src/mozilla-unified/obj-x86_64-pc-linux-gnu/tmp/scratch_user --screenshot=remote.png
gives "Firefox is already running, but is not responding. To open a new window, you must first close the existing Firefox process, or restart your system."

But I'll get started on addressing those review comments of yours that don't need input from elsewhere.
Comment on attachment 8887182 [details]
Bug 1378010 - screenshot from command line with headless;

https://reviewboard.mozilla.org/r/157930/#review177002

> Why did you need to add this here?

I don't know. Removing it doesn't seem to have any effect on the tests so let's keep it removed.
Comment on attachment 8887182 [details]
Bug 1378010 - screenshot from command line with headless;

https://reviewboard.mozilla.org/r/157930/#review177002

> We're in an async function so it would be nicer to make a few small functions that wrap the toBlob and reader in ways that return promises that you can just await on.
> 
> Something like:
> 
>     function promiseCanvasBlob(canvas) {
>         return new Promise(resolve => canvas.toBlob(resolve);
>     }
>     
>     function readBlob(blob) {
>       return new Promise(resolve => {
>         let reader = new FileReader();
>         reader.onloadend = () => resolve(reader);
>         reader.readAsArrayBuffer(blob);
>       });
>     }

Yes, that's better. Nice one!
After a conversation with Myk, I changed multiple URLs to be an error instead of silently picking the first one. I also added two more test cases:
1. Test that "firefox [implicit url] -screenshot" and "firefox -screenshot [implicit url]" behave the same
2. Test that multiple URLs is an error
Comment on attachment 8887182 [details]
Bug 1378010 - screenshot from command line with headless;

https://reviewboard.mozilla.org/r/157930/#review179276

Looks good.
Attachment #8887182 - Flags: review?(dtownsend) → review+
Screenshot only specific elements using a CSS selector +1
Don't think we have any plans for that in this feature. Maybe it's possible with our devtools screenshot (as mentioned in comment 1) and remote debugging?
Flags: needinfo?(jryans)
(In reply to B. Dahse from comment #28)
> Don't think we have any plans for that in this feature. Maybe it's possible
> with our devtools screenshot (as mentioned in comment 1) and remote
> debugging?

Within the Firefox UI, you can use the Developer Toolbar / GCLI[1] and enter something like:

```
screenshot --selector <selector>
```

to save it to a file.  There are other options for clipboard, etc.  Check `help screenshot`.

You can also right click a node in the DevTools inspector and choose "Screenshot Node".

The same actions could be triggered by a script connecting to the remote debugging port, but I am not aware of existing tools that do that today.

[1]: https://developer.mozilla.org/en-US/docs/Tools/GCLI
Flags: needinfo?(jryans)
Changed the test to screenshot a support file instead of about:blank, because of a leak when screenshotting about:blank.
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(ronoueb)
Keywords: checkin-needed
I see, should be no open issues in MozReview now.
Flags: needinfo?(ronoueb)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2ce3e68d7242
screenshot from command line with headless; r=mossop
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2ce3e68d7242
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Release Note Request (optional, but appreciated)
[Why is this notable]: Great new feature
[Affects Firefox for Android]: No
[Suggested wording]: Take a screenshot from the command line with --screenshot=/path/to/file
[Links (documentation, blog post, etc)]: no yet
Is there an API to tap into these features from other languages, like PHP, and NodeJS?
(In reply to sitelease from comment #37)
> Is there an API to tap into these features from other languages, like PHP,
> and NodeJS?

There isn't an API for the screenshot feature specifically.  For headless mode generally, however, you can use the WebDriver API, which supports a variety of languages (including Node.js) and includes a method for taking a screenshot.  See its documentation:

https://developer.mozilla.org/en-US/Firefox/Headless_mode

Also see this blog post, which gives an example of taking a screenshot using the Node.js API to WebDriver:

https://mykzilla.org/2017/08/30/headless-firefox-in-node-js-with-selenium-webdriver/
Flags: needinfo?(bdahl)
I've had a go at documenting this. I've added this section to the headless mode doc:

https://developer.mozilla.org/en-US/Firefox/Headless_mode#Taking_screenshots

I've also added a note to the Fx57 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/57#Other

Can you have a look and let me know if this reads ok?

A couple of other points:

* I couldn't get the screenshot=path or screenshot-path=path type stuff mentioned in comment 16 to work — how do you specify a custom path to save the screenshot at?
* On Mac at least, the single dash version of window-size doesn't seem to work, e.g. -window-size=x. I had to use --window-size=x. This seems a little inconsistent, given that -headless and -screenshot work?
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #39)
> I've had a go at documenting this. I've added this section to the headless
> mode doc:
> 
> https://developer.mozilla.org/en-US/Firefox/Headless_mode#Taking_screenshots
> 
> I've also added a note to the Fx57 rel notes:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/57#Other
> 
> Can you have a look and let me know if this reads ok?

Both of these look great.


> A couple of other points:
> 
> * I couldn't get the screenshot=path or screenshot-path=path type stuff
> mentioned in comment 16 to work — how do you specify a custom path to save
> the screenshot at?

Specifying the path to the file without an equals sign works:

> /path/to/firefox -screenshot path/to/name.png https://developer.mozilla.com

But the version with the equals sign should also work.  I filed bug 1406590 on that.


> * On Mac at least, the single dash version of window-size doesn't seem to
> work, e.g. -window-size=x. I had to use --window-size=x. This seems a little
> inconsistent, given that -headless and -screenshot work?

Indeed, it's very inconsistent.  I filed bug 1406589 on this.
Depends on: 1331152
Following up on the leak in bug 1331152, I looked at the code that was landed in this bug and I see [1] has been added in bug 1403934. Were the linux64-asan builds fine after that change?

[1]: http://searchfox.org/mozilla-central/source/browser/components/shell/HeadlessShell.jsm#159
Flags: needinfo?(bdahl)
(In reply to B. Dahse from comment #41)
> Following up on the leak in bug 1331152, I looked at the code that was
> landed in this bug and I see [1] has been added in bug 1403934. Were the
> linux64-asan builds fine after that change?

Yeah, that could cause it. For leak checking to work, you have to be really patient with shutdown. I see this comment in nsAppStartup::Quit(uint32_t aMode):
      // no matter what, make sure we send the exit event.  If
      // worst comes to worst, we'll do a leaky shutdown but we WILL
      // shut down. Well, assuming that all *this* stuff works ;-).

Maybe something related to this is leaking? It is a very small leak of some JS stuff. Maybe that nsAppExitEvent stuff could hold alive some JS stuff?
(In reply to Andrew McCreight [:mccr8] from comment #42)
> Yeah, that could cause it. For leak checking to work, you have to be really
> patient with shutdown. I see this comment in nsAppStartup::Quit(uint32_t
> aMode):
>       // no matter what, make sure we send the exit event.  If
>       // worst comes to worst, we'll do a leaky shutdown but we WILL
>       // shut down. Well, assuming that all *this* stuff works ;-).
> 
> Maybe something related to this is leaking? It is a very small leak of some
> JS stuff. Maybe that nsAppExitEvent stuff could hold alive some JS stuff?

I was wondering about that too and changed eForceQuit to the kinder, gentler eAttemptQuit in this tryserver run a couple days ago:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbf71fe31964725079836fd1f7908a340d52994e

Unfortunately, it didn't resolve the problem.  Perhaps eAttemptQuit still doesn't make nsAppStartup::Quit patient enough?
One temporary fix would be to disable LSan on the process by adding detect_leaks=0 to ASAN_OPTIONS.
Depends on: 1507352
You need to log in before you can comment on or make changes to this bug.