148713 - infinite loop in search/replace

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Joe Francis]

from #composer channel on mozilla irc:

5:45 PM: *** brantgurga
([e-mail]) has joined
channel #composer
55:46 PM: brantgurga: Search and Replace has a bug
5:46 PM: brantgurga: Try replacing "test" with "this test" and click replace all.
5:46 PM: brantgurga: Recursive searching causes infinite loop.

Comment 1

[Joe Francis]

Did I pick the correct component?  cc'ing Akkana and Kin.

Comment 2

[Akkana Peck]

I'm not sure what the right component is, but this would probably be my bug. 
But I don't see it.  What page were you editing?  I tried just copying the text
of this bug and pasting it into a composer page (so the word "test" appeared a
couple of times), setting the caret near the beginning of the file, then
replacing "test" with "this test" and clicking replace all.  It did the replace,
no infinite loop.

On what version of mozilla did you see this?  I tried on a branch release build
and on a current trunk build.

Comment 3

[Nicholas Allen]

Managed to reproduce this with today's branch.  Steps:

1) Edit this bug in Composer
2) Hit Ctrl-F
3) Text to find is "test" and text to replace with is "test this"
4) Select wrap around and search backwards
5) Replace all

Hangs with 100% cpu and eventually crashes.

Comment 4

[Akkana Peck]

Yup!  I see it too.  Taking the bug.

Comment 5

[Akkana Peck]

Fix coming, cc some possible reviewers, add ADT2 hoping there might be a chance
to get this into MachV since it's a hang and it's not that unusual a case.

Comment 6

[Akkana Peck]

Attachment: A fix.

The code made the assumption that the inserted text would end up selected.  In
fact, the editor puts the selection after the inserted text, so we were finding
it on the next iteration.  (I thought I checked for that case, but I guess I
only checked for it in the forward direction).

The only solution I found was to save the selection by collapsing the found
range before inserting, in the reverse direction.  That doesn't work in the
forward direction, though, apparently because of the direction that range
gravity collapses the range after the selected contents disappear, so in the
forward direction I kept the existing code.

Seeking review -- Joe?	Charley?  Kathy?

Comment 7

[Akkana Peck]

Attachment: Include strict warning fixes (bug 145071)

This file generated some JS strict warnings -- let's fix them as long as we're
in here, since it's just a couple lines more.  I've included fixes for all the
warnings I could find.

Comment 8

[Kathleen :Brade]

Comment on attachment 86744 [details] [diff] [review]
Include strict warning fixes (bug 145071)

r=brade

Comment 9

[kinmoz]

Comment on attachment 86744 [details] [diff] [review]
Include strict warning fixes (bug 145071)

sr=[e-mail]

Comment 10

[Akkana Peck]

Fixed on trunk.  Adding adt1.0.1 keyword to request branch checkin, since this
fixes a hang.

Comment 11

[Akkana Peck]

In response to an ADT question: this patch does not involve any string changes.

Comment 12

[Claudius Gayle]

VERIFIED Fixed on the trunk with 20020627 builds.

Turns out this was the wrong component. reassigning, if there need be any
follow-up the right QA can help you out.but in the meantime -to save time- this
bug _is_ 'verified' on trunk builds.

Comment 13

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending
drivers' approval. pls check this in asap, then add the "fixed1.0.1" keyword.

Comment 14

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 15

[Akkana Peck]

Fixed on the branch too.

Comment 16

[lchiang]

verified on 1.0.1branch win2k build 8-23.