1401072 - Intermittent browser/base/content/test/performance/browser_urlbar_search_reflows.js | Unused expected reflow: ["adjustHeight@chrome://global/content/bindings/autocomplete.xml","onxblpopupshown@chrome://global/content/bindings/autocomplete.xml"]

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P5)

Description

[Treeherder Bug Filer]

Filed by: rvandermeulen [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=131870020&repo=mozilla-beta

https://queue.taskcluster.net/v1/task/diI1dDpxTw-J99sP26P7gg/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[Intermittent Failures Robot]

1 failures in 943 pushes (0.001 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-beta: 1

Platform breakdown:
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1401072&startday=2017-09-18&endday=2017-09-24&tree=all

Comment 2

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

https://wiki.mozilla.org/Bugmasters#Intermittent_Test_Failure_Cleanup

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I suspect I'm about to re-open this by landing bug 1434376. Thankfully, I have a patch that seems to fix it.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1401072 - Update browser_urlbar_search_reflows to accomodate DOM dirtying done in setTimeout.

This patch doesn't introduce new reflows, but adjusts some expected maxCounts
in the test for reflows that were always there but not reliably detected.

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

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8953796 [details]
Bug 1401072 - Update browser_urlbar_search_reflows to accomodate DOM dirtying done in setTimeout.

https://reviewboard.mozilla.org/r/222988/#review228864

I'm ok with the current patch if it stops an intermittent we are seeing, but see the following comments for possible future improvements.

::: browser/base/content/test/performance/browser_urlbar_search_reflows.js:166
(Diff revision 1)
>      popup.onResultsAdded = () => {
>        dirtyFrameFn();
>        oldResultsAdded();
>      };
>  
> +    win.setTimeout = (fn, ms) => {

Is this something that should go in the head file?

If I understand correctly, you are saying that at setTimeout callback does reflows here, should we automatically ensure the test waits at least as long as the longest setTimeout that has been requested since we started the test? (ie. could this reduce intermittents?)

I see the test already uses requestIdleCallback to workaround these timeouts. Is this more reliably than using our own timeout (with a slightly longer time than the longest timeout detected)? Should we move that instead to our reflow observer code?

::: browser/base/content/test/performance/browser_urlbar_search_reflows.js:169
(Diff revision 1)
>      };
>  
> +    win.setTimeout = (fn, ms) => {
> +      oldSetTimeout(() => {
> +        dirtyFrameFn();
> +        fn();

I hope:
- nothing is using the setTimeout form where 'fn' is a string rather than a function. (Hopefully we have eslint coverage to ban that deprecated form)
- nothing expects clearTimeout to work.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8953796 [details]
Bug 1401072 - Update browser_urlbar_search_reflows to accomodate DOM dirtying done in setTimeout.

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

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8953796 [details]
Bug 1401072 - Update browser_urlbar_search_reflows to accomodate DOM dirtying done in setTimeout.

https://reviewboard.mozilla.org/r/222988/#review228864

> Is this something that should go in the head file?
> 
> If I understand correctly, you are saying that at setTimeout callback does reflows here, should we automatically ensure the test waits at least as long as the longest setTimeout that has been requested since we started the test? (ie. could this reduce intermittents?)
> 
> I see the test already uses requestIdleCallback to workaround these timeouts. Is this more reliably than using our own timeout (with a slightly longer time than the longest timeout detected)? Should we move that instead to our reflow observer code?

Yeah, I think opening a bug to maybe try moving setTimeout wrapping into the head.js is a good idea. I'll file a follow-up after this lands.

> I hope:
> - nothing is using the setTimeout form where 'fn' is a string rather than a function. (Hopefully we have eslint coverage to ban that deprecated form)
> - nothing expects clearTimeout to work.

I can't see instances of the string form of `setTimeout` in the URLbar code, so that's good. I _do_, however, see usage of clearTimeout. I can get that to work if I properly return the old setTimeout function value though, so I'll do that. Thanks!

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8953796 [details]
Bug 1401072 - Update browser_urlbar_search_reflows to accomodate DOM dirtying done in setTimeout.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17857acb21a5
Update browser_urlbar_search_reflows to accomodate DOM dirtying done in setTimeout. r=florian

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/17857acb21a5

Comment 11

[Intermittent Failures Robot]

8 failures in 675 pushes (0.012 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* autoland: 8

Platform breakdown:
* linux64: 6
* osx-10-10: 1
* linux64-stylo-disabled: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1401072&startday=2018-02-19&endday=2018-02-25&tree=all