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

VERIFIED FIXED in Firefox 50

Status

()

Core
DOM: Security
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: kjozwiak, Assigned: jhao)

Tracking

(Blocks: 1 bug, {regression})

50 Branch
mozilla50
x86
Mac OS X
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 unaffected, firefox48 unaffected, firefox49 unaffected, firefox50 verified)

Details

(Whiteboard: [userContextId][domsecurity-active])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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
(Reporter)

Updated

2 years ago
Keywords: regression

Updated

2 years ago
Priority: -- → P1
(Assignee)

Comment 1

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

Updated

2 years ago
Depends on: 1261842
Hm. Original work to fix this was in bug 1271516 - giving that solution a quick look...

Comment 3

2 years ago
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?
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?
(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.
(Assignee)

Updated

2 years ago
Assignee: nobody → jhao
Status: NEW → ASSIGNED
(Assignee)

Comment 6

2 years ago
Created attachment 8775545 [details] [diff] [review]
Set browser's userContextId before updateRemoteness in restoreTabContent.
Attachment #8775545 - Flags: review?(mdeboer)
(Assignee)

Comment 7

2 years ago
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
Priority: P1 → P3
Whiteboard: [userContextId][domsecurity-backlog] → [userContextId][domsecurity-active]
Sorry, I haven't seen that this is actually a regression, hence putting back to P1.
Priority: P3 → P1
status-firefox47: --- → unaffected
status-firefox48: --- → unaffected
status-firefox49: --- → unaffected
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. ;-)
Attachment #8775545 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8775885 [details] [diff] [review]
Set browser's userContextId before updateRemoteness in restoreTabContent.

Thank you, Mike!
(Assignee)

Updated

2 years ago
Attachment #8775545 - Attachment is obsolete: true

Comment 12

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f06b617447c
Set browser's userContextId before updateRemoteness in restoreTabContent. r=mikedeboer
Keywords: checkin-needed

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f06b617447c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Reporter)

Comment 14

2 years ago
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
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
You need to log in before you can comment on or make changes to this bug.