132612 - 'Find in This Page' does not find all matches

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Martin Horn]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.9) Gecko/20020311
BuildID:    2002031104

In the mentioned page (mozilla bug list with about 500 bugs)
'Find in This Page' does not work.
Searching for owners starting with 'c' (e.g. 'curt') finds only some
matches and misses others.

Reproducible: Always
Steps to Reproduce:
1. Go to the mentioned URL or a different Mozilla-Bugzilla page with
   many bugs (> 300)
2. Sort by owner
3. 'Find on This Page' (Ctrl+F) and enter an owner name starting
   with 'c' (e.g. 'curt'), search forward.
4. Repeat search: press 'Find' again in the search dialog or 
   leave search dialog and press Ctrl+G

Actual Results:  When searching forward, only one match is found.
When searching backward, all matches (nine) are found.

Expected Results:  All matches should be found.

- Search fails only on large pages
- Search fails only if the text starts with 'c'
- Search fails only if search direction is forward

Comment 1

[Boris Zbarsky [:bzbarsky]]

to akkana

Comment 2

[Akkana Peck]

I bet this was caused by the same problems that caused 129971.  In any case, I
don't see it in today's build.  Try it and see if you agree.

*** This bug has been marked as a duplicate of 129971 ***

Comment 3

[Paul Wyskoczka]

v

Comment 4

[Martin Horn]

I still see this bug. I used the sources from 2002-03-27.
Did you search for 'curt'?

I will try the latest nightly build.

Comment 5

[Martin Horn]

Now I tried build 2002032803 and still see the bug.

Comment 6

[Akkana Peck]

Attachment: static copy since bugzilla queries are slow

This is working for me, in today's build.  I can go through the "curt"
instances with either return-return in the find dialog, or with accel-G (find
again).

I wasn't able to test this for the last few days because bugzilla slowness has
made the query time out.  I finally got a successful query, so I saved it,
edited the initial "Please wait" stuff out and am attaching it here for
testing.

If you are still seeing this, please save your query to a file, test on the
file and see if you're still seeing it.  Maybe we're getting different queries
somehow, based on different bugzilla cookies.  Do you see the behavior when
searching for curt in this attachment?

Comment 7

[Martin Horn]

You are right, I don't see the bug with your query.
My query (which shows the bug) looks different. I get other columns.
I will save as file an create an attachment.

Comment 8

[Martin Horn]

Attachment: copy of a query that shows the bug

Comment 9

[Martin Horn]

Attachment: copy of a query that shows the bug

Comment 10

[Paul Wyskoczka]

qa --> [e-mail]

Comment 11

[Akkana Peck]

Yes, now I see it in your query.

Comment 12

[[not reading bugmail]]

my first thought is it looks like its a problem with
backwards(checked/unchecked) state with the first search and forward searching
doesn't get set correctly initially upon loading find in this page dialog.. 
maybe its a problem with the state widget file like the scripts & windows bug
that was just fixed.. 

Comment 13

[Akkana Peck]

No, actually the problem is that it's getting confused by the two 'c' characters
in a row, spanning from one text node to the next one.  It doesn't happen in all
cases, just some of them.  In the reverse direction the comparison is completely
different (it goes back to front) so you don't see the same behavior.

Comment 14

[[e-mail]]

I was idly wondering... are the forward and reverse cases so different that the
bug simply cannot occur in reverse?

Comment 15

[Akkana Peck]

It turns out that the simple patch in bug 134275 also fixes this bug.  But
that's accidental and doesn't fix the real cause, so I'm still working on a fix
to the algorithm.  I still don't know what causes this -- it's very sensitive to
position, has to have exactly the right sort of table entries plus an entity
immediately after the end of the found string.  When I figure out why it's
happening, I may be able to answer the question of whether it could be made to
happen in the reverse direction too.

Comment 16

[Akkana Peck]

Attachment: A fix

Got it.  It was a problem with the way we went back to the match anchor node
when ending a partial match.  The important part of this patch is that it sets
mIterOffset back to the match anchor offset, and sets frag=0, so that when we
loop we'll know we're changing nodes and need to start findex at mIterOffset. 
A less important change is that restart wasn't being used, but missed being
deleted from the code in the last go-round, so I'm removing it now.

Comment 17

[[e-mail]]

Comment on attachment 81104 [details] [diff] [review]
A fix

diff -u is usual :-)

Comment 18

[Akkana Peck]

Attachment: diff -u

Argh!  I should think about doing alias diff diff -u -- I keep thinking I typed
it and find out I didn't.  Sorry.

Comment 19

[Akkana Peck]

Kathy, can you review this?  Kin, can you sr?

Comment 20

[Kathleen :Brade]

Comment on attachment 81113 [details] [diff] [review]
diff -u

r=brade

Comment 21

[kinmoz]

Comment on attachment 81113 [details] [diff] [review]
diff -u

Do we want to outdent this comment?


+      mIterOffset = matchAnchorOffset;
	   // +incr will be added to findex when we continue
-      nsCOMPtr<nsIContent> content (do_QueryInterface(matchAnchorNode));


Should |content| ever be null in the code below, what are the chances of us
going into an infinite loop since the outer loop is a |while(1)|? Should we be
bailing in such a case?


+	 nsCOMPtr<nsIContent> content (do_QueryInterface(matchAnchorNode));
+	 nsresult rv = NS_ERROR_UNEXPECTED;
+	 if (content)
+	   rv = mIterator->PositionAt(content);
+	 frag = 0;

Comment 22

[Akkana Peck]

I don't think we can get an infinite loop from that, since we reset
matchAnchorNode to nsnull right after that clause, before we loop around.  So
we'll go through the loop looking to start a new match, and we can't go back to
the problematic node (the one that didn't QI to nsIContent) because we've
already forgotten about that node and moved on.

I can outdent the comment if you prefer it that way.  (I actually indented it on
purpose, though that made it look more like it applied to the previous line than
to the next line, but I don't feel strongly about it.)

Comment 23

[kinmoz]

Comment on attachment 81113 [details] [diff] [review]
diff -u

sr=[e-mail]

I just mentioned the comment thing, cause I didn't see any of the other
comments indented that way. It's up to you. :-)

Comment 24

[Akkana Peck]

Fixed (on the trunk).

Comment 25

[sairuh (rarely reading bugmail)]

vrfy'd fixed on all platforms using 2003.01.08 trunk bits.