Closed Bug 1169798 Opened 9 years ago Closed 9 years ago

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

Categories

(Remote Protocol :: Marionette, defect)

39 Branch
defect
Not set
major

Tracking

(firefox39 fixed, firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: whimboo, Assigned: chmanchester)

References

Details

(Keywords: regression, Whiteboard: [qa-automation-blocked])

Attachments

(3 files, 1 obsolete file)

Attached file 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.
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.
Blocks: 1096488
Keywords: regression
Version: 41 Branch → 39 Branch
Attached file log.txt (obsolete) —
Log output with marionette logging turned on.
Attached file log.txt
The non-cut off version of the log.
Attachment #8613150 - Attachment is obsolete: true
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).
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.
Assignee: nobody → cmanchester
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
Bug 1169798 - Refresh the marionette server's window reference when switching between windows to avoid intermittent exception.;r=ato
Attachment #8613245 - Flags: review?(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

Bug 1169798 - Refresh the marionette server's window reference when switching between windows to avoid intermittent exception.;r=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

There's a problem with e10s in that last try run, I'll re-request review when I figure out what's up.
Attachment #8613245 - Flags: review?(ato)
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.
Status: NEW → ASSIGNED
Blocks: 1169644
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 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
Attachment #8613245 - Flags: review?(ato)
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.
Attachment #8613245 - Flags: review?(ato) → review+
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!
Blocks: 1165215
https://hg.mozilla.org/mozilla-central/rev/f8ec794163af
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
According to Henrik this has fixed the test failures in question so we'd like to uplift it to aurora and beta.
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.