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

RESOLVED FIXED

Status

Toolkit Graveyard
XULRunner
RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: Brian Crowder, Assigned: Brian Crowder)

Tracking

Trunk
All
Windows CE

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

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

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

Updated

9 years ago
tracking-fennec: 1.0a3-wm+ → ---
(Assignee)

Comment 1

8 years ago
Created attachment 412056 [details] [diff] [review]
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.
(Assignee)

Comment 2

8 years ago
Created attachment 412268 [details] [diff] [review]
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.
Attachment #412056 - Attachment is obsolete: true
Attachment #412268 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #412268 - Flags: review? → review?(benjamin)
(Assignee)

Updated

8 years ago
Attachment #412268 - Flags: review?(benjamin) → review?(mark.finkle)
(Assignee)

Comment 3

8 years ago
Moved review to finkle, but I would still be glad to hear bsmedberg's thoughts, if he has time for a review.
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
Attachment #412268 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 5

8 years ago
Created attachment 413203 [details] [diff] [review]
This needed some tweaks
Attachment #412268 - Attachment is obsolete: true
(Assignee)

Comment 6

8 years ago
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.
Attachment #413203 - Flags: review?(mark.finkle)
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
Attachment #413203 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 8

8 years ago
Created attachment 413349 [details] [diff] [review]
use GetModuleFileNameW instead of L"dummy "
Attachment #413203 - Attachment is obsolete: true
Attachment #413349 - Flags: review?(mark.finkle)
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
Attachment #413349 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 10

8 years ago
Created attachment 413354 [details] [diff] [review]
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.
Attachment #413349 - Attachment is obsolete: true
Attachment #413354 - Flags: review?(mark.finkle)
Comment on attachment 413354 [details] [diff] [review]
the one to land

Carry forward was fine. Sorry for confusion.
Attachment #413354 - Flags: review?(mark.finkle) → review+
tracking-fennec: --- → 1.0a4-wm+
(Assignee)

Comment 12

8 years ago
Created attachment 415285 [details] [diff] [review]
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.
Attachment #413354 - Attachment is obsolete: true
(Assignee)

Comment 13

8 years ago
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.
Attachment #415285 - Flags: review?(mark.finkle)
Attachment #415285 - Flags: review?(mark.finkle) → review+
(Assignee)

Updated

8 years ago
Attachment #415285 - Flags: approval1.9.2?
(Assignee)

Comment 14

8 years ago
http://hg.mozilla.org/mozilla-central/rev/568231ef38d9
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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! ;-)
Attachment #415285 - Flags: approval1.9.2?
(Assignee)

Comment 16

8 years ago
(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!
Ok, I have no problem taking this for WinCE on 192 with those changes, just have beltzner a+ it.
(Assignee)

Comment 18

8 years ago
Created attachment 415755 [details] [diff] [review]
Review fixes
Attachment #415755 - Flags: review?(dolske)
Attachment #415755 - Flags: review?(dolske)
Attachment #415755 - Flags: review+
Attachment #415755 - Flags: approval1.9.2?
(Assignee)

Comment 19

8 years ago
Created attachment 415758 [details] [diff] [review]
the rollup for 192
Attachment #415758 - Flags: approval1.9.2?
(Assignee)

Updated

8 years ago
Attachment #415755 - Flags: approval1.9.2?
(Assignee)

Updated

8 years ago
Attachment #415758 - Flags: approval1.9.2?
(Assignee)

Comment 20

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

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

Updated

8 years ago
status1.9.2: --- → final-fixed
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.