Closed Bug 352069 Opened 14 years ago Closed 14 years ago

[10.3] Can't open Preferences after RSS landing

Categories

(Camino Graveyard :: Preferences, defect)

PowerPC
macOS
defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED
Camino1.5

People

(Reporter: suishouen, Assigned: nick.kreeger)

References

Details

(Keywords: fixed1.8.1, regression)

Attachments

(3 files, 1 obsolete file)

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
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....)
Severity: normal → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Target Milestone: --- → Camino1.1
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 ;)
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.
Filed comments 2/3 as bug 352078, with cause.
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.
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])
I haven't installed any RSS Reader.
So I try to set "RSS Reader" to <disable> using RCDefaultApp, then I can open Preferences.
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).
(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]) {
Attached patch Patch version 0.1 (obsolete) — Splinter Review
Here is the correction for the NSWorkspace+Utils method. I might need to tweak the  popup menu building code as well.
Assignee: nobody → nick.kreeger
Status: NEW → ASSIGNED
(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 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.
Attached patch Patch v0.2Splinter Review
Attachment #237741 - Attachment is obsolete: true
Attachment #237767 - Flags: review?(stuart.morgan)
Comment on attachment 237767 [details] [diff] [review]
Patch v0.2

r=me
Attachment #237767 - Flags: superreview?(mikepinkerton)
Attachment #237767 - Flags: review?(stuart.morgan)
Attachment #237767 - Flags: review+
*** Bug 352561 has been marked as a duplicate of this bug. ***
Comment on attachment 237767 [details] [diff] [review]
Patch v0.2

rs=pink
Attachment #237767 - Flags: superreview?(mikepinkerton) → superreview+
Checked into MOZILLA_1_8_BRANCH and trunk.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Confirmed.
Thank you, Nick.
Verified fixed per comment 18.
Status: RESOLVED → VERIFIED
*** Bug 354782 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.