416564 - ctrl-z on any xul page results in bookmarks JS exception "aTarget is null" in onDragRemoveFeedBack

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Andrew Schultz]

Any any xul page (the URL field is an empty xul page), hitting ctrl-z results in:

[Exception... "'[JavaScript Error: "aTarget is null" {file: "chrome://communicator/content/bookmarks/bookmarksMenu.js" line: 707}]' when calling method: [nsIController::doCommand]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]

onDragRemoveFeedback is being called from BookmarksMenuController.doCommand, where aCommand is cmd_undo.  This bug means we'll throw the exception once bug 416561 lands, although the test will still pass.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Seems that the caller is expected to null-check.

Comment 2

[Andrew Schultz]

OK, I'm going to ask a dumb question...
Why does navigator.js do |controllers.appendController(BookmarksMenuController);| in the first place?

It's not what handles ctrl-D or ctrl-B.  If you do some bookmarks thing (copy/paste, delete, etc), and hit ctrl-Z (while focused on the content area), it doesn't undo it, even if you did the action from the bookmarks menu.

I commented on that line, and I don't see any adverse effects... OK, I thought maybe the sidebar needed that, but undo works there without the BookmarksMenuController.  It seems to do a lot of drag/drog stuff, but that seems to work fine without it as well (dragging a link into the menu works OK).

Comment 3

[[e-mail]]

The context menu needs it e.g. the properties menuitem's command attribute of cmd_bm_properties relates to a command element of that id whose oncommand event handler calls goDoCommand which looks up the command in the controllers.

Comment 4

[Andrew Schultz]

Hmm.  OK, that makes sense, but the context menu items (Bookmark this {Link,Page} still work in a build with the appendController line commented out.  Is that what it's there for?

Comment 5

[[e-mail]]

(In reply to comment #4)
>Hmm.  OK, that makes sense, but the context menu items (Bookmark this
>{Link,Page} still work in a build with the appendController line commented out.
Sorry, I meant the context menu for a bookmark needs the controller.

Comment 6

[Andrew Schultz]

right!  OK, I guess the fact that this stuff is getting called when I'm just messing with the content area is a bit disturbing.  Even with the patch, we're still calling into the bookmark undo code (which (thank goodness) doesn't find anything to undo).  Can we do:

 if (!document.popupNode) return;

?

Comment 7

[[e-mail]]

Navigator isn't the only window that uses the bookmarks controller...

Comment 8

[Andrew Schultz]

http://mxr.mozilla.org/seamonkey/search?string=BookmarksMenuController

only finds navigator.  And I can't think of any other window that has bookmarks in the menu.

Comment 9

[[e-mail]]

Oh right, the tree has its own controller.

Comment 10

[[e-mail]]

Attachment: Remove bogus line

OK, so it looks like destroyContextMenu does the honours anyway...

Comment 12

[[e-mail]]

Fix checked in.