1289571 - Regression: File -> New Container Tab doesn't work if there are no windows open

Status: VERIFIED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Kamil Jozwiak [:kjozwiak]]

Opening container windows via "File -> New Container Tab" has regressed in the latest version of fx50. Mozregression found the following range:

12:29.52 INFO: Last good revision: 7c669d5d63efceb12696cd65cfa72c296013dafb
12:29.52 INFO: First bad revision: f44bb9de08ade45299223de89953c6d0f4d003d1
12:29.52 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7c669d5d63efceb12696cd65cfa72c296013dafb&tochange=f44bb9de08ade45299223de89953c6d0f4d003d1

Some of the errors that appeared under the terminal that might be relevant:

[Parent 89946] WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x80004002: file /Users/kjozwiak/projects/m-c-containers/dom/base/nsDocument.cpp, line 5990
[Child 89957] WARNING: nsAppShell::Exit() called redundantly: file /Users/kjozwiak/projects/m-c-containers/widget/cocoa/nsAppShell.mm, line 678
[Child 89957] WARNING: NS_ENSURE_TRUE(maybeContext) failed: file /Users/kjozwiak/projects/m-c-containers/xpcom/threads/nsThread.cpp, line 921

Here's the full log of the errors that appear under the terminal once you've opened a container via the file menu while there's no fx windows opened:
* https://pastebin.mozilla.org/8887066

Comment 1

[Jonathan Hao (inactive) [:jhao]]

Thank you, Kamil. I bisect between the godd and bad revision. The cause is this changeset: https://hg.mozilla.org/mozilla-central/rev/454b3874c934

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hm. Original work to fix this was in bug 1271516 - giving that solution a quick look...

Comment 3

[Tanvi Vyas[:tanvi]]

Is there a way we can add an automated test to make sure we don't regress this again?  How do we make an automated test that opens the file menu and then checks that a tab is properly decorated?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Solution from that bug looks sensible to me at a glance. Hrm.

What bug 1261842 did was make the initial browser in new windows remote as soon as possible. Does that help isolate where the trouble is?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #3)
> Is there a way we can add an automated test to make sure we don't regress
> this again?  How do we make an automated test that opens the file menu and
> then checks that a tab is properly decorated?

Yes, I believe it should be possible to get OS X into the state where no windows are open but the hidden window (which provides the menu bar in that case) is still around. I'm reasonably certain we cannot instrument the native menubar itself, but I believe we can run the command that the menubar item for opening a new container tab fires.

Comment 6

[Jonathan Hao (inactive) [:jhao]]

Attachment: Set browser's userContextId before updateRemoteness in restoreTabContent.

Comment 7

[Jonathan Hao (inactive) [:jhao]]

At _loadURIWithFlags[1], the browser has already be remote because of bug 1261842. So it goes to LoadInOtherProcess() in the else branch, and eventually to restoreTabContent() in SessionStore.jsm.

I set the browser's usercontextid attribute before updateBrowserRemoteness(), in which nsFrameLoader::MaybeCreateDocShell() will populate the userContextId based on the browser's usercontextid attribute.[2]

[1]: http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#812
[2]: http://searchfox.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#3485

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Sorry, I haven't seen that this is actually a regression, hence putting back to P1.

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775545 [details] [diff] [review]
Set browser's userContextId before updateRemoteness in restoreTabContent.

Review of attachment 8775545 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/SessionStore.jsm
@@ +3370,5 @@
>      let uri = activePageData ? activePageData.url || null : null;
>      if (aLoadArguments) {
>        uri = aLoadArguments.uri;
> +
> +      if (aLoadArguments.userContextId) {

I'm fine with this, as long as you remove the newline above here and fix the indentation to two characters. ;-)

Comment 10

[Jonathan Hao (inactive) [:jhao]]

Attachment: Set browser's userContextId before updateRemoteness in restoreTabContent.

Thank you, Mike!

Comment 11

[Jonathan Hao (inactive) [:jhao]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f06b617447c
Set browser's userContextId before updateRemoteness in restoreTabContent. r=mikedeboer

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f06b617447c

Comment 14

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following build:

* https://archive.mozilla.org/pub/firefox/nightly/2016/07/2016-07-31-03-02-03-mozilla-central/
** fx50.0a1, buildId: 20160731030203, changeset: e5859dfe0bcb
* https://archive.mozilla.org/pub/firefox/nightly/2016/08/2016-08-02-03-04-37-mozilla-central/
** fx51.0a1, buildId: 20160802030437, changeset: ffac2798999c
* also used the following local debug mode: fx51.0a1, buildId: 20160802120943, changeset: 6608e5864780 tip (using --enable-debug

I went through the following test cases when there's currently no windows fx opened:

* opened several tabs via File -> New Tab
* opened several Windows via File -> New Window
* opened every container via File -> New Container Tab
* opened several PB sessions via File -> New Private Window
* opened several non-e10s windows via File -> New non-e10s Window
* opened a new window via right clicking on the FX Icon under the dock and selecting New Window
* opened a new PB session via right clicking on the FX Icon under the dock and selecting New Private Window
* opened a new window by simply clicking on the FX icon under the dock