456186 - crash when closing tab containing bookmarks manager

Status: VERIFIED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[philippe (part-time)]

Attachment: crash log

20080920-00 build, FKA on

STR
1. have a tab with some web content
2. open tab, load history (Cmd Y)
3. open tab, load bookmarks manager (Cmd B) -> active tab
4. set focus in location bar
5. tab through (BM bar, open tabs)

When you reach the active tab - containing the BM manager, notice that you can't tab anywhere (or only into dev/null ?). tab a few more times.

Try to close the BM manager (Cmc-W, or with the mouse).
--> crash

That is a regression from the recent tab-chain landings. 20080919-00 doesn't crash.

Comment 1

[Stuart Morgan]

> When you reach the active tab - containing the BM manager, notice that you
> can't tab anywhere

Filed as bug 456195

Comment 2

[Sean Murphy]

The crash happens when the deallocated CHBrowserView is still the window's firstResponder and then sent a message by some automatic AppKit behavior:

2008-09-20 22:51:14.808 Camino[33163:10b] *** -[CHBrowserView respondsToSelector:]: message sent to deallocated instance 0x1aeb5660

Stack:
#0  0x96b49907 in ___forwarding___ ()
#1  0x96b49a12 in __forwarding_prep_0___ ()
#2  0x9247cf90 in +[NSInputContext currentInputContext] ()
#3  0x9247cedd in +[NSInputContext updateInputContexts] ()
#4  0x9247c96d in -[NSApplication updateWindows] ()
#5  0x9247c897 in _handleWindowsNeedUpdateNote ()
#6  0x96ac89c2 in __CFRunLoopDoObservers ()
#7  0x96ac9d1c in CFRunLoopRunSpecific ()
#8  0x96acacf8 in CFRunLoopRunInMode ()
#9  0x95593da4 in RunCurrentEventLoopInMode ()
#10 0x95593af6 in ReceiveNextEventCommon ()
#11 0x95593a31 in BlockUntilNextEventMatchingListInMode ()
#12 0x9247b505 in _DPSNextEvent ()
#13 0x9247adb8 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#14 0x92473df3 in -[NSApplication run] ()
#15 0x92441030 in NSApplicationMain ()
#16 0x00003650 in main (argc=1, argv=0xbffff634) at /Users/seanmurph/Software/Camino/CaminoObj/camino/src/application/main.m:75

So, throwing the bookmark editing view into the key view loop, instead of the CHBrowserView, when it is visible, will most likey fix this problem along with merely adding necessary behavior.

Comment 3

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

Blocking, per the meeting.

Comment 4

[Sean Murphy]

Attachment: Fix

The patch fixes the bug by actually integrating the BookmarkEditingView into the window's loop, excluding the CHBrowserView when a content view is provided instead.

On Leopard, tabbing will get stuck in the History view's ExtendedOutlineView.  It needs a fix identical to the one I submitted for ExtendedOutlineView: https://bugzilla.mozilla.org/attachment.cgi?id=296563&action=diff.

Comment 5

[Sean Murphy]

Attachment: BookmarksEditing.nib

Updated nib, with the next key view of the parent BookmarkEditingView itself set to the first subview, the Collections table view.

Comment 6

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

(In reply to comment #4)

> On Leopard, tabbing will get stuck in the History view's ExtendedOutlineView. 
> It needs a fix identical to the one I submitted for ExtendedOutlineView:
> https://bugzilla.mozilla.org/attachment.cgi?id=296563&action=diff.

That's bug 456806, right?

Comment 7

[Sean Murphy]

(In reply to comment #6)
> (In reply to comment #4)
> 
> > On Leopard, tabbing will get stuck in the History view's ExtendedOutlineView. 
> > It needs a fix identical to the one I submitted for ExtendedOutlineView:
> > https://bugzilla.mozilla.org/attachment.cgi?id=296563&action=diff.
> 
> That's bug 456806, right?

Yep, that's it :).

Comment 8

[Stuart Morgan]

Comment on attachment 340166 [details] [diff] [review]
Fix

r/sr=smorgan, since this fixes the regression and I don't want to delay a1.

It occurred to me though: couldn't we avoid sprinkling this lastKeySubview stuff everywhere by instead having these kind of views override setNextKeyView: to do
  [super setNextKeyView:<first internal view>]
  [<last internal view> setNextKeyView:<outside view>]
so that the users of these classe don't all need to know about and deal with this?

If so, let's get a follow-up bug to make that change here and in the other places you've added this kind of code.

Comment 9

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

Landed on cvs trunk.  Sean, please file the follow-up from comment 8 if applicable.

Comment 10

[philippe (part-time)]

works perfectly fine now.
v. with the 2008092600 build