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

RESOLVED FIXED in Firefox 35

Status

DevTools
Performance Tools (Profiler/Timeline)
RESOLVED FIXED
4 years ago
9 days ago

People

(Reporter: Tomcat, Assigned: Sami Jaktholm)

Tracking

({intermittent-failure})

unspecified
Firefox 35
x86
Linux
intermittent-failure

Firefox Tracking Flags

(firefox33 unaffected, firefox34 unaffected, firefox35 fixed, firefox-esr31 unaffected)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Created attachment 8492727 [details] [diff] [review]
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
Assignee: nobody → sjakthol
Status: NEW → ASSIGNED
Attachment #8492727 - Flags: review?(paul)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)

Comment 4

4 years ago
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?
Attachment #8492727 - Flags: review?(paul) → review?(vporof)
(Assignee)

Comment 5

4 years ago
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 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?
Attachment #8492727 - Flags: review?(vporof) → review+
Comment hidden (Treeherder Robot)
(Assignee)

Comment 8

4 years ago
Created attachment 8495680 [details] [diff] [review]
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
Attachment #8492727 - Attachment is obsolete: true
Attachment #8495680 - Flags: review?(vporof)
Attachment #8495680 - Flags: review?(vporof) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Reporter)

Comment 10

4 years ago
https://hg.mozilla.org/mozilla-central/rev/913b6dbbfcd6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
status-firefox35: --- → fixed
status-firefox33: --- → unaffected
status-firefox34: --- → unaffected
status-firefox-esr31: --- → unaffected
Sorry for the spam. Moving bugs to Firefox :: Developer Tools: Performance Tools (Profiler/Timeline).

dkl
Component: Developer Tools: Timeline → Developer Tools: Performance Tools (Profiler/Timeline)

Updated

9 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.