628938 - Treewalker not working properly in Mozilla Treewalker Demo

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Leugner]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20110103 Gentoo Firefox/3.6.13
Build Identifier: Firefox 40b10

If you go to http://www-archive.mozilla.org/docs/dom/samples/treewalkerdemo.xml and press the "All datatypes" button with FF Beta 10, you only get the first type node, while in FF 3 you get all the type nodes in the example XML.

This bug is a follow up from Bug 625722, where Treewalker froze in a demo.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www-archive.mozilla.org/docs/dom/samples/treewalkerdemo.xml
2. Press the "All datatypes" button
Actual Results:  
FF 4 shows only the first datatype node, while it should show all like in FF 3.

Expected Results:  
Show all datatype nodes in the example XML

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Craig: Can you look into this?

Comment 2

[Craig Topper]

Looking into it. I see at least one problem. Will try out a fix later tonight.

Comment 3

[Craig Topper]

Basic problem is that as we descend into child nodes trying to find one that would be accepted, we don't update "node" to keep track of our descent. So on our way back up we started with the original "node" from before the descent. Causing us to miss siblings of nodes we walked down through.

I have a patch, but I need to run it through the other tests before I upload it.

Comment 4

[Craig Topper]

Attachment: Patch

This point makes "node" always point to a valid node that is update as we move through the tree. This allows us to be able to always find out way back. Previously we were only updating "sibling" as we went down the tree, but it could become null making it impossible to recover. Doing it this way, sibling is no longer needed to be an nsCOMPtr because as soon as we know it's non-null node we copy it to "node" which is an nsCOMPtr.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

This really needs some tests.
Mochitestifying http://www-archive.mozilla.org/docs/dom/samples/treewalkerdemo.xml
would be good.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 507790 [details] [diff] [review]
Patch

r=me, but we definitely need more tests here. Craig, would you have time to write something comprehensive up?

Comment 7

[Craig Topper]

I'll try to put some tests together this week.

Comment 8

[Peter Leugner]

Sorry to be so unpatient, but are there any updates on this? I need this functionality in my addon and it's still not working in the current beta.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

We need some tests here before the patch can land.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Peter, if Craig is swamped and you have time to help out by writing the tests, that would probably help a lot!

Comment 11

[Mounir Lamouri (:mounir)]

I'm planning to write a test so if you want to jump into this, Peter, let me know.

Comment 12

[Peter Leugner]

I've never written tests before for mozilla. What would be required? I have a fulltime job, so I could only do it in the evenings (if my wife doesn't kill me).

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to comment #12)
> I've never written tests before for mozilla. What would be required? I have a
> fulltime job, so I could only do it in the evenings (if my wife doesn't kill
> me).

If you never wrote a test now isn't the best moment but you I invite you to look at the test I'm going to attach and at https://developer.mozilla.org/en/Mochitest if you want to learn.

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Tests

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/8b140daba936
http://hg.mozilla.org/mozilla-central/rev/1c9cd44cff88