1540314 - Creating two Promises in the same test throws exception.

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Randall Barker [:rbarker]]

Trying to create a second promise in a GeckoView test causes the following error:

03-29 16:14:40.720 28430 28446 I Gecko   : console.error: "TypeError: this._navigationLifetimePool.cleanup is not a function: PromisesActor<._onWindowReady<@resource://devtools/server/actors/promises.js:194:34\nexpectState/<@resource://devtools/server/actors/common.js:213:23\nemit@resource://devtools/shared/event-emitter.js:186:24\nemit@resource://devtools/shared/event-emitter.js:267:18\n_windowReady@resource://devtools/server/actors/targets/browsing-context.js:1277:10\nDebuggerProgressListener.prototype.onWindowCreated<@resource://devtools/server/actors/targets/browsing-context.js:1593:23\nexports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:22\n"
03-29 16:14:40.720 28430 28446 E Web Content: [JavaScript Error: "TypeError: this._navigationLifetimePool.cleanup is not a function: PromisesActor<._onWindowReady<@resource://devtools/server/actors/promises.js:194:34
03-29 16:14:40.720 28430 28446 E Web Content: expectState/<@resource://devtools/server/actors/common.js:213:23
03-29 16:14:40.720 28430 28446 E Web Content: emit@resource://devtools/shared/event-emitter.js:186:24
03-29 16:14:40.720 28430 28446 E Web Content: emit@resource://devtools/shared/event-emitter.js:267:18
03-29 16:14:40.720 28430 28446 E Web Content: _windowReady@resource://devtools/server/actors/targets/browsing-context.js:1277:10
03-29 16:14:40.720 28430 28446 E Web Content: DebuggerProgressListener.prototype.onWindowCreated<@resource://devtools/server/actors/targets/browsing-context.js:1593:23
03-29 16:14:40.720 28430 28446 E Web Content: exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:22
03-29 16:14:40.720 28430 28446 E Web Content: "]
03-29 16:14:40.720 28430 28446 I GeckoDump: TypeError: this._navigationLifetimePool.cleanup is not a function: PromisesActor<._onWindowReady<@resource://devtools/server/actors/promises.js:194:34
03-29 16:14:40.720 28430 28446 I GeckoDump: expectState/<@resource://devtools/server/actors/common.js:213:23
03-29 16:14:40.720 28430 28446 I GeckoDump: emit@resource://devtools/shared/event-emitter.js:186:24
03-29 16:14:40.720 28430 28446 I GeckoDump: emit@resource://devtools/shared/event-emitter.js:267:18
03-29 16:14:40.720 28430 28446 I GeckoDump: _windowReady@resource://devtools/server/actors/targets/browsing-context.js:1277:10
03-29 16:14:40.720 28430 28446 I GeckoDump: DebuggerProgressListener.prototype.onWindowCreated<@resource://devtools/server/actors/targets/browsing-context.js:1593:23
03-29 16:14:40.720 28430 28446 I GeckoDump: exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:22

Only being able to create a single Promise per test makes creating repetitive test difficult.

Comment 1

[Randall Barker [:rbarker]]

I believe I know what was causing the issue:

    private fun waitForScroll(offset: Double, timeout: Double, param: String) {
        sessionRule.evaluateJS(mainSession, """
           const start = Date.now(); // This const variable seems to cause problems.
           new Promise((resolve, reject) => {
             function step() {
               console.log("REB  window.visualViewport.$param = " + window.visualViewport.pageTop);
               if (window.visualViewport.$param >= ($offset - $errorEpsilon)) {
                 resolve();
               } else if ($timeout < (Date.now() - start)) {
                 reject();
               } else {
                 window.requestAnimationFrame(step);
               }
             }
             window.requestAnimationFrame(step);
           });
        """.trimIndent()).asJSPromise().value
    }

Having the const start = Data.now() in the global scope seems to have caused the error.

Comment 2

[Chris Peterson [:cpeterson]]

I'm moving this bug to the Core::DOM component because Randall says appears to be a Promise bug, not a GeckoView bug. We're seeing this bug more frequently now that the GeckoView tests are using Promises more.

Comment 3

[Chris Peterson [:cpeterson]]

James says this is a detach() bug in the DevTools' Promise code, not the Promise Web API.

Comment 4

[Chris Peterson [:cpeterson]]

Harald, how do we get this bug in DevTools bug triage?

James can't reproduce this, but says it should be a straightforward fix for someone on the DevTools team.

Comment 5

[:Harald Kirschner :digitarald]

Jim, could you assess the issue to suggest next steps?

Comment 6

[Jim Blandy :jimb]

James can't reproduce this, but says it should be a straightforward fix for someone on the DevTools team.

I think you overestimate our abilities. It is very hard to fix a bug without being able to reproduce it. If you can provide steps to reproduce, then we can start taking a look at it.

The information in this bug leaves many questions unanswered:

• It is forbidden by ECMAScript to re-declare a const variable. So loading the triple-quoted code twice in the same global will certainly cause an error.

• But the backtrace shows an error being thrown by devtools server code. I don't know how GeckoView tests work - why is the devtools server even active?

But, not having steps to reproduce is a blocker.

Comment 7

[Jim Blandy :jimb]

Attachment: Bug 1540314: Devtools promise actor: Use ActorPool 'destroy' method. r?gl

The prior code calls '.cleanup()', but ActorPools have no 'cleanup' method. I
think it means to call 'destroy'.

Comment 8

[Jim Blandy :jimb]

Well, I take that back, maybe we can fix this one just from the backtrace...

Comment 9

[Chris Peterson [:cpeterson]]

(In reply to Jim Blandy :jimb from comment #6)

• It is forbidden by ECMAScript to re-declare a const variable. So loading the triple-quoted code twice in the same global will certainly cause an error.

Yes, trying to define const start = Data.now() in the global scope twice sounds like a problem. This evaluateJS() code should probably be wrapped in an IIFE scope, if evaluateJS() doesn't automatically do this.

(In reply to Jim Blandy :jimb from comment #7)

The prior code calls '.cleanup()', but ActorPools have no 'cleanup' method. I
think it means to call 'destroy'.

Oh! That fix makes sense given the "_navigationLifetimePool.cleanup is not a function" warning in comment 0's logs.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40dad5764be7
Devtools promise actor: Use ActorPool 'destroy' method. r=gl

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/40dad5764be7

Comment 12

[Chris Peterson [:cpeterson]]

Randall, does Jim's promise fix also fix the GV test problems?

Comment 13

[Randall Barker [:rbarker]]

(In reply to Chris Peterson [:cpeterson] from comment #12)

Randall, does Jim's promise fix also fix the GV test problems?

It should. We'll know for certain if the intermittent failures stop.

Comment 14

[Jim Blandy :jimb]

How are the intermittents doing? Can we close this?

Comment 15

[Randall Barker [:rbarker]]

Looks promising: https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?startday=2019-05-24&endday=2019-05-31&tree=trunk&bug=1542525