1009668 - marStageSuccessComplete.js fails on OSX 10.9

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Jonathan Griffin (:jgriffin)]

When xpcshell tests are run on OSX 10.9, marStageSuccessComplete.js fails consistently:

https://tbpl.mozilla.org/php/getParsedLog.php?id=39527119&tree=Cedar#error0

Comment 1

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

Stephen, can you take this?

Comment 2

[Stephen A Pohl [:spohl]]

I had a look at this about a week ago with smichaud. What appears to happen here is that we're looking for a resource file in a bundle that is changed after the application is started, causing the native [NSBundle pathForResource] to crash [1]. I've found some info here [2]. I'm not sure if a recent change triggered this, or if this was always present on 10.9. We may have to replace the [NSBundle pathForResource] call with something more robust.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/launchchild_osx.mm#97
[2] https://stackoverflow.com/questions/3858647/nsbundle-pathforresource-failing-in-shell-tool

Comment 3

[Stephen A Pohl [:spohl]]

Is there any way to figure out when this test started failing on 10.9? Either via tbpl, mozregression or some othe means? I'm fairly confident that this test used to work for me on 10.9, but I'm not sure how to go about finding the regression point.

Comment 4

[Jonathan Griffin (:jgriffin)]

Unfortunately we can't tell.  We haven't been running them regularly for long, and we're not sheriffing these so they won't appear in OrangeFactor.

Comment 5

[Stephen A Pohl [:spohl]]

Good enough, I'll try to figure this out via hg bisect. Thanks, Jonathan!

Comment 6

[Stephen A Pohl [:spohl]]

This bug was triggered by bug 964550. What we did there is to turn on marStageSuccessComplete.js (and others) for OSX, so we still need to either change the tests to avoid this issue or avoid using NSBundle's pathForResource in the updater.

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 8

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

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

Nice and thank you!

>diff --git a/toolkit/mozapps/update/updater/launchchild_osx.mm b/toolkit/mozapps/update/updater/launchchild_osx.mm
>--- a/toolkit/mozapps/update/updater/launchchild_osx.mm
>+++ b/toolkit/mozapps/update/updater/launchchild_osx.mm
>@@ -70,37 +70,28 @@ void LaunchChild(int argc, char **argv)
>   posix_spawnattr_destroy(&spawnattr);
> 
>   if (result != 0) {
>     printf("Process spawn failed with code %d!", result);
>   }
> }
> 
> void
>-LaunchMacPostProcess(const char* aAppExe)
>+LaunchMacPostProcess(const char* aAppBundle)
> {
>   // Launch helper to perform post processing for the update; this is the Mac
>   // analogue of LaunchWinPostProcess (PostUpdateWin).
>   NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
> 
>-  // Find the app bundle containing the executable path given
>-  NSString *path = [NSString stringWithUTF8String:aAppExe];
>-  NSBundle *bundle;
>-  do {
>-    path = [path stringByDeletingLastPathComponent];
>-    bundle = [NSBundle bundleWithPath:path];
>-  } while ((!bundle || ![bundle bundleIdentifier]) && [path length] > 1);
>-  if (!bundle) {
>-    // No bundle found for the app being launched
>-    [pool release];
>-    return;
>-  }
>+  NSString* basePath = [NSString stringWithUTF8String:aAppBundle];
>+  basePath = [basePath stringByAppendingPathComponent:@"Contents/MacOS"];
> 
>-  NSString *iniPath = [bundle pathForResource:@"updater" ofType:@"ini"];
>-  if (!iniPath) {
>+  NSString* iniPath = [basePath stringByAppendingPathComponent:@"updater.ini"];
>+  NSFileManager* fileManager = [NSFileManager defaultManager];
>+  if (![fileManager fileExistsAtPath:iniPath]) {
>     // the file does not exist; there is nothing to run
>     [pool release];
>     return;
>   }
> 
>   int readResult;
>   char values[2][MAX_TEXT_LEN];
>   readResult = ReadStrings([iniPath UTF8String],
>@@ -114,23 +105,22 @@ LaunchMacPostProcess(const char* aAppExe
>   }
> 
>   NSString *exeArg = [NSString stringWithUTF8String:values[0]];
>   NSString *exeRelPath = [NSString stringWithUTF8String:values[1]];
>   if (!exeArg || !exeRelPath) {
>     [pool release];
>     return;
>   }
>-  
>-  NSString *resourcePath = [bundle resourcePath];
>-  NSString *exeFullPath = [resourcePath stringByAppendingPathComponent:exeRelPath];
>+
>+  NSString *exeFullPath = [basePath stringByAppendingPathComponent:exeRelPath];
What do you think about making this relative from the app bundle itself? If nothing else it is more flexible.

> 
>   NSTask *task = [[NSTask alloc] init];
>   [task setLaunchPath:exeFullPath];
>   [task setArguments:[NSArray arrayWithObject:exeArg]];
>   [task launch];
>   [task waitUntilExit];
>   // ignore the return value of the task, there's nothing we can do with it
>   [task release];
> 
>-  [pool release];  
>+  [pool release];
Thank you!

> }
> 
>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>@@ -3168,17 +3168,20 @@ int NS_main(int argc, NS_tchar **argv)
>           StartServiceUpdate(installDir);
>         }
>       }
>     }
>     EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 0);
> #endif /* XP_WIN */
> #ifdef XP_MACOSX
>     if (gSucceeded) {
>-      LaunchMacPostProcess(argv[callbackIndex]);
>+      NS_tchar installDir[MAXPATHLEN];
nit: tempted for this to just be char since it is in an ifdef, is clearer that way, and the param is a char.

>+      if (GetInstallationDir(installDir)) {
>+        LaunchMacPostProcess(installDir);
>+      }
>     }
> #endif /* XP_MACOSX */

Comment 9

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed feedback.

Comment 10

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

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

>diff --git a/toolkit/mozapps/update/updater/launchchild_osx.mm b/toolkit/mozapps/update/updater/launchchild_osx.mm
>--- a/toolkit/mozapps/update/updater/launchchild_osx.mm
>+++ b/toolkit/mozapps/update/updater/launchchild_osx.mm
>...
>@@ -114,23 +105,22 @@ LaunchMacPostProcess(const char* aAppExe
>   }
> 
>   NSString *exeArg = [NSString stringWithUTF8String:values[0]];
>   NSString *exeRelPath = [NSString stringWithUTF8String:values[1]];
>   if (!exeArg || !exeRelPath) {
>     [pool release];
>     return;
>   }
>-  
>-  NSString *resourcePath = [bundle resourcePath];
>-  NSString *exeFullPath = [resourcePath stringByAppendingPathComponent:exeRelPath];
>+
>+  NSString* exeFullPath = [NSString stringWithUTF8String:aAppBundle];
You aren't appending exeRelPath now.

> 
>   NSTask *task = [[NSTask alloc] init];
>   [task setLaunchPath:exeFullPath];
>   [task setArguments:[NSArray arrayWithObject:exeArg]];
>   [task launch];
>   [task waitUntilExit];
>   // ignore the return value of the task, there's nothing we can do with it
>   [task release];
> 
>-  [pool release];  
>+  [pool release];
> }
> 
r=me with that fixed.

BTW: I'll try to put together a test for this as well

Comment 11

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

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

One last thing that I think should be added. Check LaunchMacPostProcess and LaunchWinPostProcess is called when sStagedUpdate is true and if it is in the if statement before LaunchMacPostProcess and LaunchWinPostProcess first check !sStagedUpdate.

Comment 12

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

I added a test patch in progress in bug 606415 that should help with verifying the patch in this bug is doing the right thing. I'll try to add tests for comment #11 shortly as well as other tests relatively soonish.

Comment 13

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

I think the tests cover comment comment #11 now.

Comment 14

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed oversight pointed out in comment 10 (oops!).

Comment 15

[Steven Michaud [:smichaud] (Retired)]

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

Looks fine to me.

Comment 16

[Stephen A Pohl [:spohl]]

Now that the tests are ready in bug 606415, is there anything else we need to add here (see comment 11), or is this ready to land with the tests once they're reviewed?

Comment 17

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

Nope :)

If you want to steal the review of bug 606415 please feel free to do so and we'll get this and that bug landed. Thanks!

Comment 18

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c745965a2213

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/c745965a2213