Need to notify SUUpdateDriverFinishedNotification on main thread

[Dave Riggle]

The -abortUpdate method in SSUpdateDriver.m should notify SUUpdateDriverFinishedNotification on the main thread like so:

- (void)abortUpdate
{
 [self setValue:[NSNumber numberWithBool:YES] forKey:@"finished"];
 [self performSelectorOnMainThread:@selector(_notifyUpdateFinished) withObject:nil waitUntilDone:NO];
}

- (void)_notifyUpdateFinished
{
 [[NSNotificationCenter defaultCenter] postNotificationName:SUUpdateDriverFinishedNotification object:self];
}

The way the code is now, the notification goes off on the detached thread, causing the SUUpdater.m -scheduleNextUpdateCheck method to schedule the checkTimer onto the detached thread's run loop. When the detached thread exits, the checkTimer is left referencing a zombie object, and the timer never fires. If the user later clicks the -checkForUpdates button, the app will crash.

This bug is in the 1.5b6 source. See the attached screenshot showing the stack trace where the checkTimer is put onto the wrong run loop.

[Hofman (cmhofman)]

The problem is not with SUUpdateDriver or its subclasses, as that never spawns a thread, so it should also not be fixed in those classes. The problem is in the SUPlainInstaller class which does not call back on the main thread. It should be fixed there. That class has more threading problems: AFAICS the method +copyPathWithAuthentication:overPath:error: is not thread safe (i.p. NSFileManager is not thread safe). So either this method needs to be made thread safe, or it should only be called on the main thread.

[Hofman (cmhofman)]

Indeed, almost all of the code in the installer classes is not thread safe. So it should only be called synchronously, or otherwise it must be seriously rewritten.

[kdbdallas (dbrown-hashbangind)]

Dave, could you provide me with a step by step process to reproduce this?
I am curious if this is the same issue I am having.

Is it just, start a check for updates and cancel it before it finishes and then just do another check for updates?
(If so, is there any timing that needs to be waited before checking the second time?)

[Hofman (cmhofman)]

Just one more remark on my comments, before anyone may dismiss them. When I say "not thread safe" I don't mean this in the usual sense of "cannot be used on more than one thread at the same time." The problem is much more severe: the code in the installer classes cannot be used on /any/ secondary thread. The (main) reason is that the shared NSFileManager and NSWorkspace instances that are now used on the secondary thread are the /same/ object instances that are used on the main thread, and these classes are definitely not thread safe.

So there are 2 solutions: either run the installers synchronously, or use (thread safe) Carbon instead of (thread unsafe) Cocoa.

Given this, I wonder if the same thread safety issues exist in other places? Did anyone check this?

[Dave Riggle (dave-busymac)]

The way I hit the bug was by having my CFBundleVersion set
incorrectly. It was empty on the version being delivered by the
appcast and non-empty on the version I was running. Sparkle was then
failing with this error:

  NSString * errorMessage = [NSString stringWithFormat:@"Sparkle
Updater: Possible attack in progress! Attempting to \"upgrade\" from
%@ to %@. Aborting update.", [bundle version], [[NSBundle
bundleWithPath:path] objectForInfoDictionaryKey:@"CFBundleVersion"]];

That caused the stack trace I showed in the bug report. However, I
suspect other notifications from the installer thread could have the
same problem.

Dave

On Jun 18, 2009, at 7:07 PM, kdbdallas wrote:

> Dave, could you provide me with a step by step process to reproduce
> this?
> I am curious if this is the same issue I am having.
>
> Is it just, start a check for updates and cancel it before it
> finishes and then just do another check for updates?
> (If so, is there any timing that needs to be waited before checking
> the second time?)
>
> --
> Need to notify SUUpdateDriverFinishedNotification on main thread
> https://bugs.launchpad.net/bugs/388793
> You received this bug notification because you are a direct subscriber
> of the bug.
>
> Status in Sparkle Updater: New
>
> Bug description:
> The -abortUpdate method in SSUpdateDriver.m should notify
> SUUpdateDriverFinishedNotification on the main thread like so:
>
> - (void)abortUpdate
> {
> [self setValue:[NSNumber numberWithBool:YES] forKey:@"finished"];
> [self performSelectorOnMainThread:@selector(_notifyUpdateFinished)
> withObject:nil waitUntilDone:NO];
> }
>
> - (void)_notifyUpdateFinished
> {
> [[NSNotificationCenter defaultCenter]
> postNotificationName:SUUpdateDriverFinishedNotification object:self];
> }
>
> The way the code is now, the notification goes off on the detached
> thread, causing the SUUpdater.m -scheduleNextUpdateCheck method to
> schedule the checkTimer onto the detached thread's run loop. When
> the detached thread exits, the checkTimer is left referencing a
> zombie object, and the timer never fires. If the user later clicks
> the -checkForUpdates button, the app will crash.
>
> This bug is in the 1.5b6 source. See the attached screenshot showing
> the stack trace where the checkTimer is put onto the wrong run loop.
>

[Paul Marks (shadowofged)]

My fix for issue #312995 addresses the case where -scheduleNextUpdateCheck is run anywhere other than main thread. It also properly clears out the checkTimer when invalidated, regardless of the originating thread. This guarantees that a background thread never gets the timer accidentally, and that we retain the timer (rather than relying on the runloop's +1).

[Andy Matuschak (andymatuschak)]

This is one of the new trinity of threading issues bugs, and it needs to be addressed for 1.5.

[Hofman (cmhofman)]

I attached a fix for SUPlainInstaller.m at bug # 389869 that fixes this one as well.

[Andy Matuschak (andymatuschak)]

Fixed in 2fc1b66.

Unfortunately, now all file-handling code is CoreServices-based, since NSFileManager isn't threadsafe. This is disgusting and will be stricken from all records when installation is performed by relaunch in Next Major, as it should have been in the first place.