1169600 - "MarionetteException: this.browserForTab is null" when closing the first tab of a chrome window

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: test_tabbar.py

Closing the first tab of a chrome browser window results in a MarionetteException because this.browserForTab does not longer exist.

As it looks like we register the Marionette stuff only with the first tab, and when its gone Marionette will no longer work. 

Here the stacktrace:

A coding exception was thrown and uncaught in a Task.
Full message: TypeError: this.browserForTab is null
Full stack: BrowserObj.prototype.hasRemotenessChange@chrome://marionette/content/driver.js:3177:7
BrowserObj.prototype.executeWhenReady@chrome://marionette/content/driver.js:3208:7
GeckoDriver.prototype.sendAsync@chrome://marionette/content/driver.js:223:5
ContentSender.prototype.send/proxy<@chrome://marionette/content/proxy.js:152:5
ContentSender.prototype.send@chrome://marionette/content/proxy.js:99:15
proxy.toListener/handler.get/<@chrome://marionette/content/proxy.js:48:27
GeckoDriver.prototype.getPageSource@chrome://marionette/content/driver.js:1256:26
TaskImpl_run@resource://gre/modules/Task.jsm:330:41
TaskImpl@resource://gre/modules/Task.jsm:275:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
Task_spawn@resource://gre/modules/Task.jsm:164:12
TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:381:1
TaskImpl_run@resource://gre/modules/Task.jsm:322:13
TaskImpl@resource://gre/modules/Task.jsm:275:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
Task_spawn@resource://gre/modules/Task.jsm:164:12
CommandProcessor.prototype.execute@chrome://marionette/content/command.js:153:13
Dispatcher.prototype.onPacket@chrome://marionette/content/dispatcher.js:80:5
DebuggerTransport.prototype._onJSONObjectReady/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:471:9
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14

Please also see the attachment, which let you easily reproduce the failure.

This breakage is blocking some of our Firefox UI Tests. So I'm going to file them as deps and disable those until this bug has been fixed.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is actually a regression from bug 1107706. Andreas could you have a look at this please?

Comment 2

[Chris Manchester (limited bugmail, email directly)]

This particular exception is in a message listener that's receiving a message that isn't relevant to it so appears benign, but avoiding it looks straightforward. According to :whimboo the tests that were failing are fixed with bug 1169798

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Possible fix here, still need to verify on the test machine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6b977e602fd

Given the nature of the change I'm not sure it's worth uplifting.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Chris, I checked this tryserver build and it works. The errors are no longer shown.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1169600 - Remove message listeners intended to coordinate registering a new browser with marionette once the browser has been registered. r=ato

Bug 1169600 - Avoid misleading exception in message listeners in marionette server.;r=ato

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8614779 [details]
MozReview Request: Bug 1169600 - Remove message listeners intended to coordinate registering a new browser with marionette once the browser has been registered. r=ato

https://reviewboard.mozilla.org/r/10039/#review9377

Ship It!

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f253a0baf1e3

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Checking on bug 1176663 identified the underlying issue here (which my previous patch only papered over). We add message listeners to coordinate browser registration as well as to get results from a content script. We're not correctly removing the message listeners we use to coordinate the browser getting registered, so all the logic to register a browser (and re-register when a browser changes process) is getting called more often than expected and results in unhealthy states.

In the case of bug 1176663, we have a cached flag that says we're changing process that's getting ignored in one out of the three responses to the frame script registering itself. Because it gets ignored in the first of the three, the listener doesn't know to proceed.

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

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Henrik, can you verify the try build from comment 9 avoids the original exception we were addressing?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Yes, so far as I can see it's working now. No hang and also no javascript errors shown in the gecko log.

IMHO we should have left this bug closed and do all the work on the newly filed bug, which is the normal workflow.

Comment 12

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8614779 [details]
MozReview Request: Bug 1169600 - Remove message listeners intended to coordinate registering a new browser with marionette once the browser has been registered. r=ato

Bug 1169600 - Remove message listeners intended to coordinate registering a new browser with marionette once the browser has been registered. r=ato

Comment 13

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90f08994e3de

Comment 14

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8614779 [details]
MozReview Request: Bug 1169600 - Remove message listeners intended to coordinate registering a new browser with marionette once the browser has been registered. r=ato

https://reviewboard.mozilla.org/r/10039/#review10293

Ship It!

Comment 15

[Chris Manchester (limited bugmail, email directly)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/affd99e9228e

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Chris, do you think we have to backport anything to older branches?

Comment 18

[Chris Manchester (limited bugmail, email directly)]

There wasn't a merge between comment 7 and comment 16, so I don't think so. The original patch in comment 7 was intended to suppress the exception message, I'm not aware of any other problems this was causing.