1303874 - Sometimes the white highlights disappear

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

I'm not sure how to reproduce this, but it seems to happen sometimes when switching tabs.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

STR might be something like this:
- Search for X
- Switch to another tab and search for Y there
- Switch to original tab and use the search for Y there. Other Y's on the original page are not highlighted.

Comment 2

[Simona Badau, Desktop QA]

I've also reproduced this issue on the latest Nightly 52.0a1 (Build ID: 20160920030429). Here are the scenarios that I used:

STR1:
1. Launch Firefox
2. Navigate to: https://developer.mozilla.org/en-US/
3. Press CTRL+F to open Find toolbar
4. Search for "to"
5. Click twice on the "Highlight All" button  
6. Click once on the page content to lose the dimmed background
7. In the Find Toolbar, click on the input box and click Enter

Actual results: 
After step 7 - there is only one match for "to". Please see STR1 screen-cast for more details.

STR2:
1. Launch Firefox
2. Navigate to: https://www.reddit.com/
3. Press CTRL+F to open Find toolbar
4. Scroll down the page 
5. In the Find Toolbar, click on the input box and click Enter

Actual results: 
In step 5 - only one match for "ago" is highlighted. Please see STR2 screen-cast for more details.

Comment 3

[Simona Badau, Desktop QA]

Attachment: STR1.mp4

Comment 4

[Simona Badau, Desktop QA]

Attachment: STR2.mp4

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1303874 - clear the dictionary for the current find window only upon restart of the iterator, not each reset.

Review commit: https://reviewboard.mozilla.org/r/80130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80130/

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8793381 [details]
Bug 1303874 - clear the dictionary for the current find window only upon restart of the iterator, not each reset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80130/diff/1-2/

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8793381 [details]
Bug 1303874 - clear the dictionary for the current find window only upon restart of the iterator, not each reset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80130/diff/2-3/

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8793381 [details]
Bug 1303874 - clear the dictionary for the current find window only upon restart of the iterator, not each reset.

https://reviewboard.mozilla.org/r/80130/#review79392

It doesn't seem like our current automated tests cover this case, and the changes in this patch don't increase test coverage. How can we be sure this won't break in the future?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/464d8056f9de
clear the dictionary for the current find window only upon restart of the iterator, not each reset. r=jaws

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> It doesn't seem like our current automated tests cover this case, and the
> changes in this patch don't increase test coverage. How can we be sure this
> won't break in the future?

Well, in browser_FinderHighlighter.js you saw the update of the expectedResult.rects number from 2 to 1, right? That's the bug! :/

Very, well, subtle.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing mochitest-chrome-3 test_bug263683.xul on Linux:

https://hg.mozilla.org/integration/autoland/rev/a33dcaa0f804

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=464d8056f9de69d7133cc507123a44b8d3d0f6df
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=3946316&repo=autoland

[task 2016-09-23T14:46:52.725344Z] 14:46:52     INFO -  776 INFO TEST-START | toolkit/content/tests/chrome/test_bug263683.xul
[task 2016-09-23T14:46:55.158695Z] 14:46:55     INFO -  TEST-INFO | started process screentopng
[task 2016-09-23T14:46:55.815557Z] 14:46:55     INFO -  TEST-INFO | screentopng: exit 0
[task 2016-09-23T14:46:55.815826Z] 14:46:55     INFO -  777 INFO Starting test with browser 'content'
[task 2016-09-23T14:46:55.816047Z] 14:46:55     INFO -  778 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly detects new selection type - true == true
[task 2016-09-23T14:46:55.816545Z] 14:46:55     INFO -  779 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly added a match to the selection type - 1 == 1
[task 2016-09-23T14:46:55.816923Z] 14:46:55     INFO -  780 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Added the correct match - "mozilla" == "mozilla"
[task 2016-09-23T14:46:55.817018Z] 14:46:55     INFO -  781 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly removed the range - 0 == 0
[task 2016-09-23T14:46:55.817816Z] 14:46:55     INFO -  782 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly added a match from input to the selection type - 1 == 1
[task 2016-09-23T14:46:55.818446Z] 14:46:55     INFO -  783 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Added the correct match - "mozilla" == "mozilla"
[task 2016-09-23T14:46:55.818583Z] 14:46:55     INFO -  784 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly removed the range - 0 == 0
[task 2016-09-23T14:46:55.819298Z] 14:46:55     INFO -  785 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly added a match to the selection type - 1 == 1
[task 2016-09-23T14:46:55.819390Z] 14:46:55     INFO -  786 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Added the correct match - "mozilla" == "mozilla"
[task 2016-09-23T14:46:55.820096Z] 14:46:55     INFO -  787 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly added a match to the selection type - 1 == 1
[task 2016-09-23T14:46:55.820716Z] 14:46:55     INFO -  788 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Added the correct match - "mozilla" == "mozilla"
[task 2016-09-23T14:46:55.820845Z] 14:46:55     INFO -  789 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Found correct amount of matches
[task 2016-09-23T14:46:55.821448Z] 14:46:55     INFO -  790 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly removed the range - 0 == 0
[task 2016-09-23T14:46:55.822525Z] 14:46:55     INFO -  791 INFO TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_bug263683.xul | Correctly removed the range - 1 == 0
[task 2016-09-23T14:46:55.823548Z] 14:46:55     INFO -  receiveMessage@resource://testing-common/ContentTask.jsm:117:7
[task 2016-09-23T14:46:55.824426Z] 14:46:55     INFO -  792 INFO Starting test with browser 'content-remote'

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> > It doesn't seem like our current automated tests cover this case, and the
> > changes in this patch don't increase test coverage. How can we be sure this
> > won't break in the future?
> 
> Well, in browser_FinderHighlighter.js you saw the update of the
> expectedResult.rects number from 2 to 1, right? That's the bug! :/
> 
> Very, well, subtle.

A bit too subtle I think actually. These numbers change with many of the patches that are landing, so I think it might be easy to accidentally overlook introducing a regression.

Comment 13

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43742dde8a6c

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8793381 [details]
Bug 1303874 - clear the dictionary for the current find window only upon restart of the iterator, not each reset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80130/diff/3-4/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee0663840951
clear the dictionary for the current find window only upon restart of the iterator, not each reset. r=jaws

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee0663840951

Comment 17

[roxleitan]

The issue is still reproducible on the latest Nightly 52.0a1, build ID: 20160929030426 , on Mac OS X 10.11 using the steps from the Description (Comment 1).

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: make the active window object part of the iterator params

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8797992 [details] [diff] [review]
Patch 2: make the active window object part of the iterator params

Review of attachment 8797992 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/FinderIterator.jsm
@@ +117,5 @@
>  
>      let window = finder._getWindow();
>      let resolver;
>      let promise = new Promise(resolve => resolver = resolve);
> +    let iterParams = { caseSensitive, entireWord, linksOnly, useCache, window, word };

The docs above this function need updating to include `window`.

Comment 20

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67e3ee9b41edc9d70863704fad44a50d283bb886
Bug 1303874 - make the active window object part of the iterator params to make sure that similar iterator runs for different runs are not treated as the same, thus potentially yielding incorrect results. r=jaws

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67e3ee9b41ed

Comment 22

[roxleitan]

Verified fixed using the latest Nightly 52.0a1 (Build ID:20161016030205) on Ubuntu 16.04, Windows 10 and Mac OS X 10.11.