1050944 - Get Firefox to launch with the new .app bundle structure

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Get Firefox to launch with the new bundle structure, mostly by changing the GRE and XRE directory to point to the Resources directory instead of MacOS.

Note that this also requires the patch in bug 1048687.

Comment 1

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Attachment: Patch

Comment 2

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Comment on attachment 8470212 [details] [diff] [review]
Patch

Argh, cleanup broke the patch. Will upload a fixed version shortly.

Comment 3

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Comment on attachment 8470212 [details] [diff] [review]
Patch

False alarm. Patch is okay, I just forgot to apply it locally before building. :-)

Comment 4

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Comment on attachment 8470212 [details] [diff] [review]
Patch

Steven, I'm asking for your review of the Cocoa changes in nsAppShell.mm since you're going to be out next week. I still have major testing to do before I feel comfortable requesting reviews for the other parts, but I don't expect any more changes to Cocoa code.

Comment 5

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Comment on attachment 8470212 [details] [diff] [review]
Patch

Forwarding the review request for nsAppShell.mm to Markus. Markus, we used to have a structure of:

Contents/MacOS/res/MainMenu.nib and
Contents/MacOS/res/cursors/

Since we had to move all the data files from MacOS to Resources, it didn't make sense to have a structure of Resources/res anymore. Also, it seems like MainMenu.nib traditionally lives in the root of Resources anyway. The change here just allows us to find these files under Resources. Note that GRE now points at Resources, not MacOS. Here is the new structure:

Contents/Resources/MainMenu.nib and
Contents/Resources/cursors/

Comment 6

[Stephen A Pohl [:spohl] (OOO until 6/12)]

(In reply to Stephen Pohl [:spohl] from comment #5)
> The change here just allows us to find these files under Resources.

Actually, the change allows us to find MainMenu.nib. I'm actually not sure if or where we load the contents of cursors. Looking into it.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8470212 [details] [diff] [review]
Patch

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

I haven't double-checked the folder traversing code with the structure you showed me, but I expect that the browser would look completely broken if you had gotten something wrong, so I trust you on that.

::: toolkit/xre/nsXREDirProvider.cpp
@@ +300,5 @@
> +      rv = file->GetParent(getter_AddRefs(parent));
> +      if (NS_SUCCEEDED(rv)) {
> +        rv = parent->GetParent(getter_AddRefs(file));
> +        if (NS_SUCCEEDED(rv)) {
> +          rv = file->AppendNative(NS_LITERAL_CSTRING("MozResources"));

What does this code do? What's "MozResources"?

@@ +1414,5 @@
>    rv = GetAppDir()->Clone(getter_AddRefs(defaultsDir));
> +  NS_WARN_IF(NS_FAILED(rv));
> +  if(NS_FAILED(rv)) {
> +    return rv;
> +  }

I think NS_WARN_IF is supposed to be used like so:
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}
(also note the space after "if")

Comment 8

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Attachment: Patch

Addressed review feedback. Carrying over Markus' r+.

(In reply to Markus Stange [:mstange] from comment #7)
> ::: toolkit/xre/nsXREDirProvider.cpp
> @@ +300,5 @@
> > +      rv = file->GetParent(getter_AddRefs(parent));
> > +      if (NS_SUCCEEDED(rv)) {
> > +        rv = parent->GetParent(getter_AddRefs(file));
> > +        if (NS_SUCCEEDED(rv)) {
> > +          rv = file->AppendNative(NS_LITERAL_CSTRING("MozResources"));
> 
> What does this code do? What's "MozResources"?

This traverses back to the root of the .app bundle, where we will have a "MozResources" directory sitting next to "Contents" (see bug 1047584 comment 4 and bug 1047728 comment 9). This is because everything inside of "Contents" will be signed with Apple's new v2 signatures and we can no longer add or modify files inside of it after the bundle was signed. "MozResources" is our solution for things like channel-prefs.js and files for our partner builds of Firefox.

> I think NS_WARN_IF is supposed to be used like so:
> if (NS_WARN_IF(NS_FAILED(rv))) {
>   return rv;
> }

Ah, that definitely makes it less cumbersome to use.

> (also note the space after "if")

Oops, that was a typo and not intentional. Thanks for the catch!

Comment 9

[Markus Stange [:mstange]]

(In reply to Stephen Pohl [:spohl] from comment #8)
> This traverses back to the root of the .app bundle, where we will have a
> "MozResources" directory sitting next to "Contents" (see bug 1047584 comment
> 4 and bug 1047728 comment 9). This is because everything inside of
> "Contents" will be signed with Apple's new v2 signatures and we can no
> longer add or modify files inside of it after the bundle was signed.
> "MozResources" is our solution for things like channel-prefs.js and files
> for our partner builds of Firefox.

Alright, thanks for the explanation.

Comment 10

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Attachment: Patch

Fixed lookup for XUL library when running xpcshell-tests from obj-dir.

Carrying over mstange's r+ for the Cocoa parts.

Comment 11

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Attachment: Patch

Updated GeckoChildProcessHost::GetPathToBinary to work with the new GRE path.

Comment 12

[Benjamin Smedberg]

Comment on attachment 8473868 [details] [diff] [review]
Patch

This needs some docs. Especially I'm pretty sure we're changing which directory is passed to XRE_main and probably also NS_InitXPCOM, and the headers for those methods should contain a note about this structural issue. The doc comments on nsXREDirProvider should also mention what directories mGREDir/mXULAppDir point to on mac.

In nsBrowserApp.cpp you have exePath = exePath.AppendASCII("../MacOS"). In the case of symlinks, this may not be the same path as a direct path manipulation. It probably doesn't matter in this case, but is there a reason we can't use real path manipulation here as we do elsewhere?

Comment 13

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Attachment: Patch (wip)

This is just updated for current trunk and does not yet address any feedback.

Comment 14

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Attachment: Patch

Addressed feedback plus some cleanup. Still waiting for test harnesses to be updated and start passing before requesting review.

Comment 15

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Attachment: Patch

Minor fixup.

Comment 16

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Attachment: Patch

More fixup.

Comment 17

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Comment on attachment 8486547 [details] [diff] [review]
Patch

Try push is almost completely green, so requesting review (updater xpcshell test failures and gaia python integration test suite failures are handled in separate bugs):
https://tbpl.mozilla.org/?tree=Try&rev=7362867ad903

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8486547 [details] [diff] [review]
Patch

This looks fine to me.

Comment 19

[Benjamin Smedberg]

Comment on attachment 8486547 [details] [diff] [review]
Patch

>diff --git a/ipc/glue/GeckoChildProcessHost.cpp b/ipc/glue/GeckoChildProcessHost.cpp

>@@ -131,16 +132,34 @@ GeckoChildProcessHost::GetPathToBinary(F
>     nsCString path;
>     NS_CopyUnicodeToNative(nsDependentString(gGREPath), path);
>     exePath = FilePath(path.get());
> #endif
> #ifdef MOZ_WIDGET_COCOA
>     // We need to use an App Bundle on OS X so that we can hide
>     // the dock icon. See Bug 557225.
>     exePath = exePath.AppendASCII(MOZ_CHILD_PROCESS_BUNDLE);
>+
>+    const char* cPath = exePath.value().c_str();
>+    nsCOMPtr<nsIFile> tempPath;
>+    XRE_GetFileFromPath(cPath, getter_AddRefs(tempPath));

Is "exePath" an absolute path? If so, this should not be using XRE_GetFileFromPath, but NS_NewNativeLocalFile. XRE_GetFileFromPath is only for figuring out the binary path from argv[0]! It happens again below.

Comment 20

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Attachment: Patch

Sent this to Oak with all the other patches. Waiting for the results before requesting review again:
https://tbpl.mozilla.org/?tree=Oak&rev=45d91d73dbf5

Comment 21

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Comment on attachment 8488817 [details] [diff] [review]
Patch

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

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +136,5 @@
>  #ifdef OS_WIN
>      exePath = FilePath(char16ptr_t(gGREPath));
> +#elif MOZ_WIDGET_COCOA
> +    nsCOMPtr<nsIFile> childProcPath;
> +    NS_NewLocalFile(nsDependentString(gGREPath), true,

I'm assuming I should change followLinks to false here as well?

Comment 22

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Attachment: Patch

Went ahead and changed followLinks to false. Let me know if I shouldn't have.

Carrying over sr=bsmedberg and r=smichaud.

Comment 23

[Benjamin Smedberg]

Yes.

Comment 24

[Stephen A Pohl [:spohl] (OOO until 6/12)]

https://hg.mozilla.org/projects/oak/rev/f4e266d5f8f2

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 8489522 [details] [diff] [review]
Patch

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

I don't see anything here that concerns me.

Comment 26

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Attachment: Patch

Updated for current trunk. Carrying over r=smichaud,ted and sr=bsmedberg.

Comment 27

[Robert Strong (they/them - no direct email)]

Backed out old patch and relanded on oak
https://hg.mozilla.org/projects/oak/rev/834e099a2bbc

Comment 28

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Flagging this bug for addon-compat since we're changing the GRE and XRE directory to point to Contents/Resources (instead of Contents/MacOS).

Comment 29

[Robert Strong (they/them - no direct email)]

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/11d48c719ec9

Comment 30

[Robert Strong (they/them - no direct email)]

Attachment: patch for aurora

Comment 31

[[e-mail]]

Comment on attachment 8496236 [details] [diff] [review]
Patch

>+#ifdef XP_MACOSX
>+    nsCOMPtr<nsIFile> parent;
>+    greDir->GetParent(getter_AddRefs(parent));
>+    greDir = parent.forget();
>+    greDir->AppendNative(NS_LITERAL_CSTRING(kOSXResourcesFolder));
>+#endif
Out of interest, would SetNativeLeafName have worked for you here?

Comment 32

[Stephen A Pohl [:spohl] (OOO until 6/12)]

(In reply to [e-mail] from comment #31)
> Comment on attachment 8496236 [details] [diff] [review]
> Patch
> 
> >+#ifdef XP_MACOSX
> >+    nsCOMPtr<nsIFile> parent;
> >+    greDir->GetParent(getter_AddRefs(parent));
> >+    greDir = parent.forget();
> >+    greDir->AppendNative(NS_LITERAL_CSTRING(kOSXResourcesFolder));
> >+#endif
> Out of interest, would SetNativeLeafName have worked for you here?

It does seem like it would have.

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11d48c719ec9

Comment 34

[Eric Shepherd [:sheppy]]

I'll document this change. See c#28, tag #dev-doc-info.

Comment 35

[Eric Shepherd [:sheppy]]

Pages that need updating:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/XULRunner/Creating_custom_app_bundles_for_Mac_OS_X

https://developer.mozilla.org/en-US/docs/Using_nsIDirectoryService

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIDirectoryServiceProvider/getFile

Comment 36

[Eric Shepherd [:sheppy]]

(In reply to Stephen Pohl [:spohl] from comment #28)
> Flagging this bug for addon-compat since we're changing the GRE and XRE
> directory to point to Contents/Resources (instead of Contents/MacOS).

OK, I found NS_GRE_DIR easily enough, but what is XRE? That abbreviation isn't used anywhere that I could find, so I figured I'd better ask.

Comment 37

[Robert Strong (they/them - no direct email)]

https://developer.mozilla.org/en-US/docs/XRE

Comment 38

[Eric Shepherd [:sheppy]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #37)
> https://developer.mozilla.org/en-US/docs/XRE

That answers what the abbreviation stands for, but doesn't clarify for me which of the NS_*_DIR constants it refers to. (I'm sure it's obvious to you guys, but I'm afraid it's not to me)

Comment 39

[Stephen A Pohl [:spohl] (OOO until 6/12)]

If a constant is used to query it, it would be the same one, i.e. NS_GRE_DIR. In our test infrastructure, XRE directories are often passed via command line instead (via --xre-path command line flag for example).

Comment 40

[Robert Strong (they/them - no direct email)]

Landed on aurora in the Mac V2 signing combined patch in bug 1047584

Comment 41

[noitidart]

I saw this note from the addons blog. I was wondering does this affect me? In my addon I use App Bundle to create a shortcut to firefox:

https://gist.github.com/Noitidart/6a2cbe0b4c74499765be

This snippet creates a desktop icon that launches the profile manager.

You can copy paste that code and run from scratchpad. It seems to work to me, but I just wanted to check.

Comment 42

[Stephen A Pohl [:spohl] (OOO until 6/12)]

(In reply to noitidart from comment #41)
> It seems to work to me, but I just wanted to check.

After briefly looking over the code I think you should be in the clear. You're using XREExeF to find the target for your shortcut, which is the ideal way to do it.

Just fyi: What would no longer work is to request the GreD and then append the name of the target, since GreD is now pointing at Contents/Resources instead of Contents/MacOS. If you need to get the path to Contents/MacOS, use GreBinD instead. Again, this does not appear to apply to you since you're already using XREExeF.

Comment 43

[noitidart]

(In reply to Stephen Pohl [:spohl] from comment #42)
> (In reply to noitidart from comment #41)
> > It seems to work to me, but I just wanted to check.
> 
> After briefly looking over the code I think you should be in the clear.
> You're using XREExeF to find the target for your shortcut, which is the
> ideal way to do it.
> 
> Just fyi: What would no longer work is to request the GreD and then append
> the name of the target, since GreD is now pointing at Contents/Resources
> instead of Contents/MacOS. If you need to get the path to Contents/MacOS,
> use GreBinD instead. Again, this does not appear to apply to you since
> you're already using XREExeF.

Thanks Stephen! :) That FYI is real nice. I saw mentions of XRE in topic so got confused.