User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070321 Minefield/3.0a3pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070321 Minefield/3.0a3pre When doing a find on a page containing form fields, if the text the user is searching for is found inside one of the fields, when Enter or Next is pressed again, the search immediately begins from the top of the page, even though there are more instances later in the page. Reproducible: Always Steps to Reproduce: 1. Load the URL shown above 2. Search for the text "Firefox", which occurs multiple times in the article Actual Results: The first two instances of "Firefox" are found--one in the header "Editing Mozilla Firefox" and the first instance in the first line of the input box, "{{redirect|Firefox}}". Even though there are many more instances of the word "Firefox" later in that same input box and later in the page outside the input box, they are not found, and searching begins again from the top of the page, with the page header. Expected Results: All remaining instances in the page should have been found before starting again from the top.

This regressed between 2007-02-28 and 2007-03-02: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-02-28+04&maxdate=2007-03-02+09&cvsroot=%2Fcvsroot There are a lot of check-ins in that area, but it seems to me that this is a regression from bug 372086, somehow.

Seems possible... I have no idea how search goes about figuring out where to go next. I'll try to look, but I have several dozen crash and security bugs that are higher-priority, so if someone who knows something about search (or is willing to learn) looks first, so much the better.

I have checked with my debug build that the patch for bug 372086 indeed has caused this. Maybe this gives an indication of what's going on? http://lxr.mozilla.org/mozilla/source/embedding/components/find/src/nsFind.cpp#95 Especially this line: " This means that we can be given an initial search // range that stretches across the anonymous DOM and the normal DOM. "

Yeah, that's exactly the problem. This code is using ranges to keep track of part of the flattened tree (including anonymous content), and when we changed things so you can't do that anymore, it didn't deal very well... Patch coming up that seems to fix things for me.

And this is a core Find issue.

There are a few things going on here. First of all, when we initialize mIterator we may want it to iterate between endpoints one of which is anonymous and one of which is not. That's not expressible with a range, so just keep track of the endpoint nodes and offsets directly. This is the bulk of the patch -- it involves changing InitIterator, adding another Init() method to the find iterator, changing the members, and replacing all uses of mRange with the new members The other issue I ran into was that this code actually depended on a bug in the text node implementation of IsNativeAnonymous().... a bug that we fixed sometime in the 1.9 cycle. Before the fix, a text child of a native anonymous element claimed to be native anonymous; now it (correctly) only claims it if it's native anonymous with respect to its parent. Which means that to check whether a node is really "native anonymous" with respect to the toplevel document you really need to walk the bindingParent chain. That's the second change in this patch. I went for a minimally-invasive change, because I wasn't sure how this code wanted to handle XBL... With those two changes, the testcase and site both work for me.

The following bit of the patch doesn't look correct. What you want to emulate is range = mRange; // to clone/copy everything range.some_fields = new_values // while other cloned values are preserved. As it stands with the patch, some fields are left uninitialized. (to check this code-path, IIRC, try ffwd and fbwd in a document with: a textarea as first element, some text/other things, and a textarea as last element). // make sure to place the outer-iterator outside // the text control so that we don't go there again. nsresult res; - mRange->CloneRange(getter_AddRefs(range)); + // XXXbz what about out-of-memory here? + range = do_CreateInstance(kRangeCID); if (!mFindBackward) { // find forward // cut the outer-iterator after the current node - res = range->SetStartAfter(outerNode); + res = range->SetEnd(mEndNode, mEndOffset); + res |= range->SetStartAfter(outerNode); } else { // find backward // cut the outer-iterator before the current node - res = range->SetEndBefore(outerNode); + res = range->SetStart(mStartNode, mStartOffset); + res |= range->SetEndBefore(outerNode); }

While re-reading the code for the review, I saw that you can take a nice bite at the OOM issue by swapping things around, specifically: 377 nsCOMPtr<nsIDOMElement> rootElement; 378 editor->GetRootElement(getter_AddRefs(rootElement)); 379 nsCOMPtr<nsIContent> rootContent(do_QueryInterface(rootElement)); 380 381 // now create the inner-iterator 382 mInnerIterator = do_CreateInstance(kCPreContentIteratorCID); 383 384 if (mInnerIterator) { 385 nsCOMPtr<nsIDOMNode> node(do_QueryInterface(rootContent)); 386 nsCOMPtr<nsIDOMRange> range(do_CreateInstance(kRangeCID)); 387 range->SelectNodeContents(node); TO: 377 nsCOMPtr<nsIDOMElement> rootElement; 378 editor->GetRootElement(getter_AddRefs(rootElement)); 379 nsCOMPtr<nsIContent> rootContent(do_QueryInterface(rootElement)); nsCOMPtr<nsIDOMNode> node(do_QueryInterface(rootContent)); nsCOMPtr<nsIDOMRange> range(do_CreateInstance(kRangeCID)); if (!range) return; range->SelectNodeContents(node); And continue with the create mInnerIterator as is, and re-use range to init mOuterIterator without further worries about the OOM issue. [You could swap far back, BTW, but it boils down to creating the iterator only if you have a range, rather than the other way around as it is presently done.]

Comment on attachment 261644 [details] [diff] [review] Proposed fix r+sr=rbs Eh, the patch does what I mentioned... What was I reading before?! (Nevermind, it was 3am here.) Only take care of the OOM and it is just fine.

We can't re-use |range| for both iterators -- iterators don't copy the range they're inited with. So we need two different range objects. That said, I'll just go ahead and use two different variables for that.

Checked in.