378969 - ###!!! ASSERTION: ReplaceElementAt(negative index): 'aIndex >= 0', file nsVoidArray.cpp, line 491

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

STEPS TO REPRODUCE
0. enable caret browsing
1. load the URL
2. click on the text above the input fields
3. type KEY_DOWN, moving the caret through the text fields

ACTUAL RESULT
When the caret is in the text area, typing KEY_DOWN results in:

###!!! ASSERTION: ReplaceElementAt(negative index): 'aIndex >= 0', file nsVoidArray.cpp, line 491

The assertion also occurs when typing KEY_UP from the text below it.

PLATFORMS AND BUILDS TESTED
Bug occurs in Firefox trunk debug build on Linux

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack

Comment 2

[[e-mail]]

This appears to be triggered by nsAccessible::GetFirstAvailableAccessible
The code is not accessibility-specific, it can be triggered by JS.
The relevant portion of the code, transliterated to JS, is as follows:
  var walker = currentNode.ownerDocument.createTreeWalker(mDOMNode,
    NodeFilter.SHOW_ELEMENT | NodeFilter.SHOW_TEXT, null, false);
  walker.currentNode = currentNode;
  currentNode = walker.nextNode();
Note that the tree walker still manages to return a node.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Care to attach a JS testcase?  The steps to reproduce don't work for me, pssibly because caret browsing seems to be pretty much busted.

Comment 4

[[e-mail]]

Attachment: Test case

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, this just looks like a bug in treewalker.  We should either test aIndexPos here, or reset the whole index stack when SetCurrentNode happens, or something.  Not sure what's better.

Comment 6

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

We should do both actually, since the current node can move around in the tree. My intention was that the index cache would just contain hints that always have to be checked. But on SetCurrentNode we might as well blow away the cache since it's likely wrong anyway.

Comment 7

[Johnny Stenback (:jst)]

Sounds like a pretty harmless assertion, not a blocker. If someone comes up with a fix, please request approval for it, otherwise we'll ship with this...

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Sounds like a pretty harmless assertion, not a blocker. If someone comes up
> with a fix, please request approval for it, otherwise we'll ship with this...
> 

It would be very nice to fix this because I can see the assertion too often when I deal with accessibility where tree walker is used everytime, it makes a pain to debug accessibility code from time to time.

Comment 9

[Jeff Walden [:Waldo]]

I'll take a look at this in a week or so; this fell off my radar.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: like this?

Comment 11

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

Hmm.. it seems like a bug that SetChildIndex is ever called with -1 as index. Why does that happen?

Comment 12

[Jeff Walden [:Waldo]]

I plan to get to this this coming weekend; sorry for the delay so far.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 285876 [details] [diff] [review]
like this?

The patch as submitted is okay, but it doesn't have any tests for this behavior.  I'd like two tests for this: one as an xpcshell test, and one as a Mochitest.  The reason is that assertions cause xpcshell tests to fail, whereas assertions don't do anything in Mochitests (yet).  The tests can do exactly the same thing in their tests -- you can probably make the test a single function which takes as a parameter the DOM node/tree to use for treewalking -- so this shouldn't be that much extra work.  (See content/test/unit for some utilities for getting DOM trees for the xpcshell test.)

I'll more than gladly grant r+ to this patch with both those tests in it.  Sorry for the delay in reviewing this...

Comment 14

[alexander :surkov (:asurkov)]

So, why do I need a mochitest if 1) it won't fail on assertion and if 2) xpcshell test will test all we need?

Comment 15

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

We will hopefully soon make mochitests fail if they assert. Having xpcshell tests should technically be enough though. Unfortunately I'm not sure if we run those on tinderbox yet?

Comment 16

[Jeff Walden [:Waldo]]

Making Mochitests fail on assert is in the cards, but it's sadly probably a ways off.  I ran them a few days ago on Windows and got ~1100 assertions at at least 20-odd source locations.  Some of those have bugs filed, true, even some with patches, but not all of them (at least yet, I haven't had time to work through the log in bug 404077 completely yet), and I'm not sure that there's time or resources to fix them soon enough, maybe not even for 1.9.  :-\

We run xpcshell tests, but we don't run them on boxes where assertions would actually be fatal.  That's reasonably high priority (because it's needed for leak testing) and is going to be remedied soon in bug 397724, but even in the meantime I expect developers running the tests in debug builds can catch problems reasonably quickly -- I certainly catch them from time to time when I run the tests.

Also, as I mentioned, the marginal cost of writing an xpcshell test once you've written the Mochitest is pretty low for this bug.

Comment 17

[Jeff Walden [:Waldo]]

As for why you need the Mochitest (sorry, forgot to address this), it's because people using treewalkers are going to use them from browsing contexts, and minimizing differences between the actual environment and the test environment is good for minimizing surprises.

Comment 18

[alexander :surkov (:asurkov)]

Attachment: like this?2

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 294026 [details] [diff] [review]
like this?2

>Index: content/base/test/test_bug378969.html

>+  const nsIDOMNodeFilter = Components.interfaces.nsIDOMNodeFilter;
>+  var filter = nsIDOMNodeFilter.SHOW_ELEMENT | nsIDOMNodeFilter.SHOW_TEXT;

The global environment (in webpages -- doesn't apply to the xpcshell test, of course) contains a NodeFilter object which you can use here instead of doing something Mozilla-specific, i.e.:

  var filter = NodeFilter.SHOW_ELEMENT | NodeFilter.SHOW_TEXT;


>Index: content/test/unit/test_treewalker.js

>+ * The Original Code is DOM3 isEqualNode test code.

Should change this to something more appropriate before checkin...

>+ *   Alexander Surkov <surkov.alexander> (original author)

Is this email address intentional?  Doesn't matter either way unless you think it does, just making sure this wasn't a typo...


Thanks a ton for fixing this with tests!

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 294026 [details] [diff] [review]
like this?2

>+function do_test()
>+{
>+  const nsIDOMNodeFilter = Components.interfaces.nsIDOMNodeFilter;
>+
>+  var XHTMLDocString = '<html xmlns="http://www.w3.org/1999/xhtml">';
>+  XHTMLDocString += '<body><input/>input</body></html>';
>+
>+  var doc = new DOMParser().parseFromString(XHTMLDocString, "application/xml");
>+
>+  var body = doc.getElementsByTagName("body")[0];
>+  var filter = nsIDOMNodeFilter.SHOW_ELEMENT | nsIDOMNodeFilter.SHOW_TEXT;
>+  var walker = doc.createTreeWalker(body, filter, null, false);
>+  walker.currentNode = body.firstChild;
>+  walker.nextNode();
>+
>+  SimpleTest.finish();
>+}

Oh, blah, forgot: this test doesn't call any is/ok/etc. functions.  The test harness accepts this, but since it's possible for a test to silently fail before it executes any tests, you should include at least one check -- even if it's just |ok("A" == "A", "A is A")| -- so that it's clear the test didn't silently fail.  I think eventually we'll probably make tests that run to completion without actually testing anything be failures, but we haven't done so yet...

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> (From update of attachment 294026 [details] [diff] [review])
> >Index: content/base/test/test_bug378969.html
> 
> >+  const nsIDOMNodeFilter = Components.interfaces.nsIDOMNodeFilter;
> >+  var filter = nsIDOMNodeFilter.SHOW_ELEMENT | nsIDOMNodeFilter.SHOW_TEXT;
> 
> The global environment (in webpages -- doesn't apply to the xpcshell test, of
> course) contains a NodeFilter object which you can use here instead of doing
> something Mozilla-specific, i.e.:

It's strange but I got an error when I tried to use NodeFilter, therefore I started to use interface directly.

> 
> >+ *   Alexander Surkov <surkov.alexander> (original author)
> 
> Is this email address intentional?  Doesn't matter either way unless you think
> it does, just making sure this wasn't a typo...

yes, it's type of course, thank you catching!

Comment 22

[Jeff Walden [:Waldo]]

Odd, works for me when I change it -- suggest retrying?  Maybe it was a typo or something...

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> Odd, works for me when I change it -- suggest retrying?  Maybe it was a typo or
> something...
> 

probably I run it wrong. I hadn't experience to run xpcshell based tests. Can you give me lines how you do it?

Comment 24

[Jeff Walden [:Waldo]]

Oh -- you misread me.  NodeFilter will work in Mochitest but not in xpcshell, and you should only use NodeFilter in Mochitest.

Comment 25

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 26

[alexander :surkov (:asurkov)]

Peter, any plans to review this? It's really annoying thing to debug accessibility related stuffs.

Comment 27

[alexander :surkov (:asurkov)]

checked in