Sometimes the white highlights disappear

VERIFIED FIXED in Firefox 52

Status

()

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: jrmuizel, Assigned: mikedeboer)

Tracking

(Blocks 2 bugs)

unspecified
mozilla52
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox52 verified)

Details

Attachments

(4 attachments)

I'm not sure how to reproduce this, but it seems to happen sometimes when switching tabs.
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.
Blocks: 1291278
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.
Posted video STR1.mp4
Posted video STR2.mp4
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 52.1 - Oct 3
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Blocks: 1301684
Blocks: 1302033
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?
Attachment #8793381 - Flags: review?(jaws) → review+
Pushed by mdeboer@mozilla.com:
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
(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.
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'
Flags: needinfo?(mdeboer)
(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.
Pushed by mdeboer@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/ee0663840951
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Flags: needinfo?(mdeboer)
QA Contact: brindusa.tot
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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`.
Attachment #8797992 - Flags: review?(jaws) → review+
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
https://hg.mozilla.org/mozilla-central/rev/67e3ee9b41ed
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Verified fixed using the latest Nightly 52.0a1 (Build ID:20161016030205) on Ubuntu 16.04, Windows 10 and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.