Closed Bug 1066474 Opened 6 years ago Closed 5 years ago

Intermittent browser_timeline.js,browser_timelineMarkers-02.js | markers includes Paint

Categories

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

x86
macOS
defect
Not set

Tracking

(firefox35 fixed, firefox36 fixed, firefox37 fixed, firefox-esr31 unaffected)

RESOLVED FIXED
Firefox 37
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: cbook, Assigned: tromey)

References

()

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

Rev4 MacOSX Snow Leopard 10.6 mozilla-inbound debug test mochitest-browser-chrome-2 on 2014-09-11 22:35:00 PDT for push b252971fb544

slave: t-snow-r4-0082

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



123 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/browser/browser_timelineMarkers-02.js | markers includes Paint -
Summary: Intermittent browser_timelineMarkers-02.js | markers includes Paint - → Intermittent browser_timeline.js,browser_timelineMarkers-02.js | markers includes Paint
This is current #6 on OrangeFactor. Can we please get some attention on this bug? Looks like it's basically been failing since it landed.
Component: DOM → Developer Tools: Timeline
Flags: needinfo?(pbrosset)
Flags: needinfo?(paul)
Product: Core → Firefox
There are 2 tests failing here:
1) /toolkit/devtools/server/tests/browser/browser_timeline.js
2) /docshell/test/browser/browser_timelineMarkers-02.js

1 tests that the devtools actor actually works as expected, but it also verifies that paint/reflow/styles operations are part of the recorded markers. I don't think it should. This test should only really be interested in testing the actor's API, not the content of the markers, which test 2 is supposed to do.
So, removing the corresponding assertions should make test 1 far more stable.

Now, 2 is supposed to test the marker recorder mechanism at docShell level, so it makes more sense that it does verify the content of the markers. But it fails to do so in a reliable manner. So this will require more work.

I'll upload a patch for 1 first.
Flags: needinfo?(pbrosset)
Hey Paul, what do you think of this change (see my previous comment for more info)?
I haven't yet sent that to try.
Attachment #8537393 - Flags: review?(paul)
About /docshell/test/browser/browser_timelineMarkers-02.js:
It relies on a helper function: waitForMarkers [1] which starts an interval, and checks if markers have been received at each iteration. As soon as at least 1 marker is received, it stops there and resolves.
I don't think anything guarantees that all the markers the test is interested in will be sent in one batch (especially on slow machine and with async reflows, ...).

What if we re-worked that function to make it take in a new argument which would be the list of expected marker types to wait for, and make it only resolve when those markers have been received?

Tom, you know this code best now, let me know your thoughts on this.

[1] http://mxr.mozilla.org/mozilla-central/source/docshell/test/browser/browser_timelineMarkers-02.js#143
Flags: needinfo?(ttromey)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #205)

> What if we re-worked that function to make it take in a new argument which
> would be the list of expected marker types to wait for, and make it only
> resolve when those markers have been received?
> 
> Tom, you know this code best now, let me know your thoughts on this.

Funny you should mention that -- it's the approach I took when writing
the tests for bug 1069661 :-)

I'll fix this up.
Assignee: nobody → ttromey
Flags: needinfo?(ttromey)
Flags: needinfo?(paul)
Cool, thanks Tom.
My patch to fix browser_timeline.js seems to be working (try is green).
Attachment #8537393 - Flags: review?(paul) → review+
Thanks Paul. Asking for check-in of this patch, leaving the bug open for Tom to fix the other test.
Here's the patch I'm testing.  It works the way Patrick suggested.
Attachment #8537393 - Attachment is obsolete: true
Comment on attachment 8537830 [details] [diff] [review]
avoid races in browser_timelineMarkers-02.js test

This did ok on "try".  Of course that isn't definitive for an
intermittent, but I think this patch reasonably obviously removes
some raciness.
Attachment #8537830 - Flags: review?(paul)
(In reply to Tom Tromey :tromey from comment #218)
> Comment on attachment 8537830 [details] [diff] [review]
> avoid races in browser_timelineMarkers-02.js test
> 
> This did ok on "try".  Of course that isn't definitive for an
> intermittent, but I think this patch reasonably obviously removes
> some raciness.
If you want to have a better degree of confidence on try, you need to re-trigger test jobs. I've done it for a few of them already. It's better to make try work a little bit more for intermittent fix patches rather than figuring out on fx-team or central that the intermittent is still here.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #219)
> (In reply to Tom Tromey :tromey from comment #218)
> > Comment on attachment 8537830 [details] [diff] [review]
> > avoid races in browser_timelineMarkers-02.js test
> > 
> > This did ok on "try".  Of course that isn't definitive for an
> > intermittent, but I think this patch reasonably obviously removes
> > some raciness.
> If you want to have a better degree of confidence on try, you need to
> re-trigger test jobs. I've done it for a few of them already. It's better to
> make try work a little bit more for intermittent fix patches rather than
> figuring out on fx-team or central that the intermittent is still here.

Thanks Patrick -- right you are.
I re-ran a few more of the bc2 tests.  I think these are the only ones actually
requiring a re-run.  So far it seems good.
Attachment #8537830 - Flags: review?(paul) → review+
Comment on attachment 8537393 [details] [diff] [review]
bug1066474-1-browser_timeline.js v1.patch

I should not have marked this as obsolete, whoops.
Attachment #8537393 - Attachment is obsolete: false
https://hg.mozilla.org/mozilla-central/rev/396c9b403e26
https://hg.mozilla.org/mozilla-central/rev/0d78284bbf8b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
Flags: qe-verify-
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)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.