Closed Bug 378969 Opened 13 years ago Closed 12 years ago

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

Categories

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

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mats, Assigned: surkov)

References

()

Details

(Keywords: assertion, qawanted)

Attachments

(3 files, 2 obsolete files)

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
Attached file stack
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.
Assignee: aaronleventhal → nobody
Component: Disability Access APIs → DOM: Traversal-Range
QA Contact: accessibility-apis → traversal-range
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.
Keywords: qawanted
Attached file Test case
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.
Flags: blocking1.9?
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.
Assignee: nobody → jonas
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...
Flags: blocking1.9? → blocking1.9-
Assignee: jonas → jwalden+bmo
(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.
I'll take a look at this in a week or so; this fell off my radar.
Attached patch like this? (obsolete) — Splinter Review
Attachment #285876 - Flags: review?(jwalden+bmo)
Hmm.. it seems like a bug that SetChildIndex is ever called with -1 as index. Why does that happen?
I plan to get to this this coming weekend; sorry for the delay so far.
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...
Attachment #285876 - Flags: review?(jwalden+bmo) → review-
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?
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?
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.
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.
Attached patch like this?2 (obsolete) — Splinter Review
Attachment #294026 - Flags: review?(jwalden+bmo)
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!
Attachment #294026 - Flags: review?(jwalden+bmo) → review+
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...
(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!

Odd, works for me when I change it -- suggest retrying?  Maybe it was a typo or something...
(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?
Oh -- you misread me.  NodeFilter will work in Mochitest but not in xpcshell, and you should only use NodeFilter in Mochitest.
Attached patch patchSplinter Review
Assignee: jwalden+bmo → surkov.alexander
Attachment #285876 - Attachment is obsolete: true
Attachment #294026 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #294331 - Flags: superreview?(peterv)
Peter, any plans to review this? It's really annoying thing to debug accessibility related stuffs.
Attachment #294331 - Flags: superreview?(peterv) → superreview+
Attachment #294331 - Flags: approval1.9?
Attachment #294331 - Flags: approval1.9? → approval1.9+
checked in
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.