Closed Bug 1289571 Opened 8 years ago Closed 8 years ago

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

Categories

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

50 Branch
x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- verified

People

(Reporter: kjozwiak, Assigned: jhao)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [userContextId][domsecurity-active])

Attachments

(1 file, 1 obsolete file)

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
Keywords: regression
Priority: -- → P1
Thank you, Kamil. I bisect between the godd and bad revision. The cause is this changeset: https://hg.mozilla.org/mozilla-central/rev/454b3874c934
Depends on: 1261842
Hm. Original work to fix this was in bug 1271516 - giving that solution a quick look...
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: nobody → jhao
Status: NEW → ASSIGNED
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
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+
Attachment #8775545 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: