789461 - Create a new directory service entry for the directory containing firefox.exe

Status: RESOLVED WONTFIX

(Toolkit :: General, defect)

Description

[Jim Mathies [:jimm]]

According to the header, this should return the current process directory. In nsXREDirProvider we return a clone of the app directory:

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#256

This points to dist/bin/browser. This incorrect, it should return dist/bin.

We use this a lot -

http://mxr.mozilla.org/mozilla-central/search?string=CurProcD

I'm surprised there aren't more failure.

Comment 1

[Benjamin Smedberg]

Everybody relies on the current behavior, which is to return the application directory. We should change the documentation to match what we actually do.

Comment 2

[Jim Mathies [:jimm]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> Everybody relies on the current behavior, which is to return the application
> directory. We should change the documentation to match what we actually do.

will do!

Comment 3

[Jim Mathies [:jimm]]

Attachment: patch

Mostly commenting. Includes a bug fix for one of the devtools/debugger tests.

Comment 4

[Jim Mathies [:jimm]]

Attachment: patch

fixing a typo.

Comment 5

[Benjamin Smedberg]

Comment on attachment 659261 [details] [diff] [review]
patch

Actually both parts of this are incorrect ;-)

In FF-on-XR which is what Linux distros use, the firefox binary *is* in CurProcD, e.g.:

GreD: libxul.so and platform omni.ja
CurProcD: firefox and browser omni.ja

In the Metro case, it accidentally happens that firefox.exe is in the GreD, but that might change in the future (I'd like to ship the platform in firefoxdir/platform). You really need a new key specifically for 'the location of firefox.exe whether we're in metro mode or not')

Comment 6

[Jim Mathies [:jimm]]

So basically you're suggesting I add a new dir string? How about "RealProcD" which on windows would find the working dir of the exe, and on mac and linux would return CurProcD?

All these different directory identifiers that imply one thing but return another seem overly confusing.

Comment 7

[Benjamin Smedberg]

Yes, "ExecutableD".

"working dir" isn't a good phrase, because it normally means CWD which is entirely different. But yes, something kinda like CurProcD.

I'm not sure what "imply one thing" means, but I agree that both the API and the naming/docs are all sucky.

Comment 8

[Jim Mathies [:jimm]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> Yes, "ExecutableD".

Would it be safe to return CurProcD for this on unix/mac?

> I'm not sure what "imply one thing" means, but I agree that both the API and
> the naming/docs are all sucky.

Yeah that was the point I was trying to make. :)

Comment 9

[Mike Hommey [:glandium]]

(In reply to Jim Mathies [:jimm] from comment #8)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> > Yes, "ExecutableD".
> 
> Would it be safe to return CurProcD for this on unix/mac?

Depends if we move the browser part under browser/ there too. It might be simpler on the packager end if all platforms do that.

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2

This turned out to be a lot easier than I was thinking it would be.

I'm getting the exe path in init and also adding a member function, because updater code will use this call in a different bug dependent on this bug early on from the directory service provider directly.

Comment 11

[Brian R. Bondy [:bbondy]]

Also there's an xpcshell dir provider that I'll update in the context of a different bug.  Because it has a wrong value even for XREExeF.

Comment 12

[Brian R. Bondy [:bbondy]]

Comment on attachment 664988 [details] [diff] [review]
Patch v2

Requesting feedback since not a peer.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 664988 [details] [diff] [review]
Patch v2

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

LGTM, but I'm not a peer.

::: toolkit/xre/nsXREDirProvider.cpp
@@ +110,5 @@
>      if (app) {
>        bool per = false;
>        app->GetFile(NS_APP_USER_PROFILE_50_DIR, &per, getter_AddRefs(mProfileDir));
>        NS_ASSERTION(per, "NS_APP_USER_PROFILE_50_DIR must be persistent!"); 
>        NS_ASSERTION(mProfileDir, "NS_APP_USER_PROFILE_50_DIR not defined! This shouldn't happen!"); 

Can you remove the trailing spaces while here?

Comment 14

[Benjamin Smedberg]

Comment on attachment 664988 [details] [diff] [review]
Patch v2

I'll delegate, glandium understands the issue well!

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 664988 [details] [diff] [review]
Patch v2

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

Okay then, if bsmedberg flags me :)

Comment 16

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Give XRE directory service provider the ability to know the current EXE dir

Removed trailing whitespace in the file (I love vim).
Carrying forward r+.

Since this is an elm specific discussion, I just wanted to verify that this is OK to land on m-c right (as long as try tests all still pass)?  It'll be one less thing to worry about at merge time for the resource splitting bug and has no direct dependencies on that work.

Comment 17

[Mike Hommey [:glandium]]

Oh yes, please land on m-c anything related to elm that doesn't actually depend on code in elm.

Comment 18

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Expose current exe entries for easy use with the directory service itself

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 665005 [details] [diff] [review]
Patch v1 - Expose current exe entries for easy use with the directory service itself

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

::: xpcom/io/nsDirectoryServiceDefs.h
@@ +66,5 @@
> +/**
> + * Property will return the directory of the executable file used to launch the
> + * current process. This is the parent directory of XREExeF.
> + */
> +#define NS_XRE_EXECUTABLE_DIR                   "XREExeD"

That looks dangerous, as in, it could conflict with the ones in nsXULAppAPI.h. Why do you need them in here, exactly? (as opposed to including nsXULAppAPI.h where you need them)

Comment 20

[Brian R. Bondy [:bbondy]]

Well some places do not include nsXULAppAPI.h but use the directory service directly instead.  For example see bug 791694.
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=791694&attachment=665007

In these cases we want to pass the string, but using a define is better. The directory service will enumerate through the directory service providers to find one that has that string and then return it's value (unless it already did this once, then it'll just return the cached value).

So we do want it to conflict with the XRE value.

Comment 21

[Brian R. Bondy [:bbondy]]

I see now that nsXREDirProvider.cpp includes nsDirectoryServiceDefs.h.
Maybe I should just remove the ones from nsXULAppAPI.h and replace the uses with the ones that I just added inside nsDirectoryServiceDefs.h?

Comment 22

[Brian R. Bondy [:bbondy]]

Changesets so far on elm, but there will be at least one more to address recent comments:
http://hg.mozilla.org/projects/elm/rev/4d72a1998caa
http://hg.mozilla.org/projects/elm/rev/ada80ab6b975

Comment 23

[Mike Hommey [:glandium]]

(In reply to Brian R. Bondy [:bbondy] from comment #21)
> I see now that nsXREDirProvider.cpp includes nsDirectoryServiceDefs.h.
> Maybe I should just remove the ones from nsXULAppAPI.h and replace the uses
> with the ones that I just added inside nsDirectoryServiceDefs.h?

browser/components/dirprovider/DirectoryProvider.cpp includes both nsDirectoryServiceDefs.h and nsXULAppAPI.h, for XRE_EXTENSIONS_DIR_LIST. Why not do the same?

Comment 24

[Brian R. Bondy [:bbondy]]

OK I thought maybe the xre dir service provider was a bit more internal and maybe shouldn't be exposed for files who only know what a dir service is and not a dir service provider.  I'll update according to Comment 23.

Comment 25

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3.

nsXULAppAPI.h contains a bunch of defs that I don't think belong inside some other files, so I put the defines inside sDirectoryServiceDefs.h.  If you disagree I can move it back though.  

I also fixed a small bug that I would have reached once doing the update patch where the function for getting the exeDir was returning the exe file instead.

I also added a check to make sure argv[0] exists, or else return an error on init.

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 665075 [details] [diff] [review]
Patch v3.

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

::: toolkit/xre/nsXREDirProvider.cpp
@@ +97,5 @@
>                               nsIDirectoryServiceProvider* aAppProvider)
>  {
>    NS_ENSURE_ARG(aXULAppDir);
>    NS_ENSURE_ARG(aGREDir);
> +  NS_ENSURE_TRUE(gArgc > 0, NS_ERROR_INVALID_ARG);

I don't think this should be tested here (isn't that tested elsewhere, anyways?).

@@ +102,5 @@
>  
>    mAppProvider = aAppProvider;
>    mXULAppDir = aXULAppDir;
>    mGREDir = aGREDir;
> +  nsresult rv = XRE_GetBinaryPath(gArgv[0], getter_AddRefs(mEXEFile));

I'm not convinced it's worth making the class bigger for XRE_EXECUTABLE_FILE just to avoid another XRE_GetBinaryPath call in nsXREDirProvider::GetFile.

::: xpcom/io/nsDirectoryServiceDefs.h
@@ +60,5 @@
> +/**
> + * Property will return the directory of the executable file used to launch the
> + * current process. This is the parent directory of XREExeF.
> + */
> +#define NS_XRE_EXECUTABLE_DIR                   "XREExeD"

It seems weird to have XRE things defined in nsDirectoryServiceDefs.h, especially when nsXREDirProvider is the one returning the values. Benjamin, what do you think?

Comment 27

[Brian R. Bondy [:bbondy]]

> I'm not convinced it's worth making the class bigger for XRE_EXECUTABLE_FILE 
> just to avoid another XRE_GetBinaryPath call in nsXREDirProvider::GetFile.

Why does it matter though? It's a singleton.

Comment 28

[Brian R. Bondy [:bbondy]]

-  rv = mEXEDir->GetParent(getter_AddRefs(mEXEDir));
+  rv = mEXEFile->GetParent(getter_AddRefs(mEXEDir));

Comment 29

[Benjamin Smedberg]

Comment on attachment 665075 [details] [diff] [review]
Patch v3.

There is not always a gArgc. I think we should be passing this value into nsXREDirProvider::Initialize and accept that sometimes it will be null, instead of doing magic things with gArgv which aren't always correct or useful.

In embedding situations we don't always know this value, and that has to be ok.

Comment 30

[Brian R. Bondy [:bbondy]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #29)
> Comment on attachment 665075 [details] [diff] [review]
> Patch v3.
> 
> There is not always a gArgc. 
> ...
> instead of doing magic things with gArgv which aren't always correct or
> useful.

So I guess you are saying that this existing code can't be trusted right?

 -  else if (!strcmp(aProperty, XRE_EXECUTABLE_FILE) && gArgv[0]) {
 -    nsCOMPtr<nsIFile> lf;
 -    rv = XRE_GetBinaryPath(gArgv[0], getter_AddRefs(lf));
 -    if (NS_SUCCEEDED(rv))
 -      file = lf;



> I think we should be passing this value into
> nsXREDirProvider::Initialize and accept that sometimes it will be null,

That was actually my initial approach, or at least part of it.  I had modified nsXREAppData to have an extra field for exePath which was always set to NULL in application.ini.h.  Then I'd fill it at the same time as we determine the GRE and app path.  Does that sound more correct? or less correct?

I'm referring to this code:

ScopedAppData appData(&sAppData);
nsCOMPtr<nsIFile> exeFile;
rv = mozilla::BinaryPath::GetFile(argv[0], getter_AddRefs(exeFile));
if (NS_FAILED(rv))
  return 1;

nsCOMPtr<nsIFile> greDir;
exeFile->GetParent(getter_AddRefs(greDir));

nsCOMPtr<nsIFile> appSubdir;
greDir->Clone(getter_AddRefs(appSubdir));
appSubdir->Append(NS_LITERAL_STRING("browser"));

SetStrongPtr(appData.directory, static_cast<nsIFile*>(appSubdir.get()));
SetStrongPtr(appData.xreDirectory, static_cast<nsIFile*>(greDir.get()));


> In embedding situations we don't always know this value, and that has to be
> ok.

What should it be set to when NULL?  Is just returning an error when it is requested enough?

Comment 31

[Brian R. Bondy [:bbondy]]

Also mozilla::BinaryPath::GetFile seems to do the same type of things on Linux like try realpath and if that doesn't work walk through the env vars looking for the binary.   I think that's what you meant by the magic things with argv.

Comment 32

[Jim Mathies [:jimm]]

https://hg.mozilla.org/projects/elm/rev/6b638f9ec174

FYI, this checkin seems to be the cause of various ipc related test failures on elm.

Comment 33

[Brian R. Bondy [:bbondy]]

Maybe plugin-container fails or something like that with it.  In any case I think we have no choice but to keep it on elm at this time since it makes updates work.  I need feedback on Comment 30 before I can do anything else here.

Comment 34

[Brian R. Bondy [:bbondy]]

Pushed this out which may help:
https://hg.mozilla.org/projects/elm/rev/4d0507d69644

Comment 35

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #34)
> Pushed this out which may help:
> https://hg.mozilla.org/projects/elm/rev/4d0507d69644

Woot! I see green!

Comment 36

[Jim Mathies [:jimm]]

How's the progress on this? This is holding up bug 793767. Between the two they represent a big portion of differences between mc and elm.

Comment 37

[Brian R. Bondy [:bbondy]]

Setting needs info flag for Comment 30.

Comment 38

[Benjamin Smedberg]

> So I guess you are saying that this existing code can't be trusted right?
> 
>  -  else if (!strcmp(aProperty, XRE_EXECUTABLE_FILE) && gArgv[0]) {
>  -    nsCOMPtr<nsIFile> lf;
>  -    rv = XRE_GetBinaryPath(gArgv[0], getter_AddRefs(lf));
>  -    if (NS_SUCCEEDED(rv))
>  -      file = lf;

Yes I'm saying that this will sometimes crash because sometimes gArgv will be null.

> > I think we should be passing this value into
> > nsXREDirProvider::Initialize and accept that sometimes it will be null,
> 
> That was actually my initial approach, or at least part of it.  I had
> modified nsXREAppData to have an extra field for exePath which was always
> set to NULL in application.ini.h.  Then I'd fill it at the same time as we
> determine the GRE and app path.  Does that sound more correct? or less
> correct?

Less correct. This should not be part of nsXREAppData (because it's not app data), it should just be passed directly as an argument to nsXREDirProvider.

> What should it be set to when NULL?  Is just returning an error when it is
> requested enough?

Probably, yes.

Comment 39

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/projects/elm/rev/913917010ee7

Comment 40

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/projects/elm/rev/1f25756f86c1

Comment 41

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4 - This review is for m-c.

This is how it exists on elm right now, but this review is for m-c.  I'll sync any review comments back into elm.

Comment 42

[Jim Mathies [:jimm]]

Removing this dep since 793767 already blocks metro-build.

Comment 43

[Jim Mathies [:jimm]]

Comment on attachment 690089 [details] [diff] [review]
Patch v4 - This review is for m-c.

glandium picked this up in some other work.

Comment 44

[Brian R. Bondy [:bbondy]]

So it is merged as part of something else on m-c? Note this stuff is already on elm w/ my pushes.

Comment 45

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #44)
> So it is merged as part of something else on m-c? Note this stuff is already
> on elm w/ my pushes.

We'll have to do some diffing to be sure elm and mc are in sync, but glandium mentioned this wasn't needed on irc - 

<jimm> glandium: do we still need bug 789461?
<firebot> Bug https://bugzilla.mozilla.org/show_bug.cgi?id=789461 nor, --, ---, netzen, NEW, Create a new directory service entry for the directory containing firefox.exe
<jimm> or can that be duped to one of the bugs you took over?
<glandium> jimm: no, we don't need that anymore

Comment 46

[Mike Hommey [:glandium]]

I'll back it out of elm when landing my stuff.