Closed Bug 232827 Opened 20 years ago Closed 20 years ago

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

Categories

(SeaMonkey :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.7beta

People

(Reporter: durbacher, Assigned: p_ch)

Details

(Keywords: regression, Whiteboard: [Error text: see comment 3])

Attachments

(3 obsolete files)

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
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.
Keywords: regression
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...
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);
Summary: JS error: aTarget has no properties in bookmarks.js → JS error: aTarget has no properties in bookmarks.js line 856
[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...
Summary: JS error: aTarget has no properties in bookmarks.js line 856 → In <bookmarks.js>, "Error: aTarget has no properties" at line 856
Whiteboard: [Error text: see comment 3]
(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...
(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...
(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.
Attached patch (Av1) <bookmarks.js ++> (obsolete) — Splinter Review
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 |;|.
Attachment #142544 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #142544 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #142544 - Attachment is obsolete: true
Av1, with comment 9 suggestion(s).
Av1, with comment 9 suggestion(s).
Attachment #143184 - Attachment description: (Av2) <bookmarks.js ++> → (Av2) <bookmarks.js ++> (== att.143183)
Attachment #143184 - Attachment is obsolete: true
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+ }
Attachment #143183 - Flags: superreview?(jag)
Attachment #143183 - Flags: review+
Comment on attachment 143183 [details] [diff] [review]
(Av2) <bookmarks.js ++>
[Checked in: Comment 14]

sr=jag
Attachment #143183 - Flags: superreview?(jag) → superreview+
Fix checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Attachment #143183 - Attachment description: (Av2) <bookmarks.js ++> → (Av2) <bookmarks.js ++> [Checked in: Comment 14]
Attachment #143183 - Attachment is obsolete: true
Target Milestone: --- → mozilla1.7beta
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.