421243 - Corrupt WebSearchEngines.plist should be moved aside instead of erased

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

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

Currently, if there's any sort of error in WebSearchEngines.plist when we launch, we just replace the file with the defaults, overwriting anything that may be there.  This is certainly better than the old behavior of failing to open a browser window, but it's still a bit nasty.

While we don't want to encourage users to edit the plist any longer, the fact remains that n < 100% of sites people will want to add a custom search for support OpenSearch, and we're likely only to include a handful of such sites on our own "get engines" page.  It's likely some users will continue to edit this file, and it seems like a bad idea to delete all of their work next launch if they happened to screw something up (e.g., fail to &-escape various characters--and, frankly, I think Sam and I were planning on deleting that warning along with 99% of the content of our existing "editing" page).

We should think about handling this the way we handle corrupt bookmarks: saving the existing file as foo-corrupt.plist and then creating a new file from the defaults.  That way at least the user doesn't irrevocably lose all search engine customizations.

Filing UNCO because we should discuss; there may be some reason I'm missing that makes the current behavior desired.

Comment 1

[philippe (part-time)]

That would be a UserFriendlyThingie (tm).
(I've been bitten by this recently: a silly copy-paste mistake; no real damage done, thanks to the usual backup).

Comment 2

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

I asked for objections to confirming this in the channel the other day, and none have been forthcoming; NEW.

Sean, is this hard?  Can you take it?

Comment 3

[Sean Murphy]

Shouldn't be a problem at all, especially since it's something that I should've thought of from the start...

Sure, I'll take a look.

Comment 4

[Sean Murphy]

Attachment: Patch

Comment 5

[Jeff Dlouhy]

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

The malformed plist gets copied, but does not appear to get overwritten. This leaves the broken plist intact, which causes Camino to crash.

I also get a similar crash when I remove the plist entirely resulting in:
2008-03-18 01:10:59.207 Camino[59120:10b] No search engines found in the profile directory; loading the defaults
2008-03-18 01:10:59.208 Camino[59120:10b] Exception raised during posting of notification.  Ignored.  exception: '*** -[NSCFArray objectAtIndex:]: index (0) beyond bounds (0)'  invoked observer method: '*** -[MainController applicationDidFinishLaunching:]'  observer: 0x93da90  notification name: 'NSApplicationDidFinishLaunchingNotification'

So in both cases it appears that the defaults are not being written correctly ... if at all.

Comment 6

[Sean Murphy]

(In reply to comment #5)
> (From update of attachment 308944 [details] [diff] [review])
> The malformed plist gets copied, but does not appear to get overwritten. This
> leaves the broken plist intact, which causes Camino to crash.
> 
> I also get a similar crash when I remove the plist entirely resulting in:
> 2008-03-18 01:10:59.207 Camino[59120:10b] No search engines found in the
> profile directory; loading the defaults
> 2008-03-18 01:10:59.208 Camino[59120:10b] Exception raised during posting of
> notification.  Ignored.  exception: '*** -[NSCFArray objectAtIndex:]: index (0)
> beyond bounds (0)'  invoked observer method: '*** -[MainController
> applicationDidFinishLaunching:]'  observer: 0x93da90  notification name:
> 'NSApplicationDidFinishLaunchingNotification'
> 
> So in both cases it appears that the defaults are not being written correctly
> ... if at all.

Jeff, I can't seem to reproduce either of those errors here.  I definitely made sure to test both scenarios before submitting the patch, as well.  It's possible I'm overlooking something simple, but I tried every combination of corrupt/non-existent WebSearchEngines (and even migrations of SearchURLList.plist) and the patch and previously existing default restoration code seems to handle it.

I'm also scratching my head because the 'restoring from defaults' code was not even touched by this patch, and it has definitely shown in the past to successfully pull non-existent WebSearchEngines from the bundle.

How did you go about "corrupting" WebSearchEngines.plist?  Was an exception raised during your first test, with the malformed engines?

Comment 7

[Jeff Dlouhy]

The 2 ways I went about testing it was first by removing random lines from the file as well as adding random characters throughout. The second time I just removed the file completely.

What are you using as a test case?

Comment 8

[Sean Murphy]

(In reply to comment #7)
> The 2 ways I went about testing it was first by removing random lines from the
> file as well as adding random characters throughout. The second time I just
> removed the file completely.
> 
> What are you using as a test case?

Hmm, yeah same approach.  I just broke the plist syntax, making it invalid.

If you have time to step through with the debugger, go ahead and trash the files in your profile directory completely again (WebSearchEngines and SearchURLList) and set a breakpoint on "-[NSException raise]".  Trace back the stack and see which line of SearchEngineManager.mm it's coming from, so we can see how far we get.

I wonder if for some reason your copy is not finding WebSearchEngines.plist in the bundle.  The code's assumption that the bundle always has an accessible copy could be the cause for the exceptions during that second test, as it proceeds to access the dictionary after the restoration.

Comment 9

[Stuart Morgan]

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

This looks good in my tests, and from irc conversations, I gather than comments 5/7 were from a test of corrupting the file in the Camino bundle, which isn't a case that we expect to handle. If there is some other way to get those exceptions, that should be filed as a new bug since it would be a problem with the existing code.

>+      NSString *corruptedPath = [[NSFileManager defaultManager] backupFileNameFromPath:pathToSavedEngineInfo withSuffix:@"-corrupted"];

Break the line before |withSuffix:|

>+      NSLog(@"Saved search engines file detected, but moving aside to '%@' since it appears corrupt", corruptedPath);

For consistency with the bookmark logging, just use:
    NSLog(@"Moved corrupted search engines file to '%@'", corruptedPath);

sr=smorgan with those changes.

Comment 10

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH; I made the changes from comment 9 on checkin.