323718 - Crash in [FindDlgController getSearchText:]

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Simon Fraser [no longer active]]

Talkback shows a couple of crashes in [FindDlgController getSearchText:].

Comment 1

[Simon Fraser [no longer active]]

Stack:
libobjc.A.dylib.227.0.0 + 0x5118 (0x909bf118)
-[FindDlgController getSearchText:]() [/builds/camino-release/camino/1.0/mozilla/camino//builds/camino-release/camino/1.0/mozilla/camino/src/find/FindDlgController.mm, line 163]
_nsnote_callback()
__CFXNotificationPost()
_CFXNotificationPostNotification()
-   -   -   NSApplicationMain()

Comment 2

[Simon Fraser [no longer active]]

I'm not sure I trust this stack. The [FindDlgController getSearchText:] might be a red herring.

Comment 3

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

This doesn't seem to show up in Talkback anymore, and since you didn't trust the stack to begin with, should we just close this?

Comment 4

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

If you can ever get the query in the new URL to load, there are a couple of instances now for Camino10.  

TB16741476 has this comment, which may or may not be helpful: "Typing in stickies.app, but command-tab switching from Camino."  

Comment 5

[Håkan Waara]

Please see bug 337739, which also has getSearchText: and applicationWasActivated on the stack!  Seems to be the same situation.

Comment 6

[froodian (Ian Leue)]

Attachment: Patch?

Let me start off by saying that I'm totally unfamiliar with this part of the code, I didn't research this heavily, I really don't know that much about Cocoa, and I am not a doctor.

That said though, two lines before we call [mLastFindString release] we have an if statement where one of the conditions is |!mLastFindString|.  This would seem to imply that there could be cases where mLastFindString doesn't exist.  Since it's never explicitly init] alloc]ed anywhere in the code, is it possible that we're just trying to release something with a retain count of 0?  If that's the case, this would insure against that, right?

Again, if I'm just showing my ignorance, please excuse it.  But if I'm right, aha! ;)

Comment 7

[froodian (Ian Leue)]

Comment on attachment 227728 [details] [diff] [review]
Patch?

[5:32pm] <pinkerton> there is no way that can fix it
[5:32pm] <froodian> oh well.  it was worth a try
[5:32pm] <kreeger> yeah i agree
[5:32pm] <pinkerton> if it's nil, it won't release. if it's garbage, it'll be non-nil and still crash

Comment 8

[froodian (Ian Leue)]

Attachment: Patch

First, note that some of the changes in this diff are trailing whitespace fixes.

Now, to the bug.  I've found some STR:

1. In another app, Cmd-E (might not be necessary?)
2. Switch to Camino
3. Cmd-F

2 and 3 have to be done *really* fast in order to repro - it usually takes me 4 or 5 times to get it right.

Through debugging I found that the portion of the line that crashes is [mLastFindString isEqualToString:searchText] (which is reproable when s/searchText/@"" too, so I know it's the [mLastFindString isEqualToString:] bit, and not the argument).

I also noticed that as this happens, Camino spits this into the console:

> 2006-11-09 18:01:40.226 Camino[12887] Exception raised during posting of 
> notification.  Ignored.  exception: *** -[NSPlaceholderString initWithString:]: > nil argument

I also noticed that NSString's |stringWithString| calls |initWithString| (both of which explicitly say not to pass nil into them).

So it looks like somehow if you Cmd-F when the app is still becoming frontmost, [mSearchField stringValue] is returning a nil value.  I'm not sure exactly why it's doing that, but if we make sure our intended argument isn't nil before calling |stringWithString|, we should be able to avoid this crash.  Note again that my STR aren't 100% reproable, but I haven't managed to crash Camino in this way with this patch applied.

QED, sorta.

Comment 9

[froodian (Ian Leue)]

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

Comment 10

[Simon Fraser [no longer active]]

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

>Index: src/find/FindDlgController.mm
>===================================================================

>+  NSString* searchText = [mSearchField stringValue];
> 
>-  [mLastFindString release];
>-  mLastFindString = [[NSString stringWithString:[mSearchField stringValue]] retain];
>+  // sending nil to stringWithString causes a crash, so be paranoid about not doing that
>+  if (searchText != nil) {
>+    [mLastFindString release];
>+    mLastFindString = [[NSString stringWithString:[mSearchField stringValue]] retain];

Why get the stringValue again? This line could be:

    mLastFindString = [searchText coyp];

(-copy is preferable to -retain for strings, since you don't know if the underlying string is mutable, and -copy is just a -retain on an immutable string).

>+  // sending nil to stringWithString causes a crash, so be paranoid about not doing that
>+  if (searchText != nil) {
>+    [mLastFindString release];
>+    mLastFindString = [[NSString stringWithString:searchText] retain];

Again, mLastFindString = [searchText copy].

Another way to fix this would be to make a -setLastFindString: method that handles the ref counting, and is nil-safe.

Comment 11

[froodian (Ian Leue)]

Attachment: Patch

Simon, I'm targeting this to Stuart too, but since you looked at it once, if you get the time... :)

Comment 12

[Stuart Morgan]

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

>+- (void)setLastFindString:(NSString *)string
>+{
>+  if (!string)
>+    return;

Don't do this; there's no reason you shouldn't be able to set it to nil.

>   [mLastFindString release];
>+  mLastFindString = [string copy];

This isn't a good setter pattern.  The release should be an autorelease.

>-  NSString* searchText;
>-  
>-  NSPasteboard *findPboard = [NSPasteboard pasteboardWithName:NSFindPboard];
>-  if ([[findPboard types] indexOfObject:NSStringPboardType] != NSNotFound) 
>+  NSString* searchText = @"";
>+
>+  NSPasteboard* findPboard = [NSPasteboard pasteboardWithName:NSFindPboard];
>+  if ([[findPboard types] indexOfObject:NSStringPboardType] != NSNotFound)
>     searchText = [findPboard stringForType:NSStringPboardType];
>-  else
>-    searchText = @"";

Why are you changing this to a double assignment in the most common case?


I'm also wondering if there's really any value in mLastFindString at all.  It seems like the overhead of maintaining is at least as high as the work it saves, which AFAICT is just not setting a text field to a string it already has.

Comment 13

[froodian (Ian Leue)]

Attachment: Rips out mLastFindString entirely

AFAICT, that's correct.  mLastFindString appears to just be there to save us setting the text field to itself when it hasn't changed, so I've taken it out on the premise that it's more overhead to keep it than lose it.  Simon, please speak up if these are incorrect assumptions. :)

Comment 14

[Stuart Morgan]

Comment on attachment 245229 [details] [diff] [review]
Rips out mLastFindString entirely

r=me if you also remove the declaration of mLastFindString from the header.

Comment 15

[froodian (Ian Leue)]

Attachment: r=smorgan patch

Comment 16

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 245439 [details] [diff] [review]
r=smorgan patch

sr=pink

Comment 17

[froodian (Ian Leue)]

Checked in on trunk and 1.8branch