673383 - Output stdout in regular command line on Windows

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Alexandre Poirot [:ochameau]]

Currently we can build command line application using Firefox or Xulrunner through a XUL application with an application.ini and an XPCOM to catch command line arguments.
But this approach has multiple issues:
 - you can't control stdin,
 - it is not a real command line application on window, due to the temporary stdout windows displayed when you pass `-console` argument
(stdout is not displayed in you command line terminal),
 - you have a profile, that gives a heavyweight process.

So we may be able to build something looking like a command line application, but xpcshell is an obvious way to build a real one.

My first idea was to ship xpcshell binary with Firefox but ted suggested a better option:
Adding a `-shell /path/to/js/file` option in Firefox that executes the given script as an xpcshell script. It is close to the `-app` option but running only a script instead of a full XUL application.

I'm currently working on providing a command line tool for Jetpack using Jetpack itself. So that the only dependency of jetpack to develop an addon is Firefox and one Addon.
I already have built a command line application that may replace our python tool called `cfx`. My issue is that it only works with XPCshell on Windows due to the lack of real STDOUT for xul application. And release builds do not ship xpcshell, so we can't rely on it :(

Comment 1

[[e-mail]]

(In reply to comment #0)
>  - it is not a real command line application on window, due to the temporary
> stdout windows displayed when you pass `-console` argument
firefox -shell would also not be a command line application on Windows.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Use AttachConsole

I finally figured out with Vivien's help that we can actually fix the current behavior on Firefox so that we do not need to go though a XPCShell script.

So, currently, the only way to print something to the command line where we launched our program is to use xpcshell. 
If you use firefox, via a xulrunner application (-app argument) or just regular browser; any call to dump() is redirected to the temporary window where all logs go.
The issue with that window is that it prevents building a regular command line application on Windows, mainly because stdout disappear on program exit.

But we found the method that allow to fix this weakness in our Windows support:
AttachConsole:
  http://msdn.microsoft.com/en-us/library/ms681952%28v=vs.85%29.aspx

So in this patch I propose to add this call to AttachConsole:
 - If this request worked, we just ignore /console or -console argument and output stdout to the command line where we launched firefox.
 - Else, AttachConsole may return a false value when the parent process is not a command line, i.e. when firefox is launched from an icon. Then we fall back to the -console argument and allow displaying this temporary window.

Comment 3

[Mounir Lamouri (:mounir)]

Sent to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=55ca3a7b2a62

Comment 4

[Mike Hommey [:glandium]]

Hmmm. I still think a -shell option would be interesting to have.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 557149 [details] [diff] [review]
Use AttachConsole

This looks fairly sane. One nit: use spaces, not tabs for indentation. This could also use a short comment, like "try to use the console the application was started in."

What's the freopen for? Are stdout/stderr not open when we don't have a console initially? The _get_osfhandle call worries me a little, since it says it can call the invalid parameter handler when it fails:
http://msdn.microsoft.com/en-us/library/ks2530z6.aspx

Also I agree with Mike that the original premise of this bug is still useful, but this is a good fix.

Comment 6

[Alexandre Poirot [:ochameau]]

The test with _get_osfhandle is here to avoid redirecting stdout, if, for some reason it has been redirected to a valid file.

To have a better understanding of these windows specific things,
you can give a look at these pages:
  http://dslweb.nwnexus.com/~ast/dload/guicon.htm
  http://support.microsoft.com/kb/105305

If I got it right, we have to update stdout/stderr/stdin references in order to make work all standard file libraries that rely on those. (printf, fputs)
Otherwise, by default, stdout & all seems to refer to useless values.
But all win32 code that get STDOUT reference through GetStdHandle(STD_OUTPUT_HANDLE) works fine as soon as we call AttachConsole/AllocConsole.

For example dump() use fputs and stderr here:
http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/loader/mozJSComponentLoader.cpp#193

Then, take a look at current code that handle -console argument:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsNativeAppSupportWin.cpp#380
int hCrt = ::_open_osfhandle( (intptr_t)GetStdHandle( STD_OUTPUT_HANDLE ),
                                             _O_TEXT );
if ( hCrt != -1 ) {
  FILE *hf = ::_fdopen( hCrt, "w" );
  if ( hf ) {
    *stdout = *hf;
  }
}

Is equivalent to:

freopen("CONOUT$", "wb", stdout);


Finally, I already made some choices:
- I let code that handle -console "as is", we may decide to use freopen instead and factor AllocConsole/AttachConsole code.
- If AttachConsole works at first place, the whole -console code is not executed.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Use AttachConsole (with comments)

Here is a new patch with correct indentation and comments justifying these operations.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Use AttachConsole and factor some code

Almost same patch but I tried to factor code that redirect std I/O handles.

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 559193 [details] [diff] [review]
Use AttachConsole (with comments)

Ted: Mossop told me that you would be a good reviewer for this patch. But feel free to forward this review if you know an expert on windows console stuff!

Then I asked for your review on this patch, but feel free to review the second one.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 559193 [details] [diff] [review]
Use AttachConsole (with comments)

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

::: toolkit/xre/nsNativeAppSupportWin.cpp
@@ +367,5 @@
>  void
>  nsNativeAppSupportWin::CheckConsole() {
> +    // Try to attach console to the parent process.
> +    // It will succeed when the parent process is a command line,
> +    // so that std I/O will be displayed in it.

I would just say "stdio" instead of "std I/O".

@@ +370,5 @@
> +    // It will succeed when the parent process is a command line,
> +    // so that std I/O will be displayed in it.
> +    if (AttachConsole(ATTACH_PARENT_PROCESS)) {
> +        // Change std handles to refer to new console handles.
> +        // Before doing so, ensure that stdout/stderr haven't being redirected to a valid file

"haven't been"

@@ +375,5 @@
> +        if (_fileno(stdout) == -1 || _get_osfhandle(fileno(stdout)) == -1)
> +            freopen("CONOUT$", "w", stdout);
> +        if (_fileno(stderr) == -1 || _get_osfhandle(fileno(stderr)) == -1)
> +            freopen("CONERR$", "w", stderr);
> +        freopen("CONIN$", "r", stdin);

Don't you want to do the same check here in case someone redirected stdin?

Comment 11

[[e-mail]]

Don't forget to announce the updated system requirements.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Updated patch with comment 10 addressed

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f24595efb2e

Also, to make life easier for those checking in patches on your behalf, please follow the directions below for future patches you submit. Thanks!
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

Comment 14

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/1f24595efb2e

Comment 15

[Phoenix]

On couple of last nightly builds, if I open saved page or start browser from console file manager (Far Manager in my case, but cmd.exe also affected), empty CONERR$ file created in run dir created. Is this regression from this bug?

Comment 16

[Alexandre Poirot [:ochameau]]

(In reply to Phoenix from comment #15)
> On couple of last nightly builds, if I open saved page or start browser from
> console file manager (Far Manager in my case, but cmd.exe also affected),
> empty CONERR$ file created in run dir created. Is this regression from this
> bug?

Yes, it is a regression (In reply to Phoenix from comment #15)
> On couple of last nightly builds, if I open saved page or start browser from
> console file manager (Far Manager in my case, but cmd.exe also affected),
> empty CONERR$ file created in run dir created. Is this regression from this
> bug?

Yes, I took this code from other projects and it appears that CONERR$ special file doesn't exists, only CONOUT$ does!

I'm backing some patches ASAP to fix this. Should we reopen this bug or open a new one?

Comment 17

[(not currently active) Ted Mielczarek]

File a new bug and make it block this one. It's easier to track the patches that way.

Comment 18

[Phoenix]

(In reply to Ted Mielczarek [:ted] from comment #17)
It's quite an offtopic here, but Ted, while you still here, what's with Bug 301988? If you are not appropriate reviewer, can you transfer the review to appropriate one?