436998 - Implement "-app" parameter support for xulrunner-stub

Status: RESOLVED FIXED

Description

[Ronald Schouten]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5) Gecko/2008050509 Firefox/3.0b5
Build Identifier: Prism for Firefox 0.2

I'm not sure where to report this bug as it could be a problem with the Prism extension or how Firefox is packed for Ubuntu. Starting here, but please advise.

I just upgraded to Ubuntu 8.04 and installed the Prism extension from Mozilla Add-ons. I am able to create desktop launchers but double-clicking them opens Firefox to its homepage instead of Prism with the desired web application.

I have tried this with the following URLs:

http://mail.google.com/mail/#inbox
http://www.google.com/reader/view/#overview-page

Running the launchers exec command from the terminal produces the same result with no errors reported.

Reproducible: Always

Steps to Reproduce:
1. Install Prism for Firefox 0.2 from Mozilla Add-ons
2. Create desktop launcher for site of choosing
3. Double click on desktop launcher
Actual Results:  
Firefox opens to homepage

Expected Results:  
Prism opens with desired web application

BTW, I'm not sure what version of Prism I have installed. According to the Add-ons site it is "Prism for Firefox 0.2" but this seems a bit old and I recall that the current version is 0.9.

Comment 1

[Peter Petrov]

I confirm this bug. The Prism Extension v0.2 is practically unusable on Linux because of it. I have FF 3 Beta 5 on Ubuntu 8.04.

Comment 2

[Dan Dillinger]

This appears to be an issue related to the -app argument somehow simply not working. I first noticed it when trying to use firefox instead of xulrunner to start a xul application:

http://developer.mozilla.org/en/docs/XULRunner_tips#Using_Firefox_3_to_run_XULRunner_applications

I followed those instructions, and instead of opening my app window (as xulrunner would), I found that firefox simply loaded its usual window and start page.

Then, upon starting a Prism shortcut (to gmail), it exhibited the same behavior -- I got the default firefox window and start page instead.

I am also running Firefox 3 on Ubuntu 8.04

Comment 3

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

Can someone install a Mozilla build of Firefox 3 to see if there is a build configuration issue?

Comment 4

[Dan Dillinger]

Installed 3.0rc2 from http://www.mozilla.com/en-US/firefox/all-beta.html

Problem does not exist, -app flag starts up the expected xul application.

Comment 5

[Matthew Gertner]

So this bug is invalid, right?

Comment 6

[Ronald Schouten]

Most likely but I'm not sure, as it depends on what the Ubuntu developers have to say.

For more information see: https://bugs.launchpad.net/ubuntu/+source/ubufox/+bug/241787

Comment 7

[Alexander Sack]

This bug happens on all --with-libxul-sdk builds of firefox. Point is that the firefox binary (aka xulrunner-stub) doesnt provide a fully fledged xulrunner command line interface.

Comment 8

[Piotr]

I can confirm this bug on openSUSE 11.0 KDE4. Firefox version shiped with distribution: Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9.0) Gecko/2008061600 SUSE/3.0-0.2 Firefox/3.0

Clicking on short cut made by "Prism for Firefox" executes firefox homepage. It is not fixed also in Prism for Firefox 0.9.1 (Experimental)  [ refractor-linux.xpi ]

However Prism works as an standalone app, not as addon, as expected. (Prism 0.9.1 can by found here http://browsing.justdiscourse.com/2008/07/10/mozilla-prism-091-experimental-now-available/ )

Comment 9

[Piotr]

Oh, I forgot. From #Prism IRC Conversation: "need to fix stub, it doesn't handle the -app flag. firefox -app doesn't work on Linux"

Comment 10

[Ronald Schouten]

Between Alexander's and Piotr's comments I'm not sure where the fix is. Can someone in the know make a clear comment on this issue?

Comment 11

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

Some Linux distros use Firefox-on-XULRunner. Which means that the firefox binary is actually the xulrunner-stub. The xulrunner-stub doesn't have the commandline handler for the -app flag.

The fix should be to add the handler to the xulrunner-stub.

What do you think, Benjamin?

Comment 12

[Benjamin Smedberg]

That's acceptable.

Comment 13

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

Thinking more about this, changing xulrunner-stub is _not_ the right answer. Realize that "firefox -app" means "let this xulapp use the libxul runtime that powers firefox" and, further, firefox-on-xulrunner doesn't have a libxul of it's own.

Therefore, we should try to fix this bug by _not_ using "firefox -app" when Friefox has no libxul, but instead use the XULRunner runtime that Firefox itself is using.

Using "XpcomLib" or "GreD" could get us the path to the xulrunner runtime.

Comment 15

[Olivier Bilodeau]

The problem is because the firefox binary on ubuntu (and Suse I guess) doesn't accept the -app parameter.

Now this link[1] says that under linux you should use the xulrunner binary instead of the firefox binary to launch XULRunner applications.

So, in order to fix this, shouldn't the plugin create the link using the firefox binary for windows and using the xulrunner one for linux?

[1] https://developer.mozilla.org/en/XULRunner_tips#Using_Firefox_3_to_run_XULRunner_applications

Comment 16

[Alexander Sack]

Starting from comment 13 we could go further and provide a |XulAppCmd| that would allow extensions to gather the right xulrunner compatible command in a transparent fashion.

For example, when firefox is build --with-libxul-sdk it would point to $gre_dir/xulrunner ... otherwise $appdir/$appname.

Comment 17

[Matthew Gertner]

I'm happy to go with Mark's approach (i.e. using the shared XULRunner). Furthermore I would be delighted if someone with a Linux focus were willing to take a crack at this. I'm proposing it as blocking to Prism 1.0 and we want to get that out ASAP. Mark and I both have tons of work for 1.0 already. Come on Linux people, let's make Prism great on Linux as well!

Comment 19

[Antonio Gomes (tonikitoo)]

*finally* working effectively on it.

approach: trying to make xulrunner-stup to support -app ... as per mark suggestion on irc.

uploading a patch soon.

Comment 20

[Antonio Gomes (tonikitoo)]

Attachment: patch 0.1 - add -app support to xulrunner-stub (WIP)

First working version on linux. It has some issues I am trying to fix/improve, but the core of the idea would be it.

It works fine on Linux, I will test on other platforms (I have only WinXP soon).


@Mark:

1) should we change bug title to reflect to what the fix really is ?

2) is that the right approach ? (keep in mind if it looks hacky it is because i started yesterday at around 11:47 PM :))

Comment 21

[Antonio Gomes (tonikitoo)]

Attachment: patch 0.2 - add -app support to xulrunner-stub

same patch as previous 0.1, but functional.

Comment 22

[Antonio Gomes (tonikitoo)]

Attachment: patch 0.21 - add -app support to xulrunner-stub

same patch as 0.2 but making changes ifdef'ed to XP_UNIX.

Comment 23

[Antonio Gomes (tonikitoo)]

so patch works fine if launching:

1) from xulrunner-stub itself (in .../browser/dist/bin/xulrunner)

$ ./xulrunner-stub -app /home/agomes/.mozilla/firefox/uu0nc5ri.prism/extensions/[e-mail]/prism/application.ini  -override /home/agomes/.webapps/[e-mail]/override.ini -webapp [e-mail]

2) from firefox-xulrunner-stub  (in .../browser/dist/bin/)

/firefox -app /home/agomes/.mozilla/firefox/uu0nc5ri.prism/extensions/[e-mail]/prism/application.ini  -override /home/agomes/.webapps/[e-mail]/override.ini -webapp [e-mail]

3) from clicking the webapp icon (e.g. in ~/Desktop)

Exec="/home/agomes/mozilla/objdir/browser/dist/bin/firefox" -app "/home/agomes/.mozilla/firefox/uu0nc5ri.prism/extensions/refracto
[e-mail]/prism/application.ini" -override "/home/agomes/.webapps/[e-mail]/override.ini" -webapp [e-mail]


one thing that has to be pointed out is: 

1) patch does add support for "-app" parameter only, so other ones that show up above , including "-override" and "-webapp" are being ignored by the stub.

Comment 24

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

Comment on attachment 367490 [details] [diff] [review]
patch 0.21 - add -app support to xulrunner-stub   

Overall, this looks pretty good. One thing I would like to do is move the "--app" out of the Linux-only block and make it available for all platforms.

We already have a use for this in Fennec (Windows Mobile).

Comment 25

[Antonio Gomes (tonikitoo)]

Attachment: patch 0.3 - add -app support to xulrunner-stub

same patch as 0.21 but not ifdef'ed to Linux.

highlights:

* I have not tested it on Win32/CE or MAC, but on linux only. I suppose these are the platforms to be considered here (?).

* the Mac change has to be reviewed and tested. I can test on windows.

* my bigger concern: scenario

0) pre-conditions: 
- patch is applied (xulrunner stub supports -app parameter).
- user has prism addon installed.
1) user goes to any website (www.xyz.ab) and make a webapp off of it.
2) user does not close FF, and try to run the webapp by clicking on the Prism Icon generated on desktop. Although "-app" is supported, it will keep opening a FF window, as if user has one firefox opened and type 'firefox' again in the terminal (just a new window is created not another instance of Firefox on its own process). 

if we close firefox xulrunner_stub + "-app" works fine.

Mark, ^

Comment 26

[Antonio Gomes (tonikitoo)]

as bug scope is now bigger than original goal, I am changing it accordingly.

was: "prism extension creates desktop launcher but opens firefox".

Comment 27

[Antonio Gomes (tonikitoo)]

Comment on attachment 368827 [details] [diff] [review]
patch 0.3 - add -app support to xulrunner-stub

as per discussed on IRC:

1) wince does not work w/ this. Brad helped me to fix this.

2) there should be no link to nspr in stub.

3) this stub will need a more robust way to find the greDir.

new patch coming...

Comment 28

[Antonio Gomes (tonikitoo)]

Attachment: patch 0.4 - add -app support to xulrunner-stub

Comment 29

[Antonio Gomes (tonikitoo)]

Comment on attachment 369827 [details] [diff] [review]
patch 0.4 - add -app support to xulrunner-stub

* patch removes NSPR dep introduced in previous patch. It is wrong according to GRE docs.

Open questions:

-> @mfinkle: we find GRE in two ways here:

1) looks for a xulrunner dir in the same folder of application.in
2) find the xulrunner dir installed/registered in the system through GRE_GetGREPathWithProperties call

what can be improved ?

-> need brad (or some other wince lover) to test on that platform.

Comment 30

[Antonio Gomes (tonikitoo)]

Attachment: patch 0.5 - add -app support to xulrunner-stub

as per irc discussion, patch 0.5 now postpones the use of -app parameter (if passed in) to after greDir initial checks. 

Reason: it is desired to keep the current way of looking for the greDir, avoiding making it relative to the specified application.ini path.

ps: tested on linux only so far.

Comment 31

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

Comment on attachment 370647 [details] [diff] [review]
patch 0.5 - add -app support to xulrunner-stub


>+static nsresult SetEnvironmentVariable(const char* aEnvVar, const char* aValue)

I worry about this function name since it is the same as a #define in the windows headers. Could we rename to | SetEnv | ?

>+
>+/**
>+ * Return true if |greDir| is a valid GRE directory.

   * Return true if a given folder exists.

(this function is not limited to gre folders)

>+ */
>+static PRBool FolderExists(const char* greDir)

const char* aDir

>+{
>+#ifdef WINCE
>+  wchar_t wideGreDir[MAX_PATH];

wchar_t wideDir[MAX_PATH];

>+#else
>+  return access(greDir, R_OK) == 0;
>+#endif
>+
>+  return 0;

This | return 0; | is not needed. The #else handles it.

>+}
>+
>+static const char* GetRealPath(const char* appDataFile)
>+{
>+  char result[MAXPATHLEN];

I guess the the static function makes the data inside static too. I was a little worried the buffer was being freed when the function returned.

r=me with these comments addressed

Comment 32

[Brad Lassey [:blassey] (use needinfo?)]

shouldn't the functionality of SetEnvironmentVariable be provided by PR_SetEnv?  If not, lets fix PR_SetEnv.

Comment 33

[Antonio Gomes (tonikitoo)]

(In reply to comment #32)
> shouldn't the functionality of SetEnvironmentVariable be provided by PR_SetEnv?
>  If not, lets fix PR_SetEnv.

brad, according to https://developer.mozilla.org/En/GRE , section "Statically link to xpcomglue.lib (the "standalone glue") " XR stub can not link to NSPR as it is statically getting linked against the glue. that is why we are implementing our own setenv routine.

Comment 34

[Antonio Gomes (tonikitoo)]

Attachment: patch 0.6 - add -app support to xulrunner-st

addressed mark's comment + fixed problem w/ the way it was looking for libxpcom in the same dir as the stub.

Comment 35

[Antonio Gomes (tonikitoo)]

fixed error in patch 0.6:

* new patch:

+      if (!greFound) {
+        lastSlash = strrchr(iniPath, PATH_SEPARATOR_CHAR);
+        if (!lastSlash)
+          return 1;
+
+        *(++lastSlash) = '\0';
+
+        snprintf(greDir, sizeof(greDir), "%s" XPCOM_DLL, iniPath);
+        greFound = FolderExists(greDir);
+      }

* previous patch:

+      // We have a valid application.ini path passed in to the -app parameter 
+      // but not yet a valid greDir, so lets look for it also on the same folder
+      // as the stub.
+      if (!greFound) {
+        snprintf(greDir, sizeof(greDir), "%s" XPCOM_DLL, iniPath);
+        greFound = FolderExists(greDir);
+      }

Comment 36

[Antonio Gomes (tonikitoo)]

Attachment: patch 0.7 - add -app support to xulrunner-stub

more xp-like IsArg implementation. from irc:

<blassey> fine by me
<mfinkle> yes, looks good

Comment 37

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

Comment on attachment 370959 [details] [diff] [review]
patch 0.7 - add -app support to xulrunner-stub 

Benjamin - The only odd thing you might notice in this patch (hopefully) is the second look for the greDir.

I had Antonio only do this check if the -app param was used so it didn't change the old behavior of the stub. We need to allow for the stub to be _in_ the greDir (instead of above it as the first check assumes) for WinCE at least.

Comment 38

[Benjamin Smedberg]

Comment on attachment 370959 [details] [diff] [review]
patch 0.7 - add -app support to xulrunner-stub 

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

This file uses nsWindowsWMain.cpp. I think this means that all Windows char* in this file are UTF8, not native charset. If this is the case, several i18n bugs were already present in the Windows (desktop) version, but are being made more explicitly bad in the WINCE version.

If this is *not* the case, we should really make it clear which char* are the UTF8-munged kind and which are truly native, probably using typedefs.

>+static PRBool FolderExists(const char* aDir)
>+{
>+#ifdef WINCE
>+  wchar_t wideDir[MAX_PATH];
>+  MultiByteToWideChar(CP_ACP, 0, aDir, -1, wideDir, MAX_PATH);

CP_UTF8

>+  DWORD fileAttrs = GetFileAttributesW(wideDir);
>+  return fileAttrs != INVALID_FILE_ATTRIBUTES;
>+#else
>+  return access(aDir, R_OK) == 0;

Should be _waccess

>+static nsresult GetRealPath(const char* appDataFile, char* *aResult)
>+{
>+#ifdef XP_WIN
>+  wchar_t wAppDataFile[MAX_PATH];
>+  wchar_t wIniPath[MAX_PATH];
>+  MultiByteToWideChar(CP_ACP, 0, appDataFile, -1, wAppDataFile, MAX_PATH);

CP_UTF8

> class AutoAppData
> {
> public:
>   AutoAppData(nsILocalFile* aINIFile) : mAppData(nsnull) {
>     nsresult rv = XRE_CreateAppData(aINIFile, &mAppData);
>     if (NS_FAILED(rv))
>@@ -186,17 +241,16 @@ main(int argc, char **argv)
> 
> #ifdef XP_WIN
>   wchar_t wide_path[MAX_PATH];
>   if (!::GetModuleFileNameW(NULL, wide_path, MAX_PATH))
>     return 1;
> 
>   WideCharToMultiByte(CP_ACP, 0, wide_path,-1,
> 		      iniPath, MAX_PATH, NULL, NULL);

I think this should be CP_UTF8 also

>+      char *result = (char*) calloc(sizeof(char), MAXPATHLEN);
>+      memset(result, 0, MAXPATHLEN);

It shouldn't be necessary to calloc() and memset this data: calloc initializes to 0

Comment 39

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

(In reply to comment #38)
> (From update of attachment 370959 [details] [diff] [review])
> >diff --git a/xulrunner/stub/nsXULStub.cpp b/xulrunner/stub/nsXULStub.cpp
> >+  DWORD fileAttrs = GetFileAttributesW(wideDir);
> >+  return fileAttrs != INVALID_FILE_ATTRIBUTES;
> >+#else
> >+  return access(aDir, R_OK) == 0;
> 
> Should be _waccess

access -> _waccess  or we should use _waccess in the WINCE block? I'm pretty sure _waccess is not supported on WinCE

Comment 40

[Benjamin Smedberg]

Windows desktop should not use access() on a UTF8 char*. It should either use the GetFileAttributesW code above, or should use _waccess.

Comment 41

[Antonio Gomes (tonikitoo)]

Attachment: patch 0.8 - add -app support to xulrunner-stub

fixed benjamin's comments.

Comment 42

[Antonio Gomes (tonikitoo)]

> >+static PRBool FolderExists(const char* aDir)
> >+{
> >+#ifdef WINCE
> >+  wchar_t wideDir[MAX_PATH];
> >+  MultiByteToWideChar(CP_ACP, 0, aDir, -1, wideDir, MAX_PATH);
> 
> CP_UTF8

done.

> >+  DWORD fileAttrs = GetFileAttributesW(wideDir);
> >+  return fileAttrs != INVALID_FILE_ATTRIBUTES;
> >+#else
> >+  return access(aDir, R_OK) == 0;
> 
> Should be _waccess

instead of #WINCE it was changed to #WINXP, so both will be using GetFileAttributesW as suggested.

> >+static nsresult GetRealPath(const char* appDataFile, char* *aResult)
> >+{
> >+#ifdef XP_WIN
> >+  wchar_t wAppDataFile[MAX_PATH];
> >+  wchar_t wIniPath[MAX_PATH];
> >+  MultiByteToWideChar(CP_ACP, 0, appDataFile, -1, wAppDataFile, MAX_PATH);
> 
> CP_UTF8

done.

> > public:
> >   AutoAppData(nsILocalFile* aINIFile) : mAppData(nsnull) {
> >     nsresult rv = XRE_CreateAppData(aINIFile, &mAppData);
> >     if (NS_FAILED(rv))
> >@@ -186,17 +241,16 @@ main(int argc, char **argv)
> > 
> > #ifdef XP_WIN
> >   wchar_t wide_path[MAX_PATH];
> >   if (!::GetModuleFileNameW(NULL, wide_path, MAX_PATH))
> >     return 1;
> > 
> >   WideCharToMultiByte(CP_ACP, 0, wide_path,-1,
> > 		      iniPath, MAX_PATH, NULL, NULL);
> 
> I think this should be CP_UTF8 also

done.

> >+      char *result = (char*) calloc(sizeof(char), MAXPATHLEN);
> >+      memset(result, 0, MAXPATHLEN);
> 
> It shouldn't be necessary to calloc() and memset this data: calloc initializes
> to 0

done.

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 372700 [details] [diff] [review]
patch 0.8 - add -app support to xulrunner-stub

>+static PRBool FolderExists(const char* aDir)
>+{
>+#ifdef WINXP
>+  wchar_t wideDir[MAX_PATH];
>+  MultiByteToWideChar(CP_UTF8, 0, aDir, -1, wideDir, MAX_PATH);
>+  DWORD fileAttrs = GetFileAttributesW(wideDir);
>+  return fileAttrs != INVALID_FILE_ATTRIBUTES;
>+#else
>+  return access(aDir, R_OK) == 0;
>+#endif
>+}

WINCE insted of WINXP maybe.

Comment 44

[Benjamin Smedberg]

Comment on attachment 372700 [details] [diff] [review]
patch 0.8 - add -app support to xulrunner-stub

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

>+#ifdef WINXP

I think you meant XP_WIN here?

r=me with that fixed

Comment 45

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/fc10ea0d9806

Comment 46

[Antonio Gomes (tonikitoo)]

marking "wanted 191 ?" 

it'd be nice to have it on 1.9.1, once fennec wince AND prism 1.0 would be using it.

Comment 47

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

It's also a relatively safe XULRunner stub only change. It would be a benefit to Linux distros that use FF+XR builds.

Comment 48

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

Attachment: Mac bustage fix

Gavin discovered bustage on Mac. Here's the fix.

Comment 50

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

I'd like to get this patch on 1.9.1 so Firefox on Linux, which uses XULRunner as a runtime, can support the same -app functionality as Firefox on Mac and Windows. This patch is local to xulrunner-stub and does not affect Firefox directly, or at all on Windows and Mac.

On Linux, this patch only affects using Firefox as a runtime to launch xulapps. The only risk is that the patch is in the startup path of Firefox on Linux, but this patch has baked for a while on trunk with no ill affects.

Comment 51

[Mike Beltzner [:beltzner, not reading bugmail]]

Benjamin: thoughts on comment 50? I'm inclined to take it if you feel that it's safe.

Comment 52

[Karl Tomlinson (back Apr 29 :karlt)]

Comment on attachment 373396 [details] [diff] [review]
Mac bustage fix

This landed as
http://hg.mozilla.org/mozilla-central/rev/cffd84cbe5bb

Comment 53

[Karl Tomlinson (back Apr 29 :karlt)]

It attachment 373396 [details] [diff] [review] needed on 1.9.1?
1.9.1 includes <CFBundle.h> instead of <CoreFoundation/CoreFoundation.h>.

Comment 54

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

(In reply to comment #53)
> It attachment 373396 [details] [diff] [review] needed on 1.9.1?
> 1.9.1 includes <CFBundle.h> instead of <CoreFoundation/CoreFoundation.h>.

Karl - That is from bug 482277 which is removing carbon code. That bug/patch does not seem to be going to 191, only trunk. So <CFBundle.h> should stay in 191.

Comment 55

[Bruno Girin]

Related bug in launchpad: https://bugs.launchpad.net/ubuntu/+source/xulrunner/+bug/337176

Comment 56

[Karl Tomlinson (back Apr 29 :karlt)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/86b8356da552
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/03b37937512a