1169798 - MarionetteException in switch_to_window() with chrome scope enabled: "this.browser is undefined"

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/02 - 10/06)]

Attachment: test_windows.py

In our Firefox UI Tests I see constant test failures on Windows machines when opening and closing chrome windows. The root cause is located somewhere in switch_to_window().

Please see the attachment for a testcase. The failure might not always occur, so please run the test multiple times. When commenting out the chrome context switch at the beginning, the test will pass for me. Keep also in mind that this exception does only occur on Windows but not on OS X and Linux.

Here the stack:

Full message: TypeError: this.browser is undefined
Full stack: get@chrome://marionette/content/driver.js:3022:5
BrowserObj.prototype.switchToTab@chrome://marionette/content/driver.js:3123:3
GeckoDriver.prototype.switchToWindow@chrome://marionette/content/driver.js:1520:
9
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

Code in question here:

Object.defineProperty(BrowserObj.prototype, "browserForTab", {
  get() {
    return this.browser.getBrowserForTab(this.tab);
  }
});


This misbehavior is blocking us from having green tests, and are also causing test failures in the update tests. That's why it is blocking us.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/02 - 10/06)]

Chris mentioned that this could be a regression. So I did a regression test and it's indeed a regression. Specifically in Nightly between 03/11 and 03/12. Here the pushlog:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fd8e079d6335&tochange=58c9d079f318

I strongly believe that this is a fallout from bug 1096488.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/02 - 10/06)]

Attachment: log.txt

Log output with marionette logging turned on.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/02 - 10/06)]

Attachment: log.txt

The non-cut off version of the log.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Looking at the log, I think there's a timing issue where we end up with a reference to a window that's no longer valid. Bug 1096488 added a bunch of interactions with gBrowser when we switch between windows and makes us lean more on the validity of BrowserObj.window, both of which seem relevant.

I'm not entirely sure so I'd like to debug on one of the machines where this is reproducible (bug 1169874 is about getting the access to do that).

Comment 5

[Chris Manchester (limited bugmail, email directly)]

I have a possible fix on try (not sure if it reproduces there though). There isn't a build environment on the test machine, so debugging there wasn't especially informative.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

The problem doesn't reproduce on try, but I downloaded a build with my fix from try to the test machine and confirmed it works. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de268bb3c082
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cb5e03d834a

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1169798 - Refresh the marionette server's window reference when switching between windows to avoid intermittent exception.;r=ato

Bug 1169798 - Refresh the marionette server's window reference when switching between windows to avoid intermittent exception.;r=ato

Comment 8

[Chris Manchester (limited bugmail, email directly)]

New try:

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

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8613245 [details]
MozReview Request: Bug 1169798 - Refresh the marionette server's window reference when switching between windows to avoid intermittent exception.;r=ato

Bug 1169798 - Refresh the marionette server's window reference when switching between windows to avoid intermittent exception.;r=ato

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8613245 [details]
MozReview Request: Bug 1169798 - Refresh the marionette server's window reference when switching between windows to avoid intermittent exception.;r=ato

There's a problem with e10s in that last try run, I'll re-request review when I figure out what's up.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/02 - 10/06)]

Great progress Chris! Thanks a lot for digging into this problem that quickly. Do you have any idea why only Windows boxes are showing this problem? Maybe they are a bit slower, which is causing this race condition? Let me know if you need something else or getting tested.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

I've seen bugs before that only surfaced on old windows versions, so I wouldn't be surprised if their slowness was causing us to hit this. Unfortunately I can't point to a definitive root cause for this, so the patch I have is addressing mostly the symptom. 

I fixed the issue in comment 10, I'm just waiting or a try build to verify on the test machine.

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8613245 [details]
MozReview Request: Bug 1169798 - Refresh the marionette server's window reference when switching between windows to avoid intermittent exception.;r=ato

Bug 1169798 - Refresh the marionette server's window reference when switching between windows to avoid intermittent exception.;r=ato

Comment 15

[Chris Manchester (limited bugmail, email directly)]

The new try build is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b2bb52fad6f and works on the test machine with this test case.

Comment 16

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/9729/#review8573

Comment 17

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8613245 [details]
MozReview Request: Bug 1169798 - Refresh the marionette server's window reference when switching between windows to avoid intermittent exception.;r=ato

https://reviewboard.mozilla.org/r/9731/#review8575

Ship It!

Comment 18

[Chris Manchester (limited bugmail, email directly)]

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

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Chris Manchester (limited bugmail, email directly)]

According to Henrik this has fixed the test failures in question so we'd like to uplift it to aurora and beta.

Comment 21

[Chris Manchester (limited bugmail, email directly)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3bd2f562d587
https://hg.mozilla.org/releases/mozilla-beta/rev/d8904a3f0278