Last Comment Bug 323718 - Crash in [FindDlgController getSearchText:]
: Crash in [FindDlgController getSearchText:]
Status: RESOLVED FIXED
: crash, fixed1.8.1.1
Product: Camino Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: PowerPC Mac OS X
-- critical (vote)
: Camino1.5
Assigned To: froodian (Ian Leue)
:
:
Mentors:
http://talkback-public.mozilla.org/se...
: 337739 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-16 22:37 PST by Simon Fraser
Modified: 2006-11-13 07:40 PST (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch? (1.26 KB, patch)
2006-06-30 14:12 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
Patch (2.54 KB, patch)
2006-11-09 15:55 PST, froodian (Ian Leue)
sfraser_bugs: review-
Details | Diff | Splinter Review
Patch (2.70 KB, patch)
2006-11-09 19:44 PST, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Rips out mLastFindString entirely (2.73 KB, patch)
2006-11-10 09:28 PST, froodian (Ian Leue)
stuart.morgan+bugzilla: review+
Details | Diff | Splinter Review
r=smorgan patch (3.57 KB, patch)
2006-11-12 23:54 PST, froodian (Ian Leue)
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Simon Fraser 2006-01-16 22:37:36 PST
Talkback shows a couple of crashes in [FindDlgController getSearchText:].
Comment 1 User image Simon Fraser 2006-01-16 22:51:33 PST
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 User image Simon Fraser 2006-01-16 22:59:20 PST
I'm not sure I trust this stack. The [FindDlgController getSearchText:] might be a red herring.
Comment 3 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-03-21 01:29:14 PST
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-03-31 01:18:22 PST
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 User image Håkan Waara 2006-05-12 10:55:37 PDT
Please see bug 337739, which also has getSearchText: and applicationWasActivated on the stack!  Seems to be the same situation.
Comment 6 User image froodian (Ian Leue) 2006-06-30 14:12:03 PDT
Created attachment 227728 [details] [diff] [review]
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 User image froodian (Ian Leue) 2006-06-30 14:38:36 PDT
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 User image froodian (Ian Leue) 2006-11-09 15:55:10 PST
Created attachment 245156 [details] [diff] [review]
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 User image froodian (Ian Leue) 2006-11-09 15:56:08 PST
*** Bug 337739 has been marked as a duplicate of this bug. ***
Comment 10 User image Simon Fraser 2006-11-09 18:26:52 PST
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 User image froodian (Ian Leue) 2006-11-09 19:44:28 PST
Created attachment 245172 [details] [diff] [review]
Patch

Simon, I'm targeting this to Stuart too, but since you looked at it once, if you get the time... :)
Comment 12 User image Stuart Morgan 2006-11-09 20:01:11 PST
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 User image froodian (Ian Leue) 2006-11-10 09:28:31 PST
Created attachment 245229 [details] [diff] [review]
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 User image Stuart Morgan 2006-11-12 16:39:39 PST
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 User image froodian (Ian Leue) 2006-11-12 23:54:59 PST
Created attachment 245439 [details] [diff] [review]
r=smorgan patch
Comment 16 User image Mike Pinkerton (not reading bugmail) 2006-11-13 05:44:02 PST
Comment on attachment 245439 [details] [diff] [review]
r=smorgan patch

sr=pink
Comment 17 User image froodian (Ian Leue) 2006-11-13 07:40:38 PST
Checked in on trunk and 1.8branch

Note You need to log in before you can comment on or make changes to this bug.