Closed Bug 211046 Opened 21 years ago Closed 6 years ago

In <chrome://global/content/bindings/tree.xml>, "Error: this.treeBoxObject.selection has no properties"

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
mozilla1.8alpha4

People

(Reporter: sgautherie, Assigned: janv)

References

Details

(Keywords: testcase, Whiteboard: [See comment 35 when closing this bug])

Attachments

(1 file, 7 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4) Gecko/20030624

Saw this once.

Reproducible: Didn't try

Steps to Reproduce:

Actual Results:  
{
Error: this.treeBoxObject.selection has no properties
Source File: chrome://global/content/bindings/tree.xml#tree.currentIndex (getter)
Line: 0
}


Expected Results:  
No JS error.


Cluttering JS Console.
Other bad effect ?
.
Assignee: general → varga
Component: Browser-General → XP Toolkit/Widgets: Trees
QA Contact: general → shrir
Happened again:
I still don't know how, but I'll try to pay attention...
Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.6b) Gecko/20031120

I'm seeing this a lot now too, in my development of a widget; investigating.  My
testcase which shows the error is not a minimized testcase, so don't expect the
attachment soon.
OS: Windows 95 → All
Attached file testcase
Minimal testcase.  Steps to reproduce:
(1) Open the attachment in Mozilla.

The tree box object's selection is somehow null when the tree is inside the
binding's content, and the binding's constructor element looks for the tree's
current index.
Here's what happens:
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/tree.xml#51

Calling tree.xml#tree -> currentIndex references
tree.treeBoxObject.selection.currentIndex.  At that point, there is no selection
in the tree box object; it's a null.  Why?

http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/tree/src/nsTreeBoxObject.cpp#228
This code specifies how we get a selection.  We actually get the tree body to
give us the selection.

Lines 144-171 of nsTreeBoxObject.cpp define the related GetTreeBody() function,
but as far as I can tell, it feeds back on itself.  It looks like it should be
in an infinite loop and thus crashing.  Obviously it's not, as it does return a
response fairly quickly.  It eventually returns an object whose selection
property is occasionally null...

Note that if it were C++ code calling the currentIndex of the selection instead
of JS, I would be posting a stack trace.

The most obvious fix for this to to change the tree.xml binding so
tree.currentIndex does a sanity check for this.treeBoxObject.selection != null.
 I would write up a patch for that right now, except I am not convinced it
wouldn't conceal a bug in nsTreeBoxObject.cpp.
Keywords: testcase
After a little more searching (and consultation with db48x), I've discovered the
nsTreeBoxObject instance is created only when a view is being set for the
corresponding TreeBodyFrame.

http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#728

That tree body frame is itself only constructed from the CSS Frame Constructor,
which defines appearance.

So basically, the tree.xml binding is calling on the tree selection that doesn't
get created until after the page's implementations (DOM, XBL bindings, etc)
finish, but an XBL binding, through its constructor (which is implementation)
calling said something will do so before the tree selection is initialized, by
the tree body frame's view being initialized.

This is a guess; it makes sense to have the implementation working before you
have the appearance of anything the implementation is supposed to implement.

Based on that, I'd say the C++ code is working as intended, and the bug is in
the XBL binding.  Patch coming up in a few minutes.
Attached patch (Av1) patch, v1 (obsolete) — Splinter Review
Attachment #136151 - Flags: review?(varga)
How about using view.selection rather than treeBoxObject.selection?
Attached patch (Av1b) patch using tree.view (obsolete) — Splinter Review
Thanks for the suggestion; tested it, and it works.  Best of all, it means we
don't have to throw an exception.
Attachment #136151 - Attachment is obsolete: true
Attachment #136180 - Flags: review?(varga)
Attachment #136151 - Flags: review?(varga)
Comment on attachment 136180 [details] [diff] [review]
(Av1b) patch using tree.view

r=varga
I did something similar for bug 231733, so it will be included in my checkin
Attachment #136180 - Flags: review?(varga) → review+
(In reply to comment #10)
> I did something similar for bug 231733, so it will be included in my checkin

Updating:
+(d.o.) 231733, as a reminder in the meantime.
Depends on: 231733
Jan:

I guess this, like bug 231733, was fixed by
{{
<http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpfe/global/resources/content/bindings/tree.xml>
1.34	varga%nixcorp.com	2004-04-16 22:53	 	Fix for bug 221619. Tree widget
refactoring and enhancement. r=neil sr=bryner
}}

I notice that you changed your mind from
{{
+                onget="return this.view.selection.currentIndex;"
+                onset="return this.view.selection.currentIndex=val;"/>
}}
to
{{
 63                 onget="return this.treeBoxObject.view.selection.currentIndex;"
 64                 onset="return
this.treeBoxObject.view.selection.currentIndex=val;"/>
}}
in the meantime :-|
Hardware: PC → All
Yeah, it seems there are other places in this file where we could change it too.
Do you volunteer to do it?
Attachment #136151 - Attachment description: patch, v1 → (Av1) patch, v1
Attachment #136180 - Attachment description: patch using tree.view → (Av1b) patch using tree.view
Attachment #136180 - Attachment is obsolete: true
Attached patch (Av2) <tree.xml> (obsolete) — Splinter Review
Av1b, with comment 13 suggestion(s);
and a |var column| redeclaration fix.

I also added a missing space indentation to the modified lines.
Comment on attachment 146534 [details] [diff] [review]
(Av2) <tree.xml>

'r=?': (see comment 14)
This trunk version file is not suited to my Mv1.7b:
Could you test/review this patch ? Thanks.
Attachment #146534 - Flags: review?(varga)
Comment on attachment 146534 [details] [diff] [review]
(Av2) <tree.xml>

-		 onset="return this.treeBoxObject.view=val;"/>
+		 onset="return (this.treeBoxObject.view = val);"/>

Why is this needed, JS strict warnings?
r=varga
Attachment #146534 - Flags: review?(varga) → review+
Attachment #146534 - Flags: superreview?(jag)
and please update toolkit/content/widgets/tree.xml too
(In reply to comment #16)
> (From update of attachment 146534 [details] [diff] [review])
> -		 onset="return this.treeBoxObject.view=val;"/>
> +		 onset="return (this.treeBoxObject.view = val);"/>
> 
> Why is this needed, JS strict warnings?

No warning: only to look the same (more readable !?) as
{{
-                onset="return
this.treeBoxObject.view.selection.currentIndex=val;"/>
+                onset="return (this.view.selection.currentIndex = val);"/>
}}
I don't think you should to wrap it with ().
Comment on attachment 146534 [details] [diff] [review]
(Av2) <tree.xml>

	  var c = this.currentIndex;
-	  var l = this.treeBoxObject.view.rowCount - 1;
+	   var l = this.view.rowCount - 1;
	  if (c == l)

and

	  var c = this.currentIndex;
-	  try { if (c+1 == this.treeBoxObject.view.rowCount)
-	    return;
+	   try {
+	     if (c+1 == this.view.rowCount)
+	       return;

Indentation nit in several places. Looks like whoever wrote this code in the
first place only indented one space after the <![CDATA[.

Jan, wanna fix that (in a separate checkin perhaps)?

And I agree, get rid of the () around the onset return.

sr=jag with that.
Attachment #146534 - Flags: superreview?(jag) → superreview+
(In reply to comment #20)
> Indentation nit in several places. Looks like whoever wrote this code in the
> first place only indented one space after the <![CDATA[.
> 
> Jan, wanna fix that (in a separate checkin perhaps)?
ok, will do
Attached patch (Av2b) <tree.xml> (/XpFE part) (obsolete) — Splinter Review
Av2, with comment 19 suggestion(s).
Attachment #146534 - Attachment is obsolete: true
Comment on attachment 147569 [details] [diff] [review]
(Av2b) <tree.xml> (/XpFE part)


Keeping:
{{
(Av2) <tree.xml>	 patch		2004-04-19 15:57 PDT	varga: review+
jag: superreview+
}}

Not fixing the "Indentation nits" (comment 20), as the rest of the file will be
fixed (comment 21).
Attachment #147569 - Flags: superreview+
Attachment #147569 - Flags: review+
(In reply to comment #21)
> (In reply to comment #20)
> > Indentation nit in several places. Looks like whoever wrote this code in the
> > first place only indented one space after the <![CDATA[.
> > 
> > Jan, wanna fix that (in a separate checkin perhaps)?
> ok, will do

I'm doing...
(In reply to comment #17)
> and please update toolkit/content/widgets/tree.xml too
I'll do too, in yet another patch to come...
Attached patch (Bv1) <tree.xml> (/Toolkit part) (obsolete) — Splinter Review
Av2b patch applied to FireFox.
Comment on attachment 147832 [details] [diff] [review]
(Bv1) <tree.xml> (/Toolkit part)


I don't use FireFox: Could you test/review/check in this patch ? Thanks.
Attachment #147832 - Flags: review?(p_ch)
Attachment #147569 - Attachment description: (Av2b) <tree.xml> → (Av2b) <tree.xml> (Moz-Suite part)
(In reply to comment #24)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > Indentation nit in several places. Looks like whoever wrote this code in the
> > > first place only indented one space after the <![CDATA[.
> > > 
> > > Jan, wanna fix that (in a separate checkin perhaps)?
> > ok, will do
> 
> I'm doing...

Well, I won't eventually :-( helpwanted (Jan ?)
Attachment #147832 - Attachment description: (Bv1) <tree.xml> (FireFox part) → (Bv1) <tree.xml> (Aviary part)
Attachment #147832 - Attachment is obsolete: true
Attachment #147832 - Flags: review?(p_ch)
Bv1, updated to current tree.
Attachment #156642 - Flags: review?(bryner)
Av2b, updated to current tree.
Attachment #147569 - Attachment description: (Av2b) <tree.xml> (Moz-Suite part) → (Av2b) <tree.xml> (SeaMonkey part)
Attachment #147569 - Attachment is obsolete: true
Comment on attachment 156684 [details] [diff] [review]
(Av2c) <tree.xml> (/XpFE part)
[Checked in: Comment 33]


Keeping:
{{
(Av2) <tree.xml>	 patch		2004-04-19 15:57 PDT	varga: review+
jag: superreview+
}}

Not fixing the "Indentation nits" (comment 20), as the rest of the file will be
fixed (comment 21).
Attachment #156684 - Flags: superreview+
Attachment #156684 - Flags: review+
Comment on attachment 156642 [details] [diff] [review]
(Bv1a) <tree.xml> (/Toolkit part)
[Checked in: Comment 39]


I don't use "Aviary": Could you test/review/check in this patch ? Thanks.
Comment on attachment 156684 [details] [diff] [review]
(Av2c) <tree.xml> (/XpFE part)
[Checked in: Comment 33]

checked in
aviary != mozilla/toolkit, aviary is just a branch in the cvs repository
anyway, r=varga on the mozilla/toolkit patch
Attachment #156684 - Attachment description: (Av2c) <tree.xml> (SeaMonkey part) → (Av2c) <tree.xml> (/XpFE part) [Checked in: Comment 33]
Attachment #156684 - Attachment is obsolete: true
Attachment #147569 - Attachment description: (Av2b) <tree.xml> (SeaMonkey part) → (Av2b) <tree.xml> (/XpFE part)
Attachment #147832 - Attachment description: (Bv1) <tree.xml> (Aviary part) → (Bv1) <tree.xml> (/Toolkit part)
Attachment #156642 - Attachment description: (Bv1a) <tree.xml> (Aviary part) → (Bv1a) <tree.xml> (/Toolkit part) [Checked in: Comment n]
Attachment #156642 - Flags: superreview?(jag)
Attachment #156642 - Flags: review?(varga)
Attachment #156642 - Flags: review?(bryner)
Attachment #156642 - Attachment description: (Bv1a) <tree.xml> (/Toolkit part) [Checked in: Comment n] → (Bv1a) <tree.xml> (/Toolkit part)
(In reply to comment #13)
> Yeah, it seems there are other places in this file where we could change it too.
> Do you volunteer to do it?

Jan:
Now, should the same pattern be applied to <bookmarksTree.xml> !?
(If yes, can I morph this bug, or should I file a new one ?)
Severity: major → normal
Target Milestone: --- → mozilla1.8alpha4
new one please, thanks
Comment on attachment 156642 [details] [diff] [review]
(Bv1a) <tree.xml> (/Toolkit part)
[Checked in: Comment 39]

Marking r=varga. Not sure I can sr= toolkit/ and browser/ stuff, let me find
out.
Attachment #156642 - Flags: review?(varga) → review+
Blocks: 253607
Attachment #156642 - Flags: superreview?(jag) → superreview?(mconnor)
Comment on attachment 156642 [details] [diff] [review]
(Bv1a) <tree.xml> (/Toolkit part)
[Checked in: Comment 39]

r=me as well for toolkit, I don't remember jan's status as a reviewer (hell,
mine isn't defined in writing anywhere either)
Attachment #156642 - Flags: superreview?(mconnor) → superreview+
attachment 156642 [details] [diff] [review] is now checked in, with the merge conflict fixed. I can't tell
whether this bug is fixed now - please mark it so if it is. thanks.
Comment on attachment 156642 [details] [diff] [review]
(Bv1a) <tree.xml> (/Toolkit part)
[Checked in: Comment 39]

'approval-aviary=?':
Trivial U.I. code cleanup, no risk.
Has been in Mozilla for some time, is now in "Toolkit".
Attachment #156642 - Attachment description: (Bv1a) <tree.xml> (/Toolkit part) → (Bv1a) <tree.xml> (/Toolkit part) [Checked in: Comment 39]
Attachment #156642 - Attachment is obsolete: true
Attachment #156642 - Flags: approval-aviary?
Whiteboard: [See comment 35 when closing this bug]
Comment on attachment 156684 [details] [diff] [review]
(Av2c) <tree.xml> (/XpFE part)
[Checked in: Comment 33]

'approval1.7.x=?':
Trivial U.I. code cleanup, no risk.
Has been in Mozilla for some time.
Attachment #156684 - Flags: approval1.7.x?
Comment on attachment 156684 [details] [diff] [review]
(Av2c) <tree.xml> (/XpFE part)
[Checked in: Comment 33]

not a necessary fix for aviary.
Attachment #156684 - Flags: approval1.7.x? → approval1.7.x-
Comment on attachment 156642 [details] [diff] [review]
(Bv1a) <tree.xml> (/Toolkit part)
[Checked in: Comment 39]

not a necessary fix for aviary.
Attachment #156642 - Flags: approval-aviary? → approval-aviary-
(In reply to comment #42)
> (From update of attachment 156684 [details] [diff] [review])
> not a necessary fix for aviary.

I guess you meant v1.7.x !?
Ben reverted his checkin to tree.xml, so this doesn't need to be relanded.
Keywords: aviary-landing
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: shrir → xptoolkit.widgets
I don't think anyone is going to work on this given the plan to remove the XUL "tree" widget (bug 1446335).
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: