158378 - After opening Info Bookmark window, attempting to delete bookmark/folder results in crash (@BookmarksService::LocateMenu(nsIContent *)

Status: VERIFIED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Chris Petersen]

Build: 2002-07-19-05
Platform: OS X 10.1.5
Expected Results: Item should be deleted from the sidebar without crashing the
application
What I got: Application immediately crashs after clicking Delete button.

Steps to reproduce:

1) Open sidebar
2) Select a folder or bookmark
3) Click on pencil icon to display Info window
4) Close Info window
5) Click on Delete icon. Message appears asking if you realy want to delete
bookmark. Click Delete.
6) Crash occurs.

Comment 1

[Chris Petersen]

Attachment: Stack trace

Comment 2

[Chris Petersen]

Regressed in today's build (2002-07-19-05)

Comment 3

[David Haas (gone, not reading mail)]

Ack - probably my bad, from patch that sfraser checked in yesterday.

I can reproduce this - but only if the bookmark/folder is the last bookmark (ie,
one furthest down in the list, end of the menu, etc).  Agreed?

It might be an easy fix.  I'll check.

Comment 4

[Chris Petersen]

Yes, that is correct. Last item bookmark or folder in sidebar.

Comment 5

[David Haas (gone, not reading mail)]

Attachment: fix, and then some

I'm so embarrassed.  I was sure I had fixed this stuff before submitting the
last patch.  Oh well.

I think the main problem was ControlTextDidEndEditing was firing more often
then expected. I thwacked it a couple ways.

First: Don't set things in Bookmark editing window if it's not visible.
Second: nil checks for the [bookmark contentNode], rather than [bookmark]
Third: Moved updating of bookmark data from TextDidEndEditing to TextDidChange
(this was the key part)

But, since now every time you type a letter, it instantly changes the actual
bookmark value, I figured I needed to add in Undo & Redo.  So that's in here
too.

Comment 6

[Mike Pinkerton (not reading bugmail)]

over to smfr to get landed.

Comment 7

[David Haas (gone, not reading mail)]

BTW - whenever somebody checks this out, do you like the automatically-updated
text of bookmarks, or would you rather there was an "OK" & "Cancel" button for
the edits?

Comment 8

[Simon Fraser [no longer active]]

I find it rather odd that this is a utility panel, and not just a dialog. As a
panel, it should *not* have OK/Cancel buttons. Turn it into a dialog, and it should.

Comment 9

[David Haas (gone, not reading mail)]

Attachment: new fix

Fixes crash, implements undo & redo for text changes, and changes from utility
panel to dialog.

Just to be difficult, part of a patch for bug 154081 ended up in here.	Same
method & all getting edited - so I was stuck.

Comment 10

[David Haas (gone, not reading mail)]

Attachment: nib for Info Panel

Here's the modified BookmarkInfoPanel.nib needed by "new fix"

Comment 11

[Mike Pinkerton (not reading bugmail)]

so do we want this to be a dialog or a panel? i guess it really doesn't matter
too much. I look at it as "an info panel you happen to be able to edit" rather
than "an edit panel that gives you info". The former should be a panel, a la
other apps that provide info. If we think it's more the latter, then it should
be a dialog.

ideas?

Comment 12

[David Haas (gone, not reading mail)]

Well, comment 4 of bug 148933 wondered why there wasn't an OK/Cancel button.
I wondered why there wasn't an OK/Cancel button.  Mozilla has an OK/Cancel 
button for modifying bookmarks.  

IE doesn't seem to have a dialog for this, and I don't have omniweb available,
so I can't really tell how other apps handle bookmarks.  

Comment 13

[Mike Pinkerton (not reading bugmail)]

ok, simon and i talked it over, we should leave it a panel. david, can you
provide a new patch? ;)

Comment 14

[Mike Pinkerton (not reading bugmail)]

i'm still a little confused how creating a NSTextView on its own acts as the
field editor for the window. The docs seem to imply that calling setFieldEditor:
causes the other text fields to respond to certain events to end editing, not
pick up functionality such as undo. Maybe the docs just assume you understand
all this and gloss over too much.

can you vend me a clue?

Comment 15

[David Haas (gone, not reading mail)]

Drat - I liked the buttons.  But I'll whip up a new patch in a couple of minutes.

As for the NSTextView serving as the window's field editor - the key is right here:

-(NSText *)windowWillReturnFieldEditor:(NSWindow *)aPanel toObject:(id)aObject
{
  return mFieldEditor;
}


The BookmarkInfoController is the delegate of the info window.  Whenever
something in the window asks for the built-in window field editor, the window
gives its delegate a chance to send back something else through this method.  In
this case, it always returns the NSTextView we made earlier - so that's why the
NSTextView acts as the window's field editor.  And NSTextViews have built in
undo/redo, unlike the NSText that normally acts as the field editor.

setFieldEditor:YES just makes sure the text view doesn't try to add a new line
when you hit return or insert a tab character when you press tab & stuff like
that.  Which is behavior you want for a field editor, but doesn't do diddly as
to actually making the NSTextView act as one for the window.

Comment 16

[David Haas (gone, not reading mail)]

Attachment: back to panel

OK, it's a panel again.

Comment 17

[Simon Fraser [no longer active]]

Patch is almost good to go, but for 2 things:
1. I don't think we should do 'live' updating while you type in the bookmarks
   panel. Just do it on tabbing out of a field, choosing a new bookmark, or
   closing the window or panel.

2. the title of the bookmarks info panel looks weird.

Comment 18

[Simon Fraser [no longer active]]

Also, we need to deal with deleting the bookmark that the panel is showing.

Comment 19

[Simon Fraser [no longer active]]

With this patch, I've crashed 4-5 times in hard to reproduce ways. Twice, I've
gotten this stack:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_PROTECTION_FAILURE (0x0002) at 0x00000020

Thread 0 Crashed:
 #0   0x706bab64 in objc_msgSend
 #1   0x70c02778 in -[NSLayoutManager(NSTextViewSupport)
layoutManagerOwnsFirstResponderInWindow:]
 #2   0x70e4466c in -[NSLayoutManager(NSPrivate)
_drawGlyphsForGlyphRange:atPoint:parameters:]
 #3   0x70bbcea8 in -[NSLayoutManager(NSTextViewSupport)
drawGlyphsForGlyphRange:atPoint:]
 #4   0x70e5437c in -[NSStringDrawingTextStorage
drawTextContainer:inRect:onView:pinToTop:]
 #5   0x70bc7870 in -[NSStringDrawingTextStorage drawTextContainer:inRect:onView:]
 #6   0x70bad9f8 in _NXDrawTextCell
 #7   0x70bb050c in -[NSTextFieldCell drawInteriorWithFrame:inView:]
 #8   0x70c57228 in -[NSTextFieldCell drawWithFrame:inView:]
 #9   0x70ba3444 in -[NSTableView drawRow:clipRect:]
 #10  0x70bdc34c in -[NSOutlineView drawRow:clipRect:]
 #11  0x70bba4cc in -[NSTableView drawRect:]
 #12  0x70c39cf8 in -[NSView _drawRect:clip:]
 #13  0x70bac7d0 in -[NSView
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
 #14  0x70baca00 in -[NSView
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
 #15  0x70baca00 in -[NSView
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
 #16  0x70baca00 in -[NSView
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
 #17  0x70baca00 in -[NSView
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
 #18  0x70baca00 in -[NSView
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
 #19  0x70baca00 in -[NSView
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
 #20  0x70bb0760 in -[NSFrameView
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
 #21  0x70be25c8 in -[NSView
_displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:]
 #22  0x70bfc4fc in -[NSView displayIfNeeded]
 #23  0x70c06040 in -[NSWindow displayIfNeeded]
 #24  0x70be31f0 in _handleWindowNeedsDisplay
 #25  0x7017ba90 in __CFRunLoopDoObservers
 #26  0x7017bdc0 in __CFRunLoopRun
 #27  0x701b70ec in CFRunLoopRunSpecific
 #28  0x7017b8cc in CFRunLoopRunInMode
 #29  0x7312d904 in RunEventLoopInModeUntilEventArrives
 #30  0x731407a4 in ReceiveNextEventCommon
 #31  0x731715fc in BlockUntilNextEventMatchingListInMode
 #32  0x70bd70b8 in _DPSNextEvent
 #33  0x70bfe5d8 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
 #34  0x70c23468 in -[NSApplication run]
 #35  0x70c91ed0 in NSApplicationMain
 #36  0x00002ebc in main
 #37  0x00002d50 in _start
 #38  0x00002b80 in start

Comment 20

[David Haas (gone, not reading mail)]

Comments:

1st: Could you check your BookmarkInfoPanel.nib file & make sure the delegate of 
the panel is BookmarkInfoController?  It should be.  I might have done this when
I made it a dialog, and then forgot that it had happen so didn't resubmit the
nib.  If it's not the delegate, that's probably where the crashes are coming
from.  My bad.

2nd: I think if the bookmark window is open, and you delete the bookmark it's
showing, it will just display the next bookmark selected, or if it's the last
bookmark and on the same level as the Toolbar Folder, the window just closes. 
At least that's what happens for me.  If that's not happening for you, I'm very
confused.

3rd: I spent some time trying to catch "window closing" messages for this panel.
 They never came.  I think that's because it's set to hide on deactive in PB. 
Anyway, it makes it tough to update bookmark edits on window closing if you
don't know when the window closes :)

4th: Updating on tab out/change tab: the message that gets sent then is for
controlTextDidEndEditing.  Unfortunately, it gets sent too frequently for our
purposes - whenever a new bookmark is selected, for example. This was the root
cause of the initial crash these patches were supposed to fix.  I couldn't come
up with a clean way to use controlTextDidEndEditing without having these crash
problems.  If you want, I can think about it some more, but the final result
will probably involve lots of posts to NSNotificationManager, which sorta seems
like GOTO for objective-c.  

5th: if I don't see window close messages, and I can't see a clean way to use
the method for new tab/tab out, i'm stuck doing instant updates.  

These problems don't exist if this is a dialog - then we just have to watch for
OK & Cancel button presses to handle updates.  That's another reason I liked
this as a dialog.

Comment 21

[David Haas (gone, not reading mail)]

Attachment: patch

here's the fix.  interacts with patch for bug 154081 - i hope this works ok
without that patch.

Comment 22

[Simon Fraser [no longer active]]

I checked this patch in, with some minor changes. I moved private methods into
an internal BookmarkInfoController(Private) interface. I moved the data saving
logic into a commitChanges() method, and refactored the code there into a single
commitField() (avoids duplicating code 4 times).

I also made it commit changes for a single text field when tabbing out of that
field.

Thanks again for the patch!

Comment 23

[Winnie Lam]

verified with 07-25 build on OS 10.1.5.