76502 - Mozilla Freezes when trying to delete bookmarks in Bookmark Manager

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect, P1)

Description

[Alex]

Mozilla Freezes everytime when trying to delete bookmarks in Bookmark Manager
using the delete button on the keyboard.

Steps to Reproduce:
1. Launch Mozilla and open the Bookmarks Manager from the Bookmarks menu.
2. Try to delete a bookmark, or folder, with the delete key.

Expected Result:
The bookmark or folder gets deleted, and life goes on ...

Actual Result:
Mozilla freezes

Tested on:
Build 2001041804
Windows 98 SE

Comment 1

[doctor__j]

I cannot reproduce that bug using Win32 build 2001041704.

Comment 2

[Alex]

Build 2001041704 is yesterdays build, and I was able to delete bookmarks just
fine yesterday. The problem I just described is in build 2001041804.

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

oops. I'll look at this this evening. 

Comment 4

[Koike Kazuhiko]

Reproduced with 2001041808/Linux.

Comment 5

[Claudius Gayle]

bad-->worse. in the 4-19 builds the delete button and delete context menuitem do nothing,
while pressing the delete button continues to cause the app to hang. All/All

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

This is likely caused by my renaming the edit commands and forgetting to rename 
a couple. I'm working on a fix right now. 

Comment 7

[Matthew Cline]

I've gotten this with Linux build 2001041908.  The stack trace for where
Mozilla seems to be stuck is:

#0  0x400db9d8 in nsAString::do_AppendFromReadable () at eval.c:41
#1  0x400dbe46 in nsAString::do_AppendFromElementPtrLength () at eval.c:41
#2  0x4008e862 in AtomImpl::ToString () at eval.c:41
#3  0x40c9e11d in nsXULAttributeValue::GetValue ()
   from mozilla/components/libgkcontent.so
#4  0x40c9ebb7 in nsXULAttribute::GetValue ()
   from mozilla/components/libgkcontent.so
#5  0x40ca62ef in nsXULElement::GetAttribute ()
   from mozilla/components/libgkcontent.so
#6  0x40ca6207 in nsXULElement::GetAttribute ()
   from mozilla/components/libgkcontent.so
#7  0x40cf2013 in nsXBLWindowHandler::WalkHandlersInternal ()
   from mozilla/components/libgkcontent.so
#8  0x40cf2a19 in nsXBLWindowKeyHandler::WalkHandlers ()
   from mozilla/components/libgkcontent.so
#9  0x40cf2aba in nsXBLWindowKeyHandler::KeyPress ()
   from mozilla/components/libgkcontent.so
#10 0x40bcf913 in nsEventListenerManager::HandleEvent ()
   from mozilla/components/libgkcontent.so
#11 0x40cba3ac in nsXULDocument::HandleDOMEvent ()
   from mozilla/components/libgkcontent.so
#12 0x40ca8530 in nsXULElement::HandleDOMEvent ()
   from mozilla/components/libgkcontent.so
#13 0x40ca8530 in nsXULElement::HandleDOMEvent ()
   from mozilla/components/libgkcontent.so
#14 0x40f10ec7 in PresShell::HandleEventInternal ()
   from mozilla/components/libgklayout.so
#15 0x40f10d5c in PresShell::HandleEvent ()
   from mozilla/components/libgklayout.so
#16 0x4103a62a in nsView::HandleEvent ()
   from mozilla/components/libgkview.so
#17 0x41043cae in nsViewManager::DispatchEvent ()
   from mozilla/components/libgkview.so
#18 0x41039fbd in HandleEvent ()
   from mozilla/components/libgkview.so
#19 0x40564cba in nsWidget::DispatchEvent ()
   from mozilla/components/libwidget_gtk.so
#20 0x40564be5 in nsWidget::DispatchWindowEvent ()
   from mozilla/components/libwidget_gtk.so
#21 0x40562f91 in nsWidget::OnInput ()
   from mozilla/components/libwidget_gtk.so
#22 0x4055f4b0 in handle_key_press_event ()
   from mozilla/components/libwidget_gtk.so
#23 0x4055f98f in dispatch_superwin_event ()
   from mozilla/components/libwidget_gtk.so
#24 0x4055f6fc in handle_gdk_event ()
   from mozilla/components/libwidget_gtk.so
#25 0x406f5d60 in gdk_event_free () from /usr/lib/libgdk-1.2.so.0
#26 0x4072aec1 in g_main_iterate () from /usr/lib/libglib-1.2.so.0
#27 0x4072b08c in g_main_run () from /usr/lib/libglib-1.2.so.0

Comment 8

[André Dahlqvist]

I tried this with 2001-04-20-10, and pressing 'Del' doesn't freeze the browser
any more. BUT it doesn't delete the bookmark either. 'Delete' in the context
menu works just fine however.

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Hm. Renaming the command fixes the button etc, but I still see the hang in my 
older tree. Updating to the tip again to see if I can verify what Andre is 
seeing. 

Comment 10

[Alex]

I see the exact same thing that André described on Win98SE with Build 2001042012.

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: Fix this, and a barrell of other bugs.

Comment 12

[André Dahlqvist]

Ben, I applied this patch locally and I can verify that it fixes this bug.
Build ID: 2001042113 on Linux.

Is there a way to split out the fix for only this bug, or do the changes
together make this bugfix possible?

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

Well there are several parts of the patch that fix the bug, and the broken 
clipboard ops (which is also a Bad Thing). I'm not sure exactly which now, but 
there are several, scattered here and there ;) 

Comment 14

[Alec Flett]

ok, sr=alecf

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: THIS IS NOT A REAL PATCH. The minimal change to fix the command problems.

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: minimaler NOT-patch

Comment 17

[chris hofmann]

need to get a bunch of people to run with the patch to help
reduce risk of taking this big of a patch in 0.9...

we could also take this patch in 0.9.1 after the branch
and the pull to the branch if no one spots side effects.

Comment 18

[Chris Waterson]

Did you mean to comment this out? If so, how come? (Either just remove it
altogether, or add a comment above; e.g., ``we don't remember the focusElt here
because it would hurt the focus manager's poor little brain.'')

-      gBookmarksShell._focusElt = document.commandDispatcher.focusedElement;
+      //gBookmarksShell._focusElt = document.commandDispatcher.focusedElement;


Same here. Does the comment lie now?

       // We need to do this because somehow focus shifts and no commands 
       // operate any more. 
-      gBookmarksShell._focusElt.focus();
+      //gBookmarksShell._focusElt.focus();


And here:

+    nodeIsValidType: function (aNode)
+    {
+      switch (aNode.localName) {
+      case "button":
+      case "menubutton":
+      // case "menu":
+      // case "menuitem":

Looks ok otherwise, from the little I can understand.

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

I've had blake test the smallest patch (not really a patch, but he hand applied 
it) and he's verified that the key and the context menu item work (although 
strangely the button and edit menu item don't, although they work with the full 
patch). 

Good catch with the first commented out portion. The code in question should 
not be necessary, but a focus/controller bug means that it appears to be.

The other commented out section merely indicates future intent (to handle other 
node types when context menus on menus may be supported)

Comment 20

[Ben Goodger (use ben at mozilla dot org for email)]

I'd rather take the smallest possible change here, as there are parts of the 
larger patch that are no longer desirable. If go is given for the third fix, I 
can apply this in my tree and check in.

Comment 21

[Viswanath Ramachandran]

adding nsbeta1+ P1. 

Comment 22

[chris hofmann]

a=drivers when ready.

Comment 23

[Asa Dotzler [:asa]]

blake, you've applied and tested this?  Can you give it a r=?
waterson, can you sr (again) the smaller patch? thanks.

Comment 24

[Asa Dotzler [:asa]]

*** Bug 77406 has been marked as a duplicate of this bug. ***

Comment 25

[Alec Flett]

ok, this looks fine to me.
I don't quite understand why we have to specifiy bookmark-specific versions of
each of these commands but for moz 0.9 this is ok. sr=alecf

Comment 26

[André Dahlqvist]

Is it too late to land this on the branch? We still need a r= from someone,
right?

Comment 27

[Paul Chen]

If we're still thinking about this one for 0.9, r=pchen

Comment 28

[Asa Dotzler [:asa]]

blake says this doesn't work.  Ben can you take another look at this please.

Comment 29

[André Dahlqvist]

Here is the status of this in a CVS build from 2001-05-01:

o 'Del' on keyboard doesn't work. No freeze however.

o 'Edit -> Delete' doesn't work either.

o 'Delete' on the toolbar doesn't work.

o 'Delete' in the context menu DOES work.

To summarize: the context menu is currently the only way to remove bookmarks.
No messages are printed to the xterm during the other failed attempts by the way.

Comment 30

[Asa Dotzler [:asa]]

0.9 is wrapping up, pushing milestone out to 0.9.1 (please excues the trespass)

Comment 31

[Koike Kazuhiko]

*** Bug 78586 has been marked as a duplicate of this bug. ***

Comment 32

[ratman]

a note to all:  since this bug seems to have morphed into more than its summary,
i would recommend checking out bug 72845, which has significant detail regarding
bookmark management errors.

Comment 33

[André Dahlqvist]

Could we perhaps change the Summary of this bug to something like "Deleting
bookmarks in Bookmark Manager doesn't work"? That's more accurate (although it
works using the context menu).

Comment 34

[Hong Kwon]

The summary exactly describes the symptoms of the problem.

Comment 35

[André Dahlqvist]

[e-mail], no it doesn't. It doesn't freeze for me like it used to when
I try to delete a bookmark.

Comment 36

[Lynn Webb]

The summary doesn't seem to be correct. Mozilla doesn't freeze for me on a 
WinNT. The issue here is that the Delete key functionality doesn't work with 
Mozilla. This functionality should be added.

Comment 37

[Hong Kwon]

The bug, as filed is:

"Mozilla freezes every time when trying to delete bookmarks." (original 
reporter)

If the problem is now:

"The issue here is that the Delete key functionality doesn't work with 
Mozilla. This functionality should be added." (lynnw)

then a separate bug should be filed.  My point is not that your reported issue 
doesn't exist, but rather that it should be reported in another bug.  Changing 
the summary of one bug report to address multiple bugs is not the proper way to 
track an issue to resolution.  If the hang has been alleviated, this bug is 
FIXED and another bug exists.  

Comment 38

[Lynn Webb]

The Delete key functionality bug was filed as 78586, which someone marked as a 
duplicate of this bug. So, is 78586 a duplicate, or is this the same issue?

Comment 39

[Paul Chen]

nav triage team:

Marking W4M because we no longer hang trying to delete, but delete doesn't work. 
That's bug 78586.

Comment 40

[Hong Kwon]

verifying WFM.

Comment 41

[Ben Goodger (use ben at mozilla dot org for email)]

do not close this just yet, as I'm using it. It has a patch in it that fixes 
other bugs. If you want to mark fixed please relocate the patch elsewhere. 

Comment 42

[Ben Goodger (use ben at mozilla dot org for email)]

Fixed now.

Comment 43

[hirata masakazu]

Is this the cause of regression bug 42080?

Comment 44

[Claudius Gayle]

there's no freeze anymore and I've verified bug 78586 to boot.
VERIFIED Fixed with 20010511 builds on all platforms