1067287 - Intermittent browser_timeline_overview-initial-selection-02.js | There are no markers available. - Got 3, expected 0

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Carsten Book [:Tomcat]]

Ubuntu VM 12.04 mozilla-inbound debug test mochitest-devtools-chrome-3 on 2014-09-15 01:44:35 PDT for push 7e47a9f5e048

slave: tst-linux32-spot-545

https://tbpl.mozilla.org/php/getParsedLog.php?id=48081485&tree=Mozilla-Inbound

02:04:13     INFO -  352 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/timeline/test/browser_timeline_overview-initial-selection-02.js | There are no markers available. - Got 3, expected 0

Comment 1

[Sami Jaktholm]

Attachment: timeline-intermittent.patch

Looking at the code I think this is caused by following (highly unlikely) chain of events:
1) actor sends new markers to the front and yields execution
2) test calls TimelineController._stopRecordingAndDiscardData
3) TimelineController._markers is cleared
4) TimelineController._stopRecording yields on gFront.stop
5) waiting package from (1) is processed
6) TimelineController._onMarkers appends the markers to the cleared list
7) gFront.stop resolves
8) test asserts markers.length == 0 which fails due to step 6 inserting new items to the list

If the failure is indeed caused by the above chain of events, here's a patch that ensures the list is still empty when TimelineController._stopRecordingAndDiscardData returns by clearing it again.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=4da7f5787bef

Comment 2

[Legacy TBPL/Treeherder Robot]

Tomcat
https://tbpl.mozilla.org/php/getParsedLog.php?id=48579252&tree=Mozilla-Inbound
Ubuntu VM 12.04 mozilla-inbound debug test mochitest-devtools-chrome-3 on 2014-09-22 04:44:41
revision: d47f0d824c2d
slave: tst-linux32-spot-142

352 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/timeline/test/browser_timeline_overview-initial-selection-02.js | There are no markers available. - Got 3, expected 0

Comment 3

[Legacy TBPL/Treeherder Robot]

Tomcat
https://tbpl.mozilla.org/php/getParsedLog.php?id=48766043&tree=Mozilla-Inbound
Ubuntu VM 12.04 mozilla-inbound debug test mochitest-devtools-chrome-3 on 2014-09-24 03:48:05
revision: 97d05c73c5a6
slave: tst-linux32-spot-180

351 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/timeline/test/browser_timeline_overview-initial-selection-02.js | There are no markers available. - Got 3, expected 0

Comment 4

[Paul Rouget [:paul]]

Comment on attachment 8492727 [details] [diff] [review]
timeline-intermittent.patch

Don't we want to move the length=0 to the bottom instead of calling it again?

Comment 5

[Sami Jaktholm]

That would also work but then _stopRecordingAndDiscardData would have to explicitly clear the UI from previously rendered markers instead of relying to _stopRecording to do it when an empty list of markers is given. However, _stopRecordingAndDiscardData is only ever called from the failing test so the UI inconsistency should not show up to users.

Comment 6

[Victor Porof [:vporof][:vp]]

Comment on attachment 8492727 [details] [diff] [review]
timeline-intermittent.patch

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

::: browser/devtools/timeline/timeline.js
@@ +131,5 @@
>     * Used in tests. Stops the recording, discarding the accumulated markers and
>     * updating the UI as needed.
>     */
>    _stopRecordingAndDiscardData: function*() {
>      this._markers.length = 0;

Do we still need this then, before the yield?

Comment 7

[Legacy TBPL/Treeherder Robot]

edmorley
https://tbpl.mozilla.org/php/getParsedLog.php?id=48872776&tree=Mozilla-Inbound
Ubuntu VM 12.04 mozilla-inbound debug test mochitest-devtools-chrome-3 on 2014-09-25 07:34:41
revision: 9a5ca02006f7
slave: tst-linux32-spot-717

351 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/timeline/test/browser_timeline_overview-initial-selection-02.js | There are no markers available. - Got 3, expected 0

Comment 8

[Sami Jaktholm]

Attachment: timeline-intermittent.patch

(In reply to Victor Porof [:vporof][:vp] from comment #6)
> Comment on attachment 8492727 [details] [diff] [review]
> timeline-intermittent.patch
> 
> Review of attachment 8492727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/timeline/timeline.js
> @@ +131,5 @@
> >     * Used in tests. Stops the recording, discarding the accumulated markers and
> >     * updating the UI as needed.
> >     */
> >    _stopRecordingAndDiscardData: function*() {
> >      this._markers.length = 0;
> 
> Do we still need this then, before the yield?

Currently nothing depends on it so it might as well be removed. Tests pass with or without it and results of the clearing doesn't show up to users. Here's a version with the duplicate operation removed that just moves the this._markers.length to after the yield.

Try: https://tbpl.mozilla.org/?tree=Try&rev=14cc30f69ab2

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/913b6dbbfcd6

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/913b6dbbfcd6

Comment 11

[David Lawrence [:dkl]]

Sorry for the spam. Moving bugs to Firefox :: Developer Tools: Performance Tools (Profiler/Timeline).

dkl