Jonathan Kingston [:jkt] he/him Reporter
	Description • 7 years ago

Developers who wish you create "container windows" are going to have a hard time currently and this is a very highly requested feature in the containers bugs.

Currently a user would have to transfer an existing container tab or create a blank tab that gets deleted to add their first container tab.

If we add a cookieStoreId field all tabs created from the url property would be in this container.

This wouldn't prevent the user then adding different cookieStoreId tabs to the window or using tabId which would use the existing cookieStoreId there.

	Kris Maglione [:kmag]
	Comment 1 • 7 years ago

I kind of worry that developers will mistake the meaning of "cookieStoreId" in this case, and think it applies to the entire window, the way that "incognito" does.

I'd rather we change it so that we can pass a list of tabs as the same sort of objects we'd pass to `tabs.create`

	Jonathan Kingston [:jkt] he/him Reporter
	Comment 2 • 7 years ago

Agreed that is a concern and there isn't current plans to make that restriction in place or change the default tab to be that cookieStoreId either.

I'm happy to see that as a solution though, this came from a developer wishing to do this not myself.

Their use-case was:
- In Window 1, use a blocking web request to prevent navigations to certain urls
- Open a new window 2 in a cookieStoreId for the url blocked in window 1

	Jamie Phelps
	Comment 3 • 7 years ago

I'm working on adding support for containers to 1Password's open and fill process (Make 1Password use the same container as the active tab whether opening a new tab in the current window or opening a new window) and this would be very helpful.

Right now, my workaround is a little bit clunky. I capture the cookieStoreId of the current tab before opening a new window. Then I either create the new window with the URL if the cookieStoreId was not available or create a new empty window, create a new tab in that window with the cookieStoreId, and then close the initial tab of the window.

I think allowing an array of Tab-describing Objects would be nice as opposed to passing a cookieStoreId for the window itself (It would allow opening multiple URLs in different containers, for instance), but I would caution against overloading windows.create in a way that could affect extensions that are sharing a lot of code between Firefox and Chrome.

	Caitlin Neiman [:caitmuenster]
	Comment 4 • 6 years ago

Hi Jonathan, this has been added to the agenda for the January 18, 2018 WebExtensions APIs triage. Would you be able to join us? 

Here’s a quick overview of what to expect at the triage: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/WebExtensions/Triage#Next_Meeting
* Meeting agenda: https://docs.google.com/document/d/13AiUqFgtLsuJ17QjzUOQ7nBN1U_ZEO0yzMaHLLl0JKk/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 5 • 6 years ago

This issue has been discussed in the last API triage meeting and it has been approved,
we also agreed that the concern from Comment 1 has to be solved to allow this
and the following is the approach suggested by Kris during the meeting:

Allowing in a browser.windows.create API call to use, as the url property of the createData parameter, 
a subset of the properties allowed in a browser.tabs.create API call (which includes cookieStoreId), 
e.g. to open a new window with a tab which loads an url in a given cookieStore: 

    browser.windows.create({
      url: [{url: "...", cookieStoreId: "...}],
      ...
    });

	Rob Wu [:robwu] Assignee
	Comment 6 • 6 years ago

> url: [{url: "...", cookieStoreId: "...}]

This looks a bit odd.

There is already precedence for a windows.create parameter that only applies to the initial tabs, the allowScriptsToClose parameter [1].

I think that cookieStoreId as a separate parameter makes for a better-looking API.
If there is a need for having distinct cookieStoreIds for every tab, then the cookieStoreId can be an array.

Currently the logic that opens multiple tabs in one window does not support distinct cookieStoreIds (userContextIds) for multiple windows:
https://searchfox.org/mozilla-central/rev/05d91d3e02a0780f44599371005591d7988e2809/browser/base/content/tabbrowser.js#1429,1494,1513

Luca, questions:
- Does a separate (well-documented) cookieStoreId property look good to you (opposed to putting it in "url")?
- Should we support distinct cookieStoreIds via an array?

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/windows/create

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 7 • 6 years ago

(In reply to Rob Wu [:robwu] from comment #6)
> Luca, questions:
> - Does a separate (well-documented) cookieStoreId property look good to you
> (opposed to putting it in "url")?

Well, I don't disagree :-), `url: [{url: "...", cookieStoreId: "...}]` seems definitely a bit odd.

How about calling the new property `initialCookieStoreId` so that even its name suggests that it is not
actually used for the tabs opened after the windows has been created?

> - Should we support distinct cookieStoreIds via an array?

Not sure how common would be for an extension to want to open an array of urls and put each of them into a different cookieStoreId, but in my own opinion it could be confusing for the user (and reviewers) and potentially error-prone for the developers (because the paired cookieStoreId is going to be in a array that is separate from the array of urls, instead of being visibly paired with the related url).

Personally I would vote for having a single cookieStoreId (as `initialCookieStoreId`), and do not support multiple cookie store ids in form of an array.

I'm also wondering if we should reject the window.create API call if an initialCookieStoreId and tabId parameters are both being used in the same call and the tab identified by tabId is not in the cookieStoreId specified in the initialCookieStoreId.

	Rob Wu [:robwu] Assignee
	Comment 8 • 6 years ago

(In reply to Luca Greco [:rpl] from comment #7)
> How about calling the new property `initialCookieStoreId` so that even its
> name suggests that it is not
> actually used for the tabs opened after the windows has been created?
"initialCookieStoreId" is no less confusing than "cookieStoreId", because it could suggest that it would apply to new tabs (=new tabs are initialized with the given cookieStoreId). Therefore I will stick to cookieStoreId (also for consistency with our other extension APIs).

> > - Should we support distinct cookieStoreIds via an array?
> 
> Not sure how common would be for an extension to want to open an array of
> urls and put each of them into a different cookieStoreId, but in my own
> opinion it could be confusing for the user (and reviewers) and potentially
> error-prone for the developers (because the paired cookieStoreId is going to
> be in a array that is separate from the array of urls, instead of being
> visibly paired with the related url).
> 
> Personally I would vote for having a single cookieStoreId (as
> `initialCookieStoreId`), and do not support multiple cookie store ids in
> form of an array.

Yeah, let's stick to one. Extensions can always use the tabs API if they want more than one.
One potential use case for having distinct cookieStoreIds is to support a custom form of window-session-restore.
If that is an actual use case, we can always add support later (single item = apply to all, array = array lengths must match and apply to individual tabs).

After having worked on the implementation, I think that it's better to not support multiple, distinct cookieStoreIds unless absolutely necessary.


> I'm also wondering if we should reject the window.create API call if an
> initialCookieStoreId and tabId parameters are both being used in the same
> call and the tab identified by tabId is not in the cookieStoreId specified
> in the initialCookieStoreId.

In my WIP I reject the API call when cookieStoreId is combined with tabId.

	Rob Wu [:robwu] Assignee
	Comment 9 • 6 years ago

This is a preparation to support cookieStoreId in the windows.create.

Depends on D4920

	Rob Wu [:robwu] Assignee
	Comment 10 • 6 years ago

Other (internal API) changes besides extension API changes:

- This also introduces support for opening a window with multiple tabs
  in a non-default container tab.

- This also adds LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL to the
  gBrowser.loadTabs call, unless allowInheritPrincipal is set. Currently
  there are no callers that set this flag, but in case it's desired,
  I added an opt-in via window.arguments[10] in browser.xul/js.

  For single-argument URLs, the flag is an opt-out, since there are
  multiple callers that rely on principal inheritance (bug 1475201).

Depends on D4928

	:Gijs (he/him)
	Comment 11 • 6 years ago

Comment on attachment 9006219 [details]
Bug 1393570 - Support cookieStoreId in windows.create extension API

:Gijs (he/him) has approved the revision.

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 12 • 6 years ago

Comment on attachment 9006218 [details]
Bug 1393570 - Move cookieStoreId validator to ext-tabs-base.js

Luca Greco [:rpl] has approved the revision.

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 13 • 6 years ago

Comment on attachment 9006219 [details]
Bug 1393570 - Support cookieStoreId in windows.create extension API

Luca Greco [:rpl] has approved the revision.

	Pulsebot
	Comment 14 • 6 years ago

Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/e41e69aa9eb5
Move cookieStoreId validator to ext-tabs-base.js r=rpl
https://hg.mozilla.org/integration/autoland/rev/23f496cd8a42
Support cookieStoreId in windows.create extension API r=Gijs,rpl

	Cristina Coroiu [:ccoroiu]
	Comment 15 • 6 years ago

Backed out 2 changesets (bug 1393570) for build bustage on linux

Backout: https://hg.mozilla.org/integration/autoland/rev/56a508217ab7e7c28e1e5622f8dd31caa98a8706

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=198882459&
revision=23f496cd8a42d49f732e0ba4bae7cb0cc8aa5818

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198882459&repo=autoland&lineNumber=38820

task 2018-09-12T16:04:15.706Z] 16:04:15     INFO -  make[8]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/browser/branding/nightly/locales'
[task 2018-09-12T16:04:15.706Z] 16:04:15     INFO -  make[8]: Nothing to be done for 'tools'.
[task 2018-09-12T16:04:15.721Z] 16:04:15     INFO -  make[6]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/browser/locales'
[task 2018-09-12T16:04:15.722Z] 16:04:15     INFO -  ../../config/nsinstall -D ../../dist/
[task 2018-09-12T16:04:15.722Z] 16:04:15     INFO -  make[6]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/browser/locales'
[task 2018-09-12T16:04:15.863Z] 16:04:15     INFO -  make[6]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/browser/locales'
[task 2018-09-12T16:04:15.863Z] 16:04:15     INFO -  /builds/worker/workspace/build/src/obj-firefox/_virtualenvs/init/bin/python -m mozbuild.action.langpack_manifest --locales en-US --min-app-ver 64.0a1 --max-app-ver 64.0a1 --app-name "Firefox Nightly" --l10n-basedir "/builds/worker/.mozbuild/l10n-central" --defines /builds/worker/workspace/build/src/toolkit/locales/en-US/defines.inc /builds/worker/workspace/build/src/browser/locales/en-US/defines.inc  --langpack-eid "langpack-en-US@firefox.mozilla.org" --input ../../dist/xpi-stage/locale-en-US
[task 2018-09-12T16:04:15.863Z] 16:04:15     INFO -  make[6]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/browser/locales'
[task 2018-09-12T16:04:16.019Z] 16:04:16     INFO -  make[6]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/browser/locales'
[task 2018-09-12T16:04:16.019Z] 16:04:16     INFO -  /builds/worker/workspace/build/src/obj-firefox/_virtualenvs/init/bin/python -m mozbuild.action.zip -C ../../dist/xpi-stage/locale-en-US -x **/*.manifest -x **/*.js -x **/*.ini /builds/worker/workspace/build/src/obj-firefox/dist/target.langpack.xpi chrome localization browser manifest.json
[task 2018-09-12T16:04:16.019Z] 16:04:16     INFO -  make[6]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/browser/locales'
[task 2018-09-12T16:04:16.019Z] 16:04:16     INFO -  make[4]: Nothing to be done for 'tools'.
[task 2018-09-12T16:04:16.019Z] 16:04:16     INFO -  make[1]: Entering directory '/builds/worker/workspace/build/src/obj-firefox'
[task 2018-09-12T16:04:16.020Z] 16:04:16     INFO -  rm -f jarlog/en-US.log
[task 2018-09-12T16:04:16.020Z] 16:04:16     INFO -  make[1]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox'
[task 2018-09-12T16:04:16.020Z] 16:04:16     INFO -  make[1]: Entering directory '/builds/worker/workspace/build/src/obj-firefox'
[task 2018-09-12T16:04:16.020Z] 16:04:16     INFO -  make[1]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox'
[task 2018-09-12T16:04:16.020Z] 16:04:16     INFO -  make[1]: Entering directory '/builds/worker/workspace/build/src/obj-firefox'
[task 2018-09-12T16:04:16.020Z] 16:04:16     INFO -  make[1]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox'
[task 2018-09-12T16:44:16.047Z] 16:44:16     INFO - Automation Error: mozprocess timed out after 2400 seconds running ['/usr/bin/python2.7', 'mach', '--log-no-times', 'build', '-v']
[task 2018-09-12T16:44:16.077Z] 16:44:16    ERROR - timed out after 2400 seconds of no output
[task 2018-09-12T16:44:16.078Z] 16:44:16    ERROR - Return code: -15
[task 2018-09-12T16:44:16.078Z] 16:44:16  WARNING - setting return code to 2
[task 2018-09-12T16:44:16.078Z] 16:44:16    FATAL - 'mach build -v' did not run successfully. Please check log for errors.
[task 2018-09-12T16:44:16.079Z] 16:44:16    FATAL - Running post_fatal callback...
[task 2018-09-12T16:44:16.079Z] 16:44:16    FATAL - Exiting -1

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 16 • 6 years ago

RyanVM mentions that from the logs, there's definitely reason to suspect that things are breaking during profiling (e.g. quitter.xpi)

	Cristina Coroiu [:ccoroiu]
	Comment 17 • 6 years ago

Also fails on windows: https://treeherder.mozilla.org/logviewer.html#?job_id=198882443&repo=autoland&lineNumber=41349

	Pulsebot
	Comment 18 • 6 years ago

Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/376b4b4ffefb
Move cookieStoreId validator to ext-tabs-base.js r=rpl
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e40be33fb0d
Support cookieStoreId in windows.create extension API r=Gijs,rpl

	Pulsebot
	Comment 19 • 6 years ago

Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53b69b83cfb9
Backed out 2 changesets for build bustage on linux on a CLOSED TREE

	Rob Wu [:robwu] Assignee
	Comment 20 • 6 years ago

This change prevents javascript:-URLs from being passed at the command
line. This restriction was already applied to every URL but the first.
Even the first URL did not result in any visible effect when Firefox is
started. Yet somehow the PGO profiler script managed to rely on it.

This commit ensures that "javascript:" URLs are not activated regardless of
position in the command line, and switches to a data:-URL for the PGO script to
achieve the (previously) desired effect of quitting the browser on startup.

Depends on D4929

	Rob Wu [:robwu] Assignee
	Comment 21 • 6 years ago

I'm going to fix bug 1488914 first because that is a regression that I intend to uplift, and this patch needs to be modified to account for that.

Removing ni - the failure is caused by the use of javascript:Quitter.quit() in the build/pgo/profileserver.py, and the above patch addresses that issue.

	Mike Hommey [:glandium]
	Comment 22 • 6 years ago

Comment on attachment 9009395 [details]
Bug 1393570 - Set allowInheritPrincipal=false by default

Mike Hommey [:glandium] has approved the revision.

	:Gijs (he/him)
	Comment 23 • 6 years ago

Comment on attachment 9009395 [details]
Bug 1393570 - Set allowInheritPrincipal=false by default

:Gijs (he/him) has approved the revision.

	Pulsebot
	Comment 24 • 6 years ago

Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/4ff201d93d71
Move cookieStoreId validator to ext-tabs-base.js r=rpl
https://hg.mozilla.org/integration/autoland/rev/c42695f1399c
Support cookieStoreId in windows.create extension API r=Gijs,rpl
https://hg.mozilla.org/integration/autoland/rev/c37ea7ae71ef
Set allowInheritPrincipal=false by default r=Gijs,glandium

	Noemi Erli[:noemi_erli]
	Comment 25 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4ff201d93d71
https://hg.mozilla.org/mozilla-central/rev/c42695f1399c
https://hg.mozilla.org/mozilla-central/rev/c37ea7ae71ef

	CosminB
	Comment 26 • 6 years ago

I was able to reproduce this issue on Firefox 62.0.2(20180920131237) under Win 7 64-bit and  Mac OS X 10.13.3.

This issue is verified as fixed on Firefox 64.0a1(20180927220034) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached screenshot.

	Chris Mills [:cmills]
	Comment 27 • 6 years ago

Note to content team:

I've added a note to cover this to the Fx 64 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#Changes_for_add-on_developers

	Irene Smith
	Comment 28 • 5 years ago

Added the following paragraph to the create method documentation:

cookieStoreId Optional
    integer. If present, specifies the CookieStoreId for all tabs that will be created when the window is opened.

	Rob Wu [:robwu] Assignee
	Comment 29 • 5 years ago

cookieStoreId is a string, not an integer.

For the basic type and extra permission requirement, see:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/create#Parameters

	Irene Smith
	Comment 30 • 5 years ago

(In reply to Rob Wu [:robwu] from comment #29)
> cookieStoreId is a string, not an integer.
> 
> For the basic type and extra permission requirement, see:
> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/
> tabs/create#Parameters

Thank you for the correction.