Closed Bug 1046924 Opened 5 years ago Closed 5 years ago

Create updates/ directory outside of Firefox.app/Contents/MacOS

Categories

(Toolkit :: Application Update, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: spohl, Assigned: spohl)

References

Details

Attachments

(1 file, 23 obsolete files)

4.76 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
With the new v2 signatures on OSX, the signature on Firefox.app will break when we create the updates/ directory under Firefox.app/Contents/MacOS. This issue is addressed in bug 394984. If we can't land bug 394984 in its entirety, we will need to land the changes for the updates/ directory separately here.
Group: mozilla-employee-confidential
Blocks: 1047584
No longer blocks: 1046306
Blocks: 1046906
No longer blocks: 1047584
Oops, blocking bug 1047584 was correct.
Blocks: 1047584
No longer blocks: 1046906
Attached patch Patch (wip) (obsolete) — Splinter Review
This is a trimmed version of the patch in bug 394984 that just places the 'updates' directory in a temporary user directory instead of the bundle.
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Attached patch Test updates (wip) (obsolete) — Splinter Review
Attached patch Patch (wip) (obsolete) — Splinter Review
Attachment #8473156 - Attachment is obsolete: true
Attached patch Test updates (wip) (obsolete) — Splinter Review
Attachment #8473869 - Attachment is obsolete: true
Attached patch Test updates (wip) (obsolete) — Splinter Review
Attachment #8473874 - Attachment is obsolete: true
Attached patch Test updates (wip) (obsolete) — Splinter Review
Attachment #8473876 - Attachment is obsolete: true
This might be a way to copy the necessary bits from the updates directory (which now lives outside of the bundle). This doesn't quite work yet. The following needs to be added:
1. Copy files to MozResources under the Updated.app bundle.
2. Update the skiplist to drop the correct files.
3. Debug and fix the relaunching of the app to apply the staged bits.
Attachment #8476434 - Flags: feedback?(robert.strong.bugs)
Attached patch additional test changes wip (obsolete) — Splinter Review
Stephen, try this out. If it works or gets it closer to working I'll take a look at the other tests that might be affected.
Attachment #8476155 - Attachment is obsolete: true
Attachment #8476347 - Attachment is obsolete: true
Attachment #8476434 - Attachment is obsolete: true
Attachment #8476434 - Flags: feedback?(robert.strong.bugs)
Attached patch Test patch (obsolete) — Splinter Review
This makes it so all of the xpcshell tests pass. I still want to go over the tests to make sure they are testing the right things with the changes to the client code.
It looks like all of the mochitest-chrome tests and xpcshell unit_base_updater tests are passing for me but one or more of the unit_aus_update tests is timing out.
So, there are 2 unit_aus_update tests timing out when they run in parallel and then pass when retried sequentially.
downloadInterruptedByOfflineRetry.js
downloadAndHashCheckMar.js

I'll investigate
Attached patch Test patch (obsolete) — Splinter Review
This fixes the problem I mentioned in comment #22
Attachment #8477914 - Attachment is obsolete: true
Comment on attachment 8477950 [details] [diff] [review]
Test patch

I moved the test patch into bug 1058182
Attachment #8477950 - Attachment is obsolete: true
Attached patch Patch (wip) (obsolete) — Splinter Review
Folded the two patches and moved the changes to GeckoChildProcessHost.cpp to bug 1050944. Additional work on this patch is necessary to remove the references to MozResources once we know how we're going to handle this change.
Attachment #8473873 - Attachment is obsolete: true
Attachment #8477913 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Dropped changes related to MozResources.
Attachment #8480067 - Attachment is obsolete: true
Comment on attachment 8480080 [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
Attachment #8480080 - Attachment description: Patch (wip) → Patch
Attachment #8480080 - Flags: review?(robert.strong.bugs)
Comment on attachment 8480080 [details] [diff] [review]
Patch

>diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp
>--- a/toolkit/xre/nsXREDirProvider.cpp
>+++ b/toolkit/xre/nsXREDirProvider.cpp
>...
>@@ -1025,18 +1025,46 @@ nsXREDirProvider::GetUpdateRootDir(nsIFi
> #else
>   nsCOMPtr<nsIFile> appFile;
>   bool per = false;
>   nsresult rv = GetFile(XRE_EXECUTABLE_FILE, &per, getter_AddRefs(appFile));
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = appFile->GetParent(getter_AddRefs(updRoot));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-#ifdef XP_WIN
>+#ifdef XP_MACOSX
>+  bool hasVendor = gAppData->vendor && strlen(gAppData->vendor) != 0;
> 
>+  if (hasVendor || gAppData->name) {
>+    nsCOMPtr<nsIFile> appRootDirFile;
>+    nsCOMPtr<nsIFile> localDir;
>+    nsAutoString appDirPath;
>+
>+    if (NS_SUCCEEDED(appFile->GetParent(getter_AddRefs(appRootDirFile))) &&
>+        NS_SUCCEEDED(appRootDirFile->GetPath(appDirPath))) {
>+      int32_t dotIndex = appDirPath.RFind(".app");
>+      if (dotIndex == kNotFound) {
>+        dotIndex = appDirPath.Length();
>+      }
>+      appDirPath = Substring(appDirPath, 1, dotIndex - 1);
>+
>+      if (NS_SUCCEEDED(GetUserDataDirectoryHome(getter_AddRefs(localDir),
>+                                                true)) &&
>+          NS_SUCCEEDED(localDir->AppendNative(nsDependentCString(hasVendor ?
>+                                                gAppData->vendor :
>+                                                gAppData->name))) &&
>+          NS_SUCCEEDED(localDir->Append(NS_LITERAL_STRING("updates"))) &&
>+          NS_SUCCEEDED(localDir->AppendRelativePath(appDirPath))) {
>+        NS_ADDREF(*aResult = localDir);
>+        return NS_OK;
>+      }
>+    }
>+  }
With v2 signing I don't think we want this to fall through to NS_ADDREF(*aResult = updRoot); when it doesn't succeed.

>+
>+#elif XP_WIN
>   nsAutoString pathHash;
>   bool pathHashResult = false;
>   bool hasVendor = gAppData->vendor && strlen(gAppData->vendor) != 0;
> 
>   nsAutoString appDirPath;
>   if (SUCCEEDED(updRoot->GetPath(appDirPath))) {
> 
>     // Figure out where we should check for a cached hash value. If the
>@@ -1115,17 +1143,17 @@ nsXREDirProvider::GetUpdateRootDir(nsIFi
>   }
> 
>   rv = GetUserLocalDataDirectory(getter_AddRefs(updRoot));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = updRoot->AppendRelativePath(programName);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-#endif
>+#endif // XP_WIN
> #endif
>   NS_ADDREF(*aResult = updRoot);
>   return NS_OK;
> }
> 
> nsresult
> nsXREDirProvider::GetProfileStartupDir(nsIFile* *aResult)
> {
Attachment #8480080 - Flags: review?(robert.strong.bugs) → review-
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #28)
> >+      if (NS_SUCCEEDED(GetUserDataDirectoryHome(getter_AddRefs(localDir),
> >+                                                true)) &&
> >+          NS_SUCCEEDED(localDir->AppendNative(nsDependentCString(hasVendor ?
> >+                                                gAppData->vendor :
> >+                                                gAppData->name))) &&
> >+          NS_SUCCEEDED(localDir->Append(NS_LITERAL_STRING("updates"))) &&
> >+          NS_SUCCEEDED(localDir->AppendRelativePath(appDirPath))) {
> >+        NS_ADDREF(*aResult = localDir);
> >+        return NS_OK;
> >+      }
> >+    }
> >+  }
> With v2 signing I don't think we want this to fall through to
> NS_ADDREF(*aResult = updRoot); when it doesn't succeed.

If I'm understanding this right, you are saying that we would rather have the update fail than to break the v2 signature on the .app bundle even though we don't know of an instance where the OS rechecks the signature after first launch? I'm fine with that, just wanted to make sure I wasn't missing something else.
Flags: needinfo?(robert.strong.bugs)
After thinking about it more I'd prefer it if the fallback was still under ~/Library/Caches. I'll provide more info later after I investigate it a little more.
Flags: needinfo?(robert.strong.bugs)
Attached patch Patch (obsolete) — Splinter Review
This keeps a fallback path under ~/Library/Caches like so:
~/Library/Caches/Mozilla/updates/<path to app>

The fallback only applies when neither vendor nor app name are available. We bail with NS_ERROR_FAILURE in any other error scenario (i.e. we never fall back to a directory that's inside of the .app bundle).
Attachment #8480080 - Attachment is obsolete: true
Attachment #8489569 - Flags: review?(robert.strong.bugs)
Comment on attachment 8489569 [details] [diff] [review]
Patch

>From: Stephen Pohl <spohl.mozilla.bugs@gmail.com>
>
>diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp
>--- a/toolkit/xre/nsXREDirProvider.cpp
>+++ b/toolkit/xre/nsXREDirProvider.cpp
>...
>@@ -1025,18 +1025,54 @@ nsXREDirProvider::GetUpdateRootDir(nsIFi
> #else
>   nsCOMPtr<nsIFile> appFile;
>   bool per = false;
>   nsresult rv = GetFile(XRE_EXECUTABLE_FILE, &per, getter_AddRefs(appFile));
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = appFile->GetParent(getter_AddRefs(updRoot));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-#ifdef XP_WIN
>+#ifdef XP_MACOSX
>+  nsCOMPtr<nsIFile> appRootDirFile;
>+  nsCOMPtr<nsIFile> localDir;
>+  nsAutoString appDirPath;
>+  if (NS_FAILED(appFile->GetParent(getter_AddRefs(appRootDirFile))) ||
>+      NS_FAILED(appRootDirFile->GetPath(appDirPath)) ||
>+      NS_FAILED(GetUserDataDirectoryHome(getter_AddRefs(localDir), true))) {
>+    return NS_ERROR_FAILURE;
>+  }
> 
>+  int32_t dotIndex = appDirPath.RFind(".app");
>+  if (dotIndex == kNotFound) {
>+    dotIndex = appDirPath.Length();
>+  }
>+  appDirPath = Substring(appDirPath, 1, dotIndex - 1);
>+
>+  bool hasVendor = gAppData->vendor && strlen(gAppData->vendor) != 0;
>+  if (hasVendor || gAppData->name) {
>+    if (NS_FAILED(localDir->AppendNative(nsDependentCString(hasVendor ?
>+                                           gAppData->vendor :
>+                                           gAppData->name)))) {
nit: might as well combine the if (NS_FAILED(localDir... into the parent if

>+      return NS_ERROR_FAILURE;
>+    }
>+  } else {
>+    if (NS_FAILED(localDir->AppendNative(NS_LITERAL_CSTRING("Mozilla")))) {
>+      return NS_ERROR_FAILURE;
>+    }
>+  }
nit: same here
Attachment #8489569 - Flags: review?(robert.strong.bugs) → review+
Attached patch PatchSplinter Review
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #33)
> >+  bool hasVendor = gAppData->vendor && strlen(gAppData->vendor) != 0;
> >+  if (hasVendor || gAppData->name) {
> >+    if (NS_FAILED(localDir->AppendNative(nsDependentCString(hasVendor ?
> >+                                           gAppData->vendor :
> >+                                           gAppData->name)))) {
> nit: might as well combine the if (NS_FAILED(localDir... into the parent if

So, the reason that I didn't combine this into the parent |if| is because it's not exactly equivalent. We want the first |if| to execute if we either have a vendor or an app name. If we then succeed in appending the vendor or app name to the localDir (i.e. |NS_FAILED()| returns false), I thought that we didn't want try the |else| clause next, since that would append "Mozilla" to |localDir| that already has the vendor or app name in it.

> >+      return NS_ERROR_FAILURE;
> >+    }
> >+  } else {
> >+    if (NS_FAILED(localDir->AppendNative(NS_LITERAL_CSTRING("Mozilla")))) {
> >+      return NS_ERROR_FAILURE;
> >+    }
> >+  }
> nit: same here

At first, I thought that having an |else| clause (instead of an |else if|) was more readable since it would make it clear that it is executed if we don't have a vendor or app name, but I don't think that's necessarily true. Changed it to |else if|. 

Setting back to r? just to make sure that you agree and that I didn't miss anything.
Attachment #8489569 - Attachment is obsolete: true
Attachment #8492180 - Flags: review?(robert.strong.bugs)
Comment on attachment 8492180 [details] [diff] [review]
Patch

Thanks!
Attachment #8492180 - Flags: review?(robert.strong.bugs) → review+
Oh wow, I was just getting ready to back out my old patch and land my new one only to find that this was already done! Thanks, Robert! :-)
https://hg.mozilla.org/mozilla-central/rev/0ce9e74b0abe
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Landed on aurora in the Mac V2 signing combined patch in bug 1047584
You need to log in before you can comment on or make changes to this bug.