Crash in [FindDlgController getSearchText:]

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
General
--
critical
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Simon Fraser, Assigned: froodian (Ian Leue))

Tracking

({crash, fixed1.8.1.1})

Trunk
Camino1.5
PowerPC
Mac OS X
crash, fixed1.8.1.1

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

11 years ago
Talkback shows a couple of crashes in [FindDlgController getSearchText:].
(Reporter)

Comment 1

11 years ago
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()
(Reporter)

Comment 2

11 years ago
I'm not sure I trust this stack. The [FindDlgController getSearchText:] might be a red herring.
Severity: normal → critical
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?
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

11 years ago
Please see bug 337739, which also has getSearchText: and applicationWasActivated on the stack!  Seems to be the same situation.
(Assignee)

Comment 6

11 years ago
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! ;)
(Assignee)

Comment 7

11 years ago
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
Attachment #227728 - Attachment is obsolete: true
(Assignee)

Comment 8

11 years ago
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.
Assignee: sfraser_bugs → stridey
Status: NEW → ASSIGNED
Attachment #245156 - Flags: review?(stuart.morgan)
(Assignee)

Comment 9

11 years ago
*** Bug 337739 has been marked as a duplicate of this bug. ***
Target Milestone: --- → Camino1.1
(Reporter)

Comment 10

11 years ago
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.
Attachment #245156 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 11

11 years ago
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... :)
Attachment #245172 - Flags: review?(sfraser_bugs)
(Assignee)

Updated

11 years ago
Attachment #245172 - Flags: review?(stuart.morgan)

Comment 12

11 years ago
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.
Attachment #245172 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 13

11 years ago
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. :)
Attachment #245156 - Attachment is obsolete: true
Attachment #245172 - Attachment is obsolete: true
Attachment #245229 - Flags: review?(stuart.morgan)
Attachment #245172 - Flags: review?(sfraser_bugs)

Comment 14

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

r=me if you also remove the declaration of mLastFindString from the header.
Attachment #245229 - Flags: review?(stuart.morgan) → review+
(Assignee)

Comment 15

11 years ago
Created attachment 245439 [details] [diff] [review]
r=smorgan patch
Attachment #245229 - Attachment is obsolete: true
Attachment #245439 - Flags: superreview?(mikepinkerton)
Comment on attachment 245439 [details] [diff] [review]
r=smorgan patch

sr=pink
Attachment #245439 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 17

11 years ago
Checked in on trunk and 1.8branch
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.