1157718 - browser_markers-parse-html.js and many other Performance tests are going to permafail when Gecko 40 merges to Aurora

Status: VERIFIED FIXED

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

Description

[Ryan VanderMeulen [:RyanVM]]

I *think* I ran a simulation push after bug 1151703 initially added this test, so calling it a regression from bug 1077464. That said, I could be misremembering.

https://treeherder.mozilla.org/logviewer.html#?job_id=6870719&repo=try

23:42:28 INFO - 5136 INFO TEST-UNEXPECTED-FAIL | browser/devtools/performance/test/browser_markers-parse-html.js | Got an error: this._recordings.includes is not a function
23:42:28 INFO - PerformanceActorsConnection.prototype.stopRecording<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/performance/front.js:433:10
23:42:28 INFO - PerformanceFront.prototype.stopRecording@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/performance/front.js:656:12
23:42:28 INFO - getMarkers@chrome://mochitests/content/browser/browser/devtools/performance/test/browser_markers-parse-html.js:23:9
23:42:28 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23
23:42:28 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7
23:42:28 INFO - this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37
23:42:28 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:5
23:42:28 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:709:7
23:42:28 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:671:7
23:42:28 INFO - emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:104:9
23:42:28 INFO - Request.prototype.emit@resource://gre/modules/devtools/dbg-client.jsm:1195:29
23:42:28 INFO - DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:1012:7
23:42:28 INFO - LocalDebuggerTransport.prototype.send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:561:11
23:42:28 INFO - makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
23:42:28 INFO - makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
23:42:28 INFO - test@chrome://mochitests/content/browser/browser/devtools/performance/test/head.js:164:3
23:42:28 INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:771:9
23:42:28 INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:665:7
23:42:28 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59
23:42:28 INFO - -
23:42:28 INFO - Stack trace:
23:42:28 INFO - chrome://mochitests/content/browser/browser/devtools/performance/test/head.js:handleError:129
23:42:28 INFO - resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:Handler.prototype.process:870
23:42:28 INFO - resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:this.PromiseWalker.walkerLoop:746
23:42:28 INFO - resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:this.PromiseWalker.scheduleWalkerLoop/<:688
23:42:28 INFO - null:null:0
23:42:28 INFO - 5137 INFO finish() was called, cleaning up... 

etc...

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Is this just from the Array.prototype.includes usage?

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1157718-arrayincludes.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f1174451249

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8596692 [details] [diff] [review]
1157718-arrayincludes.patch

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

::: browser/devtools/performance/modules/front.js
@@ +429,5 @@
>     */
>    stopRecording: Task.async(function*(model) {
>      // If model isn't in the PerformanceActorsConnections internal store,
>      // then do nothing.
> +    if (!~this._recordings.indexOf(model)) {

You want `indexOf == -1`. 0 is a valid index, but would trigger an early return here.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8596692 [details] [diff] [review]
1157718-arrayincludes.patch

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

::: browser/devtools/performance/modules/front.js
@@ +429,5 @@
>     */
>    stopRecording: Task.async(function*(model) {
>      // If model isn't in the PerformanceActorsConnections internal store,
>      // then do nothing.
> +    if (!~this._recordings.indexOf(model)) {

This is the only time I ever use a bitwise not, making 0, and all positive integers truthy number values, and -1 a falsy number value, with logical not flipping it. This is consistent with other code in the performance tools, but can change if strong feelings

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Oh, wow I didn't even notice that there.

I think it would be much more readable/clear to just check for -1.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

r+ with check for -1

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1157718-arrayincludes.patch

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/db80ed8ea070

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db80ed8ea070