240787 - bookmarks panel toggling crash [@ objc_msgSend]

Status: RESOLVED INVALID

(Camino Graveyard :: Bookmarks, defect)

Description

[Josh Aas]

When I was using Camino under a heavy load I toggled the bookmark manager a
couple of times and Camino crashed. I can't reproduce it, but the stack trace
suggests that bulletproofing a certain method with nil checks will solve the
problem. See first patch attached.

Comment 1

[Josh Aas]

Attachment: crash log

Comment 2

[Josh Aas]

Attachment: do nil checks on restoreFolderExpandedStates

This should help.

Comment 3

[Greg K.]

Couldn't reproduce using Camino/2004041608. Toggled between regular and
Bookmarks views 100 times without crashing.

Comment 4

[Mike Pinkerton (not reading bugmail)]

i'm not sure what you're nil checking here. the crash isn't because the items
are nil (that's ok in the obj-c runtime) but because they're garbage pointers.
it'll still crash with your patch if that's the case.

also, the while loop might as well be a for loop, it exactly follows the
for-loop format. might make it easier to read.

Comment 5

[Andrew Thompson]

To be precise, Apple say this about messaging nil:

http://developer.apple.com/documentation/Cocoa/Conceptual/ObjectiveC/3objc_language_overview/chapter_3_section_3.html

"A message to nil also is valid, as long as the message returns an object; 
if it does, a message sent to nil returns nil. If the message sent to nil
returns anything other than an object, the return value is undefined."

I believe that as of Panther (at least) there's a gcc option that improves
performance by disabling the runtime check for nil, assuming you're confident
yur code *never* messages nil. I assume we're not using that option.

Of course, I'd tend to agree with Pink that it's unlikely messaging nil in this
method is causing the crash. Of course, I also note that this call:

[item isKindOfClass:[BookmarkFolder class]]

is returning BOOL not "an object" and as such the result of this call if 'item'
is nil is officially undefined. I'm willing to bet what they mean by this is
simply that it returns '0' but I suppose there's an outside chance that it could
crash...?

Comment 6

[Josh Aas]

Can't repro, going to wait for a better case if it ever comes up. Also, no
talkback reports match up with this.