Closed Bug 1077282 Opened 5 years ago Closed 5 years ago

General cleanup for the Mac v2 signing changes

Categories

(Firefox :: General, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: rstrong, Assigned: spohl)

References

Details

Attachments

(3 files, 7 obsolete files)

Things can be cleaaned up some with the new GreBinD directory key and using SetNativeLeafName instead of GetParent then AppendNative.
Attached patch patch in progress (obsolete) — Splinter Review
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. :)
Attachment #8499362 - Flags: feedback?(spohl.mozilla.bugs)
OS: Windows 8.1 → Mac OS X
Hardware: x86_64 → All
Meh, failures so I backed this one out
Attached patch patch in progress (obsolete) — Splinter Review
This one compiles on Mac. The problem which I haven't investigated is in the changes to nsExceptionHandler.cpp
Attachment #8499362 - Attachment is obsolete: true
Attachment #8499362 - Flags: feedback?(spohl.mozilla.bugs)
Attachment #8499407 - Flags: review?(spohl.mozilla.bugs)
(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 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
Attachment #8499407 - Flags: review?(spohl.mozilla.bugs) → review-
If you'd like me to drive this from here, just let me know.
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Attached patch Patch (wip) (obsolete) — Splinter Review
in progress
Attachment #8499407 - Attachment is obsolete: true
Attached patch Patch for crashreporter (wip) (obsolete) — Splinter Review
in progress
Depends on: 1080338
Attached patch Patch (obsolete) — Splinter Review
Attachment #8501180 - Attachment is obsolete: true
Attachment #8502242 - Flags: review?(benjamin)
Attachment #8501181 - Attachment is obsolete: true
Attachment #8502243 - Flags: review?(benjamin)
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
Attached patch Patch (obsolete) — Splinter Review
Now with commit message.
Attachment #8502242 - Attachment is obsolete: true
Attachment #8502242 - Flags: review?(benjamin)
Attachment #8502245 - Flags: review?(benjamin)
Attached patch PatchSplinter Review
Missed the 'X' at the end of |#ifdef XP_MACOSX| in TestHarness.h.
Attachment #8502245 - Attachment is obsolete: true
Attachment #8502245 - Flags: review?(benjamin)
Attachment #8502499 - Flags: review?(benjamin)
Attached patch Workaround for bug 1080338 (obsolete) — Splinter Review
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.
Corrected #ifdef here as well.
Attachment #8502505 - Attachment is obsolete: true
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
Attachment #8502509 - Flags: review?(benjamin)
Attachment #8502243 - Flags: review?(benjamin) → review+
Attachment #8502499 - Flags: review?(benjamin) → review+
Attachment #8502509 - Flags: review?(benjamin) → review+
Landed on aurora in the Mac V2 signing combined patch in bug 1047584
Blocks: 1074021
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.