Consider windows.create cookieStoreId property

VERIFIED FIXED in Firefox 64

Status

enhancement
P5
normal
VERIFIED FIXED
2 years ago
3 months ago

People

(Reporter: jkt, Assigned: robwu)

Tracking

({dev-doc-complete})

unspecified
mozilla64
Dependency tree / graph

Firefox Tracking Flags

(firefox64 verified)

Details

(Whiteboard: [design-decision-approved])

Attachments

(4 attachments)

(Reporter)

Description

2 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.
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`
(Reporter)

Comment 2

2 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

Updated

2 years ago
Priority: -- → P5
Whiteboard: [design-decision-needed]

Comment 3

a year 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.
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
Severity: normal → enhancement
Flags: needinfo?(lgreco)
Whiteboard: [design-decision-needed] → [design-decision-approved]
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: "...}],
      ...
    });
Flags: needinfo?(lgreco)

Updated

10 months ago
Product: Toolkit → WebExtensions
(Assignee)

Comment 6

8 months 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
Flags: needinfo?(lgreco)
(Assignee)

Updated

8 months ago
See Also: → 1485961
(Assignee)

Updated

8 months ago
Assignee: nobody → rob
Status: NEW → ASSIGNED

Comment 7

8 months 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.
Flags: needinfo?(lgreco)
(Assignee)

Comment 8

8 months 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.
(Assignee)

Updated

8 months ago
Blocks: 1415333
(Assignee)

Updated

8 months ago
No longer blocks: 1415333
Depends on: 1415333
(Assignee)

Comment 9

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

Depends on D4920
(Assignee)

Comment 10

8 months 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

Comment 11

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

:Gijs (he/him) has approved the revision.
Attachment #9006219 - Flags: review+

Comment 12

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

Luca Greco [:rpl] has approved the revision.
Attachment #9006218 - Flags: review+

Comment 13

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

Luca Greco [:rpl] has approved the revision.
Attachment #9006219 - Flags: review+

Comment 14

7 months 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
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
Flags: needinfo?(rob)
RyanVM mentions that from the logs, there's definitely reason to suspect that things are breaking during profiling (e.g. quitter.xpi)

Comment 18

7 months 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

Comment 19

7 months 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
(Assignee)

Comment 20

7 months 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
(Assignee)

Comment 21

7 months 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.
Depends on: 1488914
Flags: needinfo?(rob)
Comment on attachment 9009395 [details]
Bug 1393570 - Set allowInheritPrincipal=false by default

Mike Hommey [:glandium] has approved the revision.
Attachment #9009395 - Flags: review+

Comment 23

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

:Gijs (he/him) has approved the revision.
Attachment #9009395 - Flags: review+

Comment 24

7 months 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

Comment 25

7 months 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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64

Comment 26

7 months ago
Posted image Bug1393570.png
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.

Updated

7 months ago
Status: RESOLVED → VERIFIED

Updated

6 months ago
Blocks: 1488289
(Assignee)

Updated

6 months ago
Depends on: 1501006

Comment 28

4 months 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.
Flags: needinfo?(rob)
(Assignee)

Comment 29

4 months 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
Flags: needinfo?(rob)

Comment 30

4 months 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.

Comment 31

3 months ago

I updated the documentation to list cookieStoreId as a string, and created the Pull request for the BCD data.

You need to log in before you can comment on or make changes to this bug.