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.

Oops, blocking bug 1047584 was correct.

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.

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.

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.

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

This fixes the problem I mentioned in comment #22

Comment on attachment 8477950 [details] [diff] [review] Test patch I moved the test patch into bug 1058182

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.

Dropped changes related to MozResources.

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

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) > {

(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.

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.

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).

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

(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.

Comment on attachment 8492180 [details] [diff] [review] Patch Thanks!

Backed out the previous patch from oak and landed the new patch Backout https://hg.mozilla.org/projects/oak/rev/00a0b46e9921 Landing https://hg.mozilla.org/projects/oak/rev/189f6269e21a

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! :-)

No problem and have a good weekend!

Pushed to fx-team https://hg.mozilla.org/integration/fx-team/rev/0ce9e74b0abe

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