1077282 - General cleanup for the Mac v2 signing changes

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

Things can be cleaaned up some with the new GreBinD directory key and using SetNativeLeafName instead of GetParent then AppendNative.

Comment 1

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

Attachment: patch in progress

Hey Stephen, this cleans up most of the use cases and I was hoping you'd be willing to drive this to landing.

I think I got most everything and the areas I didn't change are
* xpcom/build/XPCOMInit.cpp NS_InitXPCOM2 and specifically how Mac sets xpcomLib since it isn't clear that the check for existence isn't needed.

* toolkit/xre/nsUpdateDriver.cpp GetInstallDirPath cause I got tired. :)

Comment 2

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

Try was locked up so I pushed to oak
https://hg.mozilla.org/projects/oak/rev/3df4eb39cf4a

Comment 3

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

Meh, failures so I backed this one out

Comment 4

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

Attachment: patch in progress

This one compiles on Mac. The problem which I haven't investigated is in the changes to nsExceptionHandler.cpp

Comment 5

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #4)
> The problem which I haven't investigated is in the changes to nsExceptionHandler.cpp

NS_LITERAL_CSTRING will work (the previous patch used NS_LITERAL_STRING).

Comment 6

[Stephen A Pohl [:spohl]]

Comment on attachment 8499407 [details] [diff] [review]
patch in progress

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

This will make things much cleaner, thanks! Just a few comments below. Please feel free to also add the change to nsExceptionHandler.cpp back in (after making it use NS_LITERAL_CSTRING).

::: 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), false,

I would prefer if we could introduce a new global gGREBinPath and use this instead.

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +1360,5 @@
>          XRE_GetFileFromPath(argv[0], getter_AddRefs(greDir));
>          nsCOMPtr<nsIFile> parentDir;
>          greDir->GetParent(getter_AddRefs(parentDir));
> +        greDir = parentDir.forget();
> +        greDir->SetNativeLeafName(NS_LITERAL_CSTRING("Resources"));

Could you rewrite this to something like this (saves one line):

nsCOMPtr<nsIFile> tmpDir;
XRE_GetFileFromPath(argv[0], getter_AddRefs(tmpDir));
tmpDir->GetParent(getter_AddRefs(greDir));
greDir->SetNativeLeafName(NS_LITERAL_CSTRING("Resources"));

::: security/manager/ssl/tests/unit/head_psm.js
@@ +359,5 @@
>    let directoryService = Cc["@mozilla.org/file/directory_service;1"]
>                             .getService(Ci.nsIProperties);
>    let envSvc = Cc["@mozilla.org/process/environment;1"]
>                   .getService(Ci.nsIEnvironment);
> +  let greDir = directoryService.get("GreBinD", Ci.nsIFile);

nit: let's change the variable name to greBinDir

::: startupcache/test/TestStartupCache.cpp
@@ +411,5 @@
>    nsresult scrv;
>  
>    // Register TestStartupCacheTelemetry
>    nsCOMPtr<nsIFile> manifest;
>    scrv = NS_GetSpecialDirectory(NS_GRE_DIR,

We should be able to query NS_GRE_BIN_DIR now and do away with the entire #ifdef XP_MACOSX below, no?

::: xpcom/build/XPCOMInit.cpp
@@ +600,5 @@
>    parent->AppendNative(NS_LITERAL_CSTRING("MacOS"));
>    bool pathExists = false;
>    parent->Exists(&pathExists);
>    if (pathExists) {
> +    xpcomLib = parent.forget();

We should be able to query for NS_GRE_BIN_DIR here as well and do away with the entire #ifdef XP_MACOSX block here.

::: xpcom/tests/unit/head_xpcom.js
@@ +14,5 @@
>    var envSvc = Components.classes["@mozilla.org/process/environment;1"].
>      getService(Components.interfaces.nsIEnvironment);
>    var dirSvc = Components.classes["@mozilla.org/file/directory_service;1"].
>      getService(Components.interfaces.nsIProperties);
> +  var greDir = dirSvc.get("GreBinD", Components.interfaces.nsIFile);

nit: change variable name to greBinDir

Comment 7

[Stephen A Pohl [:spohl]]

If you'd like me to drive this from here, just let me know.

Comment 8

[Stephen A Pohl [:spohl]]

Attachment: Patch (wip)

in progress

Comment 9

[Stephen A Pohl [:spohl]]

Attachment: Patch for crashreporter (wip)

in progress

Comment 10

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 11

[Stephen A Pohl [:spohl]]

Attachment: Patch for crashreporter

Comment 12

[Stephen A Pohl [:spohl]]

These patches are blocked from landing by bug 1080338, but should be reviewable nevertheless.

Kicked off a try build with the patches here and bug 1077099 (followup) applied. The cpp unit tests on OSX are expected to fail due to bug 1080338:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0476c5006628

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: Patch

Now with commit message.

Comment 14

[Stephen A Pohl [:spohl]]

Attachment: Patch

Missed the 'X' at the end of |#ifdef XP_MACOSX| in TestHarness.h.

Comment 15

[Stephen A Pohl [:spohl]]

Attachment: Workaround for bug 1080338

This workaround for bug 1080338 would allow us to land the patches here and then transition without hiccups to a proper fix in bug 1080338. This patch could simply be reverted once bug 1080338 lands.

I could not test this on try yet since we're seemingly having problems with our jobs db. Releng is looking into it. Will wait to have a green try run before asking for review.

Comment 16

[Stephen A Pohl [:spohl]]

Attachment: Workaround for bug 1080338

Corrected #ifdef here as well.

Comment 17

[Stephen A Pohl [:spohl]]

Comment on attachment 8502509 [details] [diff] [review]
Workaround for bug 1080338

This workaround turns things green on try and I've also confirmed that running ./mach cppunittest locally still works:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7930021c75fd

Comment 18

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c87dcd65628
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a359d3faec
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a3560432b0

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9c87dcd65628
https://hg.mozilla.org/mozilla-central/rev/a2a359d3faec
https://hg.mozilla.org/mozilla-central/rev/d3a3560432b0

Comment 20

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

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