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 ?

.

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.

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.

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.

How about using view.selection rather than treeBoxObject.selection?

Thanks for the suggestion; tested it, and it works. Best of all, it means we don't have to throw an exception.

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

(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.

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 :-|

Yeah, it seems there are other places in this file where we could change it too. Do you volunteer to do it?

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.

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

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.

(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

Av2, with comment 19 suggestion(s).

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).

(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...

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.

(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 ?)

Bv1, updated to current tree.

Av2b, updated to current tree.

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).

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

(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 ?)

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.

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 [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".

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.

Comment on attachment 156684 [details] [diff] [review] (Av2c) <tree.xml> (/XpFE part) [Checked in: Comment 33] not a necessary fix for aviary.

Comment on attachment 156642 [details] [diff] [review] (Bv1a) <tree.xml> (/Toolkit part) [Checked in: Comment 39] not a necessary fix for 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 !?

attachment 156642 [details] [diff] [review] needs relanding, I think

Ben reverted his checkin to tree.xml, so this doesn't need to be relanded.

I don't think anyone is going to work on this given the plan to remove the XUL "tree" widget (bug 1446335).