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

RESOLVED FIXED in Firefox 41

Status

defect
--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: whimboo, Assigned: chmanchester)

Tracking

({pi-marionette-server, regression})

39 Branch
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments)

Posted file 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.
This is actually a regression from bug 1107706. Andreas could you have a look at this please?
Keywords: regression
Version: 38 Branch → 39 Branch
Flags: needinfo?(ato)
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
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.
Chris, I checked this tryserver build and it works. The errors are no longer shown.
Assignee: nobody → cmanchester
Status: NEW → ASSIGNED
Flags: needinfo?(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!
Attachment #8614779 - Flags: review?(ato) → review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f253a0baf1e3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1176663
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Updated

4 years ago
Blocks: 1107706
Henrik, can you verify the try build from comment 9 avoids the original exception we were addressing?
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 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
Attachment #8614779 - Attachment description: MozReview Request: Bug 1169600 - Avoid misleading exception in message listeners in marionette server.;r=ato → MozReview Request: Bug 1169600 - Remove message listeners intended to coordinate registering a new browser with marionette once the browser has been registered. r=ato
Attachment #8614779 - Flags: review+ → review?(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!
Attachment #8614779 - Flags: review?(ato) → review+
https://hg.mozilla.org/mozilla-central/rev/affd99e9228e
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Chris, do you think we have to backport anything to older branches?
Flags: needinfo?(cmanchester)
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.
Flags: needinfo?(cmanchester)
You need to log in before you can comment on or make changes to this bug.