352069 - [10.3] Can't open Preferences after RSS landing

Status: VERIFIED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Eiichi]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; chrome://navigator/locale/navigator.properties; rv:1.9a1) Gecko/20060910 Camino/1.2+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; chrome://navigator/locale/navigator.properties; rv:1.9a1) Gecko/20060910 Camino/1.2+

Can't open Preferences after RSS landing and then Console logs;
*** -[NSCFSet addObject:]: attempt to insert nil

Reproducible: Always

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Kreeger, we missed something somewhere when accounting for Safari being bogus on 10.3.

On a stock 10.3, Safari is set to handle the feed protocol.  We exclude it from the list we build because of that, but it the user hasn't installed any other apps that register feed (or coerced LS into using an app that supports feed but doesn't register it, like iCab 3 in my case), we hit this error.

I can easily repro this by setting the feed handler back to Safari using RCDefaultApp.

(This might also be related to the wonky appearance of that drop-down that I was going to file....)

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Screenshot of the feed menu on 10.3 with Safari as default

My debug build "works" OK with Safari set, but the top separator is the selected item :P (and no console error) when you open prefs ;)

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Screenshot of the feed menu on 10.3 with iCab coerced as default

FYI, here's the extra separator on 10.3 when I coerce LS to set iCab as my feed reader.

I think both of these screenshots mean we need some extra logic to handle the case on 10.3 where there's no (other) feed-handling app.

Comment 4

[Stuart Morgan]

Filed comments 2/3 as bug 352078, with cause.

Comment 5

[Stuart Morgan]

Safari is excluded on all versions, but I can't seem to repro this on my (10.4) machine despite that, even after removing all other apps and setting my reader to Safari.

Comment 6

[Stuart Morgan]

Since I can't get a stack, I took a look through the code again.  This seems like the most likely spot:

+  NSMutableSet* feedApps = [[[NSMutableSet alloc] init] autorelease]; 
+  [feedApps addObject:[self defaultFeedViewerIdentifier]];

defaultFeedViewerIdentifier can return nil, and that's not checked for here.

+-(BOOL)validateAndRegisterDefaultFeedViewer:(NSString*)inBundleID
+{
+  NSBundle* defaultAppID = [NSBundle bundleWithIdentifier:[[NSWorkspace sharedWorkspace] defaultFeedViewerIdentifier]];
+  BOOL succeeded = YES;
+  
+  // if the user selected something other than the default application
+  if (![inBundleID isEqualToString:[defaultAppID bundleIdentifier]]) {

While I was looking I also noticed this, which I thought was fixed in the last round to nil check; it was supposed to be:

NSBundle* defaultAppID = [[NSWorkspace sharedWorkspace] defaultFeedViewerIdentifier];
...
if (defaultAppID && ![inBundleID isEqualToString:defaultAppID])

Comment 7

[Eiichi]

I haven't installed any RSS Reader.
So I try to set "RSS Reader" to <disable> using RCDefaultApp, then I can open Preferences.

Comment 8

[Stuart Morgan]

That would support the above, since <disable> actually does set the reader to an app.  If anyone else sees this, it would be helpful if they could leave it unfixed on their machine so we can test a patched build to make sure it's the right fix (since Smokey is no longer able to reproduce it).

Comment 9

[Nick Kreeger]

(In reply to comment #6)
> Since I can't get a stack, I took a look through the code again.  This seems
> like the most likely spot:
> 
> +  NSMutableSet* feedApps = [[[NSMutableSet alloc] init] autorelease]; 
> +  [feedApps addObject:[self defaultFeedViewerIdentifier]];
> 
> defaultFeedViewerIdentifier can return nil, and that's not checked for here.
> 
> +-(BOOL)validateAndRegisterDefaultFeedViewer:(NSString*)inBundleID
> +{
> +  NSBundle* defaultAppID = [NSBundle bundleWithIdentifier:[[NSWorkspace
> sharedWorkspace] defaultFeedViewerIdentifier]];
> +  BOOL succeeded = YES;
> +  
> +  // if the user selected something other than the default application
> +  if (![inBundleID isEqualToString:[defaultAppID bundleIdentifier]]) {
> 
> While I was looking I also noticed this, which I thought was fixed in the last
> round to nil check; it was supposed to be:
> 
> NSBundle* defaultAppID = [[NSWorkspace sharedWorkspace]
> defaultFeedViewerIdentifier];
> ...
> if (defaultAppID && ![inBundleID isEqualToString:defaultAppID])
> 

This is what I am seeing from my trunk build:

-(BOOL)validateAndRegisterDefaultFeedViewer:(NSString*)inBundleID
{
  NSString* defaultAppID = [[NSWorkspace sharedWorkspace] defaultFeedViewerIdentifier];
  BOOL succeeded = NO;
  
  // if the user selected something other than the default application
  if (defaultAppID && ![inBundleID isEqualToString:defaultAppID]) {

Comment 10

[Nick Kreeger]

Attachment: Patch version 0.1

Here is the correction for the NSWorkspace+Utils method. I might need to tweak the  popup menu building code as well.

Comment 11

[Stuart Morgan]

(In reply to comment #9)
> This is what I am seeing from my trunk build:
> ...

That explains why I remember it being fixed; I must have pulled up the wrong patch when I was looking over things.

Comment 12

[Stuart Morgan]

Comment on attachment 237741 [details] [diff] [review]
Patch version 0.1

|[feedApps addObject:defaultFeedViewerID];| is the only part you want conditionalized.  Not having a default shouldn't prevent the installed apps from showing.

Comment 13

[Nick Kreeger]

Attachment: Patch v0.2

Comment 14

[Stuart Morgan]

Comment on attachment 237767 [details] [diff] [review]
Patch v0.2

r=me

Comment 15

[Samuel Sidler (old account; do not CC)]

*** Bug 352561 has been marked as a duplicate of this bug. ***

Comment 16

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 237767 [details] [diff] [review]
Patch v0.2

rs=pink

Comment 17

[Nick Kreeger]

Checked into MOZILLA_1_8_BRANCH and trunk.

Comment 18

[Eiichi]

Confirmed.
Thank you, Nick.

Comment 19

[Chris Lawson (gone)]

Verified fixed per comment 18.

Comment 20

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 354782 has been marked as a duplicate of this bug. ***