John Dorlus [:Silne30] Reporter
	Description • 7 years ago

I performing a search via the location bar in puppeteer and it opens a new tab. When I try to close that tab, I get the above error. I attached my test script for your review.

	krupa raj [:krupa--use this to needinfo]
	Comment 2 • 7 years ago

This issue is blocking us from landing a telemetry test. Henrik, mind taking a look?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 7 years ago

Can you also please attach the log? This is more helpful than the testcase actually. Thanks.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 4 • 7 years ago

Maybe this is related to bug 1352984, but I cannot say this without a log.

	John Dorlus [:Silne30] Reporter
	Comment 5 • 7 years ago

Log attached.

	John Dorlus [:Silne30] Reporter
	Comment 6 • 7 years ago

The gecko log did not have any additional information.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 7 • 7 years ago

The stack looks a bit different than the other test failures we currently have but also fails in tabbar.py. Here specifically in `get_handle_for_tab()`. Lets see if bug 1352984 will fix it, otherwise we will need a separate patch. 

Maybe you can workaround by specifically focusing the newly opened tab.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 8 • 7 years ago

Is this still present for you?

	John Dorlus [:Silne30] Reporter
	Comment 9 • 7 years ago

Just checked. No. This is not still present.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 10 • 7 years ago

So this can still be an issue as seen on bug 1219725 comment 117:

1497299333048        Marionette        TRACE        2 -> [0,125,"findElement",{"using":"id","value":"tabbrowser-tabs","element":"0486c14b-f539-3f42-94fe-1ee8a8ebbaf3"}]
1497299333051        Marionette        TRACE        2 <- [1,125,null,{"value":{"element-6066-11e4-a52e-4f735466cecf":"586c8c1e-b204-cc43-a0b0-f038aaee8534","ELEMENT":"586c8c1e-b204-cc43-a0b0-f038aaee8534"}}]
1497299333053        Marionette        TRACE        2 -> [0,126,"findElements",{"using":"tag name","value":"tab","element":"586c8c1e-b204-cc43-a0b0-f038aaee8534"}]
1497299333058        Marionette        TRACE        2 <- [1,126,null,[{"element-6066-11e4-a52e-4f735466cecf":"dc47494a-e346-0a47-ad55-2ab979c222a8","ELEMENT":"dc47494a-e346-0a47-ad55-2ab979c222a8"},{"element-6066-11e4-a52e-4f735466cecf":"ae20660d-cb85-1a40-9659-66a388e61d1f","ELEMENT":"ae20660d-cb85-1a40-9659-66a388e61d1f"},{"element-6066-11e4-a52e-4f735466cecf":"c13249c5-918f-d54f-b160-902ef6dc0e6f","ELEMENT":"c13249c5-918f-d54f-b160-902ef6dc0e6f"}]]
1497299333063        Marionette        TRACE        2 -> [0,127,"executeScript",{"scriptTimeout":null,"newSandbox":true,"args":[{"element-6066-11e4-a52e-4f735466cecf":"dc47494a-e346-0a47-ad55-2ab979c222a8","ELEMENT":"dc47494a-e346-0a47-ad55-2ab979c222a8"}],"filename":"tabbar.py","script":"\n          let win = arguments[0].linkedBrowser;\n          if (!win) {\n            return null;\n          }\n          return win.outerWindowID.toString();\n        ","sandbox":"default","line":218}]
1497299333067        Marionette        TRACE        2 <- [1,127,{"error":"javascript error","message":"TypeError: win.outerWindowID is undefined","stacktrace":"execute_script @tabbar.py, line 218\ninline javascript, line 5\nsrc: \"          return win.outerWindowID.toString();\"\nde:\n@tabbar.py:5:11\n@tabbar.py:0:2\nevaluate.sandbox/promise<@chrome://marionette/content/evaluate.js:154:13\nevaluate.sandbox@chrome://marionette/content/evaluate.js:105:17\nGeckoDriver.prototype.execute_@chrome://marionette/content/driver.js:894:29\nGeckoDriver.prototype.executeScript@chrome://marionette/content/driver.js:793:27\nTaskImpl_run@resource://gre/modules/Task.jsm:321:42\nTaskImpl@resource://gre/modules/Task.jsm:279:3\nasyncFunction@resource://gre/modules/Task.jsm:254:14\nTask_spawn@resource://gre/modules/Task.jsm:168:12\nTaskImpl_handleResultValue@resource://gre/modules/Task.jsm:391:16\nTaskImpl_run@resource://gre/modules/Task.jsm:329:15\nTaskImpl@resource://gre/modules/Task.jsm:279:3\nasyncFunction@resource://gre/modules/Task.jsm:254:14\nTask_spawn@resource://gre/modules/Task.jsm:168:12\nexecute@chrome://marionette/content/server.js:510:15\nonPacket@chrome://marionette/content/server.js:481:7\n_onJSONObjectReady/<@chrome://marionette/content/transport.js:480:9\n"},null]

The affected code here is:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/browser/tabbar.py#196-202

So as we also noticed on bug 1332122 and bug 1363368 the `linkedBrowser` can be null in case the browser hasn't done its initialization. So we shouldn't simply return null, but wait until win is valid. Then I'm not sure why `outerWindowID` is undefined.

Maybe best is to use the patch / test on bug 1219725 and trying to reproduce.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 11 • 7 years ago

The problem here is that the underlying tab is gone, or got replaced. This will/can happen for sessionrestore after a restart.

So the underlying issue here is in Marionette and will be fixed with bug 1373191.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 12 • 7 years ago

The problem here only persists after a restart of Firefox with sessionrestore enabled. Especially in such a condition when a background tab hasn't been restored yet. In such a situation there is no valid content browser and window available. Attached a testcase for it.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 13 • 7 years ago

Andreas, do you have an idea how we should handled not yet restored tabs? In all those cases the tab will be displayed but each of those won't have a contentBrowser yet. It means we cannot interact with the tabs, but users should be able to switch to those. Once that is done, the content browser is initialized.

Is it something you can fold into your patch for the refactoring, or should I take care of it separately?

	Andreas Tolfsen ❲:ato❳
	Comment 14 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #13)

> Andreas, do you have an idea how we should handled not yet
> restored tabs? In all those cases the tab will be displayed but
> each of those won't have a contentBrowser yet. It means we cannot
> interact with the tabs, but users should be able to switch to
> those. Once that is done, the content browser is initialized.

That’s an interesting case.  Like you say, the user should
definitely be able to interact with it and Marionette needs
to handle that a tab’s content browser may not yet have
initialised/appeared.

> Is it something you can fold into your patch for the refactoring,
> Ior should take care of it separately?

I’m trying hard not to make any feature changes or conformance
fixes in https://bugzilla.mozilla.org/show_bug.cgi?id=1311041, so I
think it would be a good idea to address this separately and write a
good test case for it.  If there’s a test case, I will try my best
to not regress that later.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 15 • 7 years ago

Ok, so I will keep this bug for puppeteer, and file a new one which will take care of necessary changes in Marionette code.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 18 • 6 years ago

I'm not actually working on this bug. Also we most likely need bug 1140237 fixed first.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 19 • 6 years ago

Well, I meant bug 1311041.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 21 • 6 years ago

Actually this is not dependent on bug 1311041! I was just able to reproduce it constantly with a minimize testcase I was using to reproduce bug 1510181. Instead it's a plain problem in an execute_script call when we try to access `.toString()` on `null`.

It's an easy fix, so taking...

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 22 • 6 years ago

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

	Pulsebot
	Comment 24 • 6 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95c294286e66
[marionette] Only convert a valid outerWindowID to a string. r=ato

	Cristina Coroiu [:ccoroiu]
	Comment 25 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/95c294286e66

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 26 • 6 years ago

Silly mistake in a library for testing. Please uplift to beta too to reduce test failures on that branch.

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/f65cadcba4bd

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/3e8c9f8c5307