Closed Bug 1285173 Opened 8 years ago Closed 8 years ago

Intermittent devtools/client/netmonitor/test/browser_net_statistics-01.js | Test timed out -

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox50 wontfix, firefox51 fixed, firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: jsnajdr)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Bug 1285173 - Netmonitor perf stats: whenDataAvailable should wait for a nonempty request list
58 bytes, text/x-review-board-request
Honza
: review+
gchang
: approval-mozilla-aurora+
gchang
: approval-mozilla-beta+
Details
Component: Developer Tools → Developer Tools: Netmonitor
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Honza, this test is failing 85 times a week, I think it needs re-prioritization and someone looking into it now.
Flags: needinfo?(odvarko)
Steve, your work on the Statistics view (e.g. bug 1308425) is closely related, could you please investigate the problem with this failing test?

Will Statistics view refactoring have big impact on how the tests for this view are written? Or do you think they will stay pretty much the same? (there are 3 tests AFAIK)

Honza
Flags: needinfo?(odvarko) → needinfo?(schung)
Priority: P3 → P1
(In reply to Jan Honza Odvarko [:Honza] from comment #15)
> Steve, your work on the Statistics view (e.g. bug 1308425) is closely
> related, could you please investigate the problem with this failing test?
> 
> Will Statistics view refactoring have big impact on how the tests for this
> view are written? Or do you think they will stay pretty much the same?
> (there are 3 tests AFAIK)
> 
> Honza

The patch for performance statistics view is mainly about the js code moving, and the xul structure is unchanged. Previous commits shows this intermittent issue happened month ago, and they are all stocked at this assertion:
  yield waitUntil(
    () => $all(".pie-chart-container:not([placeholder=true])").length == 2);
  ok(true, "Two real pie charts appear to be rendered correctly.");

Hi Jarda, since you refactored this part before(bug 1297362), do you have any idea about the possible timeout error?
Flags: needinfo?(schung) → needinfo?(jsnajdr)
(In reply to Steve Chung [:steveck] from comment #17)
This came up at our meeting today so, just for the record. I know that this intermittent isn't caused by your work on the Statistics view, it's just that your work might involve also tests refactoring (there are three for the Statistics view) and it would be great if the intermittent could be fixed as part of it.

Honza
This intermittent happens because the function whenDataAvailable at [1] is wrong.

It can be called at a time when no requests were sent yet, and the request list is empty. In that case, the "list.every()" call always returns true no matter what the predicated for "every" is.

This is exactly what happens in the failing tests. The screenshot created when the test times out always looks like the one at [2]. whenDataAvailable was testing an empty list, and the chart shows the list status a moment later - there is only one request of type "other". That's the root document, and it has type "other" instead of "html" because the response hasn't arrived yet and the MIME type is unknown.

The resulting chart is "empty", but the test is waiting for a nonempty chart to be displayed. That never happens.

[1] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/devtools/client/netmonitor/netmonitor-view.js#1200
[2] https://public-artifacts.taskcluster.net/fzuZ4OXYR_WCnqYd-BNEdA/0/public/test_info//mozilla-test-fail-screenshot_JRmWOa.png
Assignee: nobody → jsnajdr
Flags: needinfo?(jsnajdr)
Attached a patch that forces whenDataAvailable to wait at least for one request.

Note that the exact event for which whenDataAvailable should be waiting is still ill-defined. It will end waiting whenever it finds out that all the currently outstanding requests are finished. Does this mean that the page load was finished? Maybe, but most likely not always.

The whole performance stats is ripe for a complete redesign - it's very hard to understand what it's actually doing, and whatever it is, it probably does it wrong. See bug 1015201 for example. I agree with the sentiment expressed by Sole there.
See Also: → 1015201
Comment on attachment 8809806 [details]
Bug 1285173 - Netmonitor perf stats: whenDataAvailable should wait for a nonempty request list

https://reviewboard.mozilla.org/r/92332/#review92678

R+, thanks Jarda

Honza
Attachment #8809806 - Flags: review?(odvarko) → review+
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/afd22bdea63c
Netmonitor perf stats: whenDataAvailable should wait for a nonempty request list r=Honza
Backed out on request of developer for timeouts in browser_net_statistics-03.js on Linux x64 opt+pgo:

https://hg.mozilla.org/integration/autoland/rev/222dd5537727a008fbb3fc299d6e3ef856761f46

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

[task 2016-11-14T13:14:46.834693Z] 13:14:46     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_statistics-03.js | The requests-menu-filter-media-button button should not have a 'checked' class. - 
[task 2016-11-14T13:14:46.835819Z] 13:14:46     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_statistics-03.js | The requests-menu-filter-flash-button button should not have a 'checked' class. - 
[task 2016-11-14T13:14:46.842607Z] 13:14:46     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_statistics-03.js | The requests-menu-filter-ws-button button should have a 'checked' class. - 
[task 2016-11-14T13:14:46.842690Z] 13:14:46     INFO - The correct filtering predicates are used before entering perf. analysis mode.
[task 2016-11-14T13:14:46.842751Z] 13:14:46     INFO - Buffered messages finished
[task 2016-11-14T13:14:46.842852Z] 13:14:46     INFO - TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_statistics-03.js | Test timed out -
Flags: needinfo?(jsnajdr)
Submitting a new and improved version of the patch.

Solved another problem with whenDataAvailable: it retrieved the request list from the view object only once (RequestsMenu.attachments returns a copy of the requests array), and then waited for its properties to be filled in. It failed in case when the array returned on first call was empty.

Now, the RequestsMenu.attachments is called again each time a check is performed in the setInterval loop.
Flags: needinfo?(jsnajdr)
Comment on attachment 8809806 [details]
Bug 1285173 - Netmonitor perf stats: whenDataAvailable should wait for a nonempty request list

https://reviewboard.mozilla.org/r/92332/#review93072

Looks good, thanks Jarda for working on this!

Honza
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b41ff8fe81c2
Netmonitor perf stats: whenDataAvailable should wait for a nonempty request list r=Honza
https://hg.mozilla.org/mozilla-central/rev/b41ff8fe81c2
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Please request Aurora/Beta approval on this when you get a chance.
status-firefox50: --- → wontfix
status-firefox51: --- → affected
status-firefox52: --- → affected
Flags: needinfo?(jsnajdr)
Comment on attachment 8809806 [details]
Bug 1285173 - Netmonitor perf stats: whenDataAvailable should wait for a nonempty request list

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: n/a
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: No risk for the user, the fix is related to automated test.
[String/UUID change made/needed]: none
Attachment #8809806 - Flags: approval-mozilla-aurora?
Comment on attachment 8809806 [details]
Bug 1285173 - Netmonitor perf stats: whenDataAvailable should wait for a nonempty request list

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: n/a
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: No risk for the user, the fix is related to automated test.
[String/UUID change made/needed]: none
Attachment #8809806 - Flags: approval-mozilla-beta?
Flags: needinfo?(jsnajdr)
Comment on attachment 8809806 [details]
Bug 1285173 - Netmonitor perf stats: whenDataAvailable should wait for a nonempty request list

Fix an intermittent test failure. Beta51 and Aurora 52+. Should be in 51 beta 2.
Attachment #8809806 - Flags: approval-mozilla-beta?
Attachment #8809806 - Flags: approval-mozilla-beta+
Attachment #8809806 - Flags: approval-mozilla-aurora?
Attachment #8809806 - Flags: approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: