519316 - remote start sequence should happen as early in xulrunner app startup as possible

Status: RESOLVED FIXED

(Toolkit Graveyard :: XULRunner, defect)

Description

[Brian Crowder]

+++ This bug was initially created as a clone of Bug #509249 +++

This is needed for the fast-startup component to perform optimally.

Comment 1

[Brian Crowder]

Attachment: wip

This needs better error handling, but is probably the right way to go.  I -was- fiddling with a better (re)use of nsNativeAppSupport, but it doesn't seem to be possible here without lots of work (mostly due to the fact that the implementations of this interface all rely heavily on XPCOM, whether they really need it or not).

I need better error-handling, but this is a good proof-of-concept and saves us about 5 seconds (maybe more) against opening a remote window -after- loading XUL and the associated libraries.

Comment 2

[Brian Crowder]

Attachment: works

This is a dirty hack, but I think less dirty than actually figuring out how to reuse the existing code.  I don't bother using the mutex or anything like that, just a quick check to find an existing messagable window, and if it's there, fire off a message and bail.

Comment 3

[Brian Crowder]

Moved review to finkle, but I would still be glad to hear bsmedberg's thoughts, if he has time for a review.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 412268 [details] [diff] [review]
works

>+  bool noRemote = false;
>+  for (int i = 0; i < argc; i++) {
>+    if (!strcmp(argv[i], "-no-remote")) {
>+      noRemote = true;
>+      break;
>+    }
>+  }

Use the IsArg helper function?

>+
>+  if (!noRemote) {
>+     char windowName[512];  // Is there a const for appname like VERSION_MAXLEN?

You started a 3-space indent here. Should be 2-space

>+     WCHAR wwindowName[512];

I see a few styles in the file for wide string variable names (wide_name, wideName, wName). You introduce a third style. Yours is most like the wName style I see in the file. Could you change over to that?

r+ with nits

Comment 5

[Brian Crowder]

Attachment: This needed some tweaks

Comment 6

[Brian Crowder]

Comment on attachment 413203 [details] [diff] [review]
This needed some tweaks

Happily, bugzilla interdiff works.  The nsNativeAppSupportWin tweak keeps the code in sync with the new faster-remoting stuff, which in debugging today I discovered was slightly wrong.  On WinCE (Windows Mobile only?), GetCommandLineW does not provide an argv[0] (the program itself) ala unix.  Instead, argv[0] is what unix calls argv[1] and so on.  So, we provide a dummy argv[0] here before doing any messaging.  The tmpPath preprocessor guards simply silence a warning in the nsXULStub compilation.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 413203 [details] [diff] [review]
This needed some tweaks

>diff --git a/xulrunner/stub/nsXULStub.cpp b/xulrunner/stub/nsXULStub.cpp

>+  WCHAR *path = L"dummy";
>+  WCHAR *cmd = ::GetCommandLineW();
>+  WCHAR cwd[MAX_PATH];

>+  WCHAR *wmsg = (WCHAR *)malloc(len * sizeof(*wmsg));

Sometimes you prefix WCHAR with nothing, other times you use a "w". Use either "wide_xxx", "wideXxx" or "wXxx" since these are already used in this file. Looks like you are using "wXxx" below.

>+  wcscat(wmsg, L" ");

Add to "dummy " like in native app support?

>+  COPYDATASTRUCT cds = { 1, len, (void *)msg };
>+  // Bring the already running Mozilla process to the foreground.
>+  // nsWindow will restore the window (if minimized) and raise it.
>+  // for activating the existing window on wince we need "| 0x01"

Add a blank line after the COPYDATASTRUCT line

>   char iniPath[MAXPATHLEN];
>+#if defined(XP_BEOS) || defined(XP_OS2)
>   char tmpPath[MAXPATHLEN];
>+#endif

isn't this variable needed here: http://mxr.mozilla.org/mozilla-central/source/xulrunner/stub/nsXULStub.cpp#294

that looks like a linux block


r+ with those nits fixed

Comment 8

[Brian Crowder]

Attachment: use GetModuleFileNameW instead of L"dummy "

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 413349 [details] [diff] [review]
use GetModuleFileNameW instead of L"dummy "

>diff --git a/toolkit/xre/nsNativeAppSupportWin.cpp b/toolkit

>+#ifdef WINCE
>+        WCHAR modname[256];

is MAX_PATH appropriate?

>diff --git a/xulrunner/stub/nsXULStub.cpp b/xulrunner/stub/nsXULStub.cpp

>+  // For WinCE, we're stuck with providing a dummy argv[0] for the remote

"... we're stuck with providing our own argv[0] ..."

>+  // command-line.
>+  WCHAR wPath[256];

MAX_PATH ?

>+  GetModuleFileNameW(NULL, wPath, sizeof(wPath) / sizeof(*wPath));

::GetModuleFileNameW ? we seem to use leading :: for other win32 api funcs

>+  WCHAR *wMsg = (WCHAR *)malloc(len * sizeof(*wMsg));
>+  wcscpy(wMsg, wPath);                                   // Dummy path

Not Dummy anymore

r+ after you check those nits

Comment 10

[Brian Crowder]

Attachment: the one to land

Wasn't sure if you meant you wanted another look, or wanted me to carry r+ forward, so r? for you.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 413354 [details] [diff] [review]
the one to land

Carry forward was fine. Sorry for confusion.

Comment 12

[Brian Crowder]

Attachment: Going back to dummies

Unfortunately, the command-line parsing code on the receiving end is not very sophisticated, and paths with spaces in them confuse it, so I'm going back to "dummy" argv[0]s for now.

Comment 13

[Brian Crowder]

Comment on attachment 415285 [details] [diff] [review]
Going back to dummies

Need another quick review on this one, if you don't mind.  Bugzilla interdiff works; the delta is small.

Comment 14

[Brian Crowder]

http://hg.mozilla.org/mozilla-central/rev/568231ef38d9

Comment 15

[Justin Dolske [:Dolske]]

Comment on attachment 415285 [details] [diff] [review]
Going back to dummies

Clearing a192 because I have a few questions about this...

>+#ifdef WINCE
>+        NS_ConvertUTF16toUTF8 utf8buffer(L"dummy ");
>+        AppendUTF16toUTF8(cmd, utf8buffer);
>+#else
>         NS_ConvertUTF16toUTF8 utf8buffer(cmd);
>+#endif

What's the significance of "dummy" here? Can this be some other self-documenting string, or add a comment as to its purpose? (Tiny nit: why convert L"dummy " instead of using just "dummy " as straight UTF8?


>--- a/xulrunner/stub/nsXULStub.cpp
[...]
>+  // Construct a narrow UTF8 buffer <path> <commandline>\0<workingdir>\0
>+  size_t len = wcslen(wPath) + wcslen(wCmd) + wcslen(wCwd) + 3;
>+  WCHAR *wMsg = (WCHAR *)malloc(len * sizeof(*wMsg));
>+  wcscpy(wMsg, wPath);
>+  wcscpy(wMsg + wcslen(wPath), wCmd);                // The command line
>+  wcscpy(wMsg + wcslen(wPath) + wcslen(wCmd) + 2, wCwd); // Working dir

You skipped 2 bytes here (between wCmd and wCwd in wMsg), but since it's a malloc'd buffer (not calloc) their value is undefined. You should explicitly set these to \0 if that's the intention.

Also, your comment and the "+3" seems to imply there's supposed to be a space after wPath? But the code's not adding one...


>+    char windowName[512];  // Is there a const for appname like VERSION_MAXLEN?
>+    rv = parser.GetString("App", "Name", windowName, sizeof(windowName));
...
>+    strncat(windowName, "MessageWindow", sizeof(windowName));

That should be "sizeof(windowName) - strlen(windowName)"... strncat takes the number of bytes *remaining*, not the total buffer size. Hopefully this window name string isn't content controllable and thus exploitable! ;-)

Comment 16

[Brian Crowder]

(In reply to comment #15)
> (From update of attachment 415285 [details] [diff] [review])
> Clearing a192 because I have a few questions about this...
> 
> >+#ifdef WINCE
> >+        NS_ConvertUTF16toUTF8 utf8buffer(L"dummy ");
> >+        AppendUTF16toUTF8(cmd, utf8buffer);
> >+#else
> >         NS_ConvertUTF16toUTF8 utf8buffer(cmd);
> >+#endif
> 
> What's the significance of "dummy" here? Can this be some other
> self-documenting string, or add a comment as to its purpose? (Tiny nit: why
> convert L"dummy " instead of using just "dummy " as straight UTF8?
> 

argv[0] should of course be something like '\Program Files\Fennec\fennec.exe', but on Windows Mobile, GetCommandLine() does not provide an argv[0] like that.  dummy is that arg.


> 
> >--- a/xulrunner/stub/nsXULStub.cpp
> [...]
> >+  // Construct a narrow UTF8 buffer <path> <commandline>\0<workingdir>\0
> >+  size_t len = wcslen(wPath) + wcslen(wCmd) + wcslen(wCwd) + 3;
> >+  WCHAR *wMsg = (WCHAR *)malloc(len * sizeof(*wMsg));
> >+  wcscpy(wMsg, wPath);
> >+  wcscpy(wMsg + wcslen(wPath), wCmd);                // The command line
> >+  wcscpy(wMsg + wcslen(wPath) + wcslen(wCmd) + 2, wCwd); // Working dir
> 
> You skipped 2 bytes here (between wCmd and wCwd in wMsg), but since it's a
> malloc'd buffer (not calloc) their value is undefined. You should explicitly
> set these to \0 if that's the intention.

Yeah, thanks.  I hadn't tested the working-dir (not even clear to me what that's for, really) part of this and I'm definitely off-by-one there.  Thanks!

> 
> Also, your comment and the "+3" seems to imply there's supposed to be a space
> after wPath? But the code's not adding one...

This code was written previously using GetModuleFileName() to provide argv[0], and that didn't include a " " at the end, I was adding one.  I removed that code, but did not repair my math.  Will respin.

> >+    char windowName[512];  // Is there a const for appname like VERSION_MAXLEN?
> >+    rv = parser.GetString("App", "Name", windowName, sizeof(windowName));
> ...
> >+    strncat(windowName, "MessageWindow", sizeof(windowName));
> 
> That should be "sizeof(windowName) - strlen(windowName)"... strncat takes the
> number of bytes *remaining*, not the total buffer size. Hopefully this window
> name string isn't content controllable and thus exploitable! ;-)

No, it comes from application.ini, but I'll fix the math anyway.  Thanks for the careful review!

Comment 17

[Justin Dolske [:Dolske]]

Ok, I have no problem taking this for WinCE on 192 with those changes, just have beltzner a+ it.

Comment 18

[Brian Crowder]

Attachment: Review fixes

Comment 19

[Brian Crowder]

Attachment: the rollup for 192

Comment 20

[Brian Crowder]

Comment on attachment 415758 [details] [diff] [review]
the rollup for 192

I guess since this blocks Fennec, I don't need approval...

Comment 21

[Brian Crowder]

The fixes from dolske's review:
http://hg.mozilla.org/mozilla-central/rev/c65fc47cb7e0

192 landing:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ff8031d5fb11