232827 - In <bookmarks.js>, "Error: aTarget has no properties" at line 856

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Andreas Kunz]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113

This regressed between 20040113 and 20040115.
I strongly suspect the checkin for bug 228432 because it touched setting
bookmarksTree _target there, which is what fails to have a value.

The error originates in
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/bookmarksTree.xml#645
and propagates through "BookmarksController.onCommandUpdate(selection, target);"
in the same file. "target" is null there.

Reproducible: Always
Steps to Reproduce:
1. open the bookmarks manager
or
1. drag a bookmark from the bookmarks manager to the personal toolbar

Comment 1

[Andreas Kunz]

That checkin was by timeless, so cc'ing him.
I'm not sure of the results of this bug, but it would be nice if you could have
a look at it before 1.7a.

Comment 2

[timeless]

well erm, i just moved a warning. the case which triggered the warning should
presumably be the case which triggers this bug. i can't imagine my change
actually influenced the bug...

Comment 3

[Andreas Kunz]

Ok, I see...

Well, I forgot to mention the actual error message:
> Error: aTarget has no properties
> Source File: chrome://communicator/content/bookmarks/bookmarks.js
> Line: 856

The source is in isCommandEnabled:
    case "cmd_bm_newbookmark":
    case "cmd_bm_newfolder":
    case "cmd_bm_newseparator":
856     return BookmarksUtils.isValidTargetContainer(aTarget.parent);

Comment 4

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20040219] (W98SE)

I'm not sure of "my" testcase, but I already got this error twice...

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> i can't imagine my change actually influenced the bug...

Well, it did :-<
Confirming that it's a "regression" from bug 228432 attachment 137392 [details] [diff] [review].


(In reply to comment #4)
> [Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20040219] (W98SE)
> 
> I'm not sure of "my" testcase, but I already got this error twice...

Actually, I get it quite often:
opening B.M. (as noted in comment 0), and much more.

I noticed that this involved code is executed twice during the B.M. opening:
the error happens the second time only...

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> 
> I noticed that this involved code is executed twice during the B.M. opening:
> the error happens the second time only...

My mistake:
It happens the FIRST time only, because |selection.length| equals |0| on this
first call...

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> 
> I noticed that this involved code is executed twice during the B.M. opening:

Assuming that this is the intended behaviour;
I believe that timeless's patch revealed this bug, more than created it.

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <bookmarks.js ++>

The 2 |aTarget &&| fix this bug: fixing L856 reveals L817 !

Plus:
*a few "used once" variable removals;
*1 unused |var target| removal;
*1 |return| was missing a |;|.

Comment 9

[[e-mail]]

Comment on attachment 142544 [details] [diff] [review]
(Av1) <bookmarks.js ++>

>@@ -973,10 +974,9 @@ var BookmarksController = {
>                     "cmd_bm_sortfolder", "cmd_bm_sortfolderbyname",
>                     "cmd_undo", "cmd_redo", "cmd_bm_undo", "cmd_bm_redo"];
>     for (var i = 0; i < commands.length; ++i) {
>-      var enabled = this.isCommandEnabled(commands[i], aSelection, aTarget);
>       var commandNode = document.getElementById(commands[i]);
>      if (commandNode) { 
>-        if (enabled) 
>+        if (this.isCommandEnabled(commands[i], aSelection, aTarget))
>           commandNode.removeAttribute("disabled");
>         else 
>           commandNode.setAttribute("disabled", "true");
Bonus points to the person who checks this in for fixing the indent of
if (commandNode) {
to match the line above.

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <bookmarks.js ++> [Checked in: Comment 14]

Av1, with comment 9 suggestion(s).

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <bookmarks.js ++> (== att.143183)

Av1, with comment 9 suggestion(s).

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 143183 [details] [diff] [review]
(Av2) <bookmarks.js ++>
[Checked in: Comment 14]


Keeping { (Av1) <bookmarks.js ++>	 patch		2004-02-28 11:52 PST   
neil.parkwaycc.co.uk: review+ }

Comment 13

[jag (Peter Annema)]

Comment on attachment 143183 [details] [diff] [review]
(Av2) <bookmarks.js ++>
[Checked in: Comment 14]

sr=jag

Comment 14

[[e-mail]]

Fix checked in.