Add GV API for cross-origin attributes (to support container tabs)

REOPENED
Assigned to
(NeedInfo from)

Status

enhancement
P1
normal
REOPENED
7 months ago
17 days ago

People

(Reporter: cpeterson, Assigned: esawin, NeedInfo)

Tracking

(Blocks 1 bug, Regressed 1 bug)

unspecified
Unspecified
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 wontfix, firefox68 affected)

Details

(Whiteboard: [geckoview:fenix:p2], )

Attachments

(4 attachments, 1 obsolete attachment)

Reporter

Description

7 months ago
https://support.mozilla.org/en-US/kb/containers

Container tabs are like normal tabs except that the sites you visit will have access to a separate slice of the browser's storage. This means your site preferences, logged in sessions, and advertising tracking data won't carry over to the new container.
This would go in GeckoSessionSettings.
Keywords: good-first-bug
Reporter

Comment 2

7 months ago
[geckoview:fenix] because Fenix will want this feature.
Whiteboard: [geckoview:fenix]

Updated

5 months ago
Product: Firefox for Android → GeckoView
Reporter

Comment 3

4 months ago

[geckoview:fenix:p3] because this feature is not a Fenix MVP release blocker, but it might be interesting for Fenix's PWA story.

Whiteboard: [geckoview:fenix] → [geckoview:fenix:p3]
Reporter

Comment 4

4 months ago

[geckoview:fenix:p2] because Fenix Product Management is talking about using containers to sandbox PWAs.

Priority: P3 → P2
Whiteboard: [geckoview:fenix:p3] → [geckoview:fenix:p2]
Assignee: nobody → esawin
Keywords: good-first-bug

I think there are a few ways we could do this:

  1. Use the userContextId attribute as desktop does, and allow apps to specify an integer value, e.g. GeckoSessionSettings.setContainerId(42).

  2. Same as 1, but restrict to the well-known values we have for containers on desktop. We'd probably want to expose these as constants, so you would use it as GeckoSessionSettings.setContainerId(GeckoSessionSettings.CONTAINER_WORK).

  3. Choose a new hardcoded key (geckoContainerId or similar), and allow the app to set arbitrary values of any type (or just int/string/float), e.g. GeckoSessionSettings.setOriginAttribute("my attribute")

  4. Same as 3 but don't use a hardcoded key, e.g. GeckoSessionSettings.setOriginAttribute("foobar", "my attribute")

  5. Same as 3 but with a Map of (potentially) several keys/values.

If we ever want to support sync of container ids with desktop, we can't do 3. Baku, any opinion on these from the Gecko side of things? Obviously 5 is the most flexible, but I worry about us breaking Gecko with an unexpectedly high number of keys. I'm inclined to believe 4 may be the best compromise.

Flags: needinfo?(amarchesini)
Reporter

Updated

3 months ago
Summary: Add GV API for Container tabs → Add GV API for cross-origin attributes
Reporter

Updated

3 months ago
Priority: P2 → P1
Whiteboard: [geckoview:fenix:p2] → [geckoview:fenix:p1]
Assignee

Comment 7

3 months ago

I've implemented option 3 restricted to the type int.

The API provides a simple interface that is difficult to misuse (performance) at app level while maintaining enough similarity with the userContextId mechanics at Gecko level without directly conflicting with it.
Options 4 and 5 may still be realized through mapping mechanics at app level.

I hope the patch clarifies the intention of API and will move the discussion forward.

Attachment #9042554 - Attachment description: Bug 1501108 - [1.0] Add origin attribute session setting. → Bug 1501108 - [1.1] Add GeckoView Container ID support.

Comment 8

3 months ago

What is the intended use of geckoContainerId? Is it for Containers or Progessive Web Apps? We really have to think about the future where container ids may be synced across devices and design this in a way that is compatible with that.

Also, when you say "allow the app to set arbitrary values of any type", what kind of app do you have in mind? A mobile application that builds on top of Gecko View? Or an extension? Or both?

baku, please weigh in here and review.

Flags: needinfo?(tanvi)
Assignee

Comment 9

3 months ago

(In reply to Tanvi Vyas[:tanvi] from comment #8)

What is the intended use of geckoContainerId? Is it for Containers or Progessive Web Apps? We really have to think about the future where container ids may be synced across devices and design this in a way that is compatible with that.

We want to provide a GeckoView API for cookie jar separation mechanics. It should provide enough flexibility to allow the implementation of containers, isolated PWAs and more.
Syncing ids across devices is an app-level mechanic and would be difficult to account for at the GeckoView level.

Also, when you say "allow the app to set arbitrary values of any type", what kind of app do you have in mind? A mobile application that builds on top of Gecko View? Or an extension? Or both?

We currently target Fenix and other internal apps, but third-party apps may find different use cases for this API.
This is not an API for extensions.

Flags: needinfo?(tanvi)
Attachment #9042554 - Attachment description: Bug 1501108 - [1.1] Add GeckoView Container ID support. → Bug 1501108 - [1.2] Add GeckoView Session Context ID support.
Assignee

Comment 10

3 months ago

Depends on D19182

Reporter

Updated

3 months ago
Summary: Add GV API for cross-origin attributes → Add GV API for cross-origin attributes (to support container tabs)
Reporter

Comment 11

3 months ago

Fenix plans to use containers in M4, so GV needs to provide the API in M3.

Whiteboard: [geckoview:fenix:p1] → [geckoview:fenix:m3]
Assignee

Comment 13

3 months ago

Baku, I've replied to your questions on the patch, do you need any other information for the review?

I've added you as a reviewer to the test patch, maybe you also have an idea why replacing the HTML page with evaluateJS gives us a security error (for the same JS code).

Attachment #9044311 - Attachment description: Bug 1501108 - [2.0] Add Session Context ID test. → Bug 1501108 - [2.2] Add Session Context ID test.
Assignee

Updated

2 months ago
Depends on: 1537882
Reporter

Comment 15

2 months ago

Lower priority from M3 to P2 "nice-to-have" because Fenix has deferred the Container feature to post-MVP.

Whiteboard: [geckoview:fenix:m3] → [geckoview:fenix:p2]
Attachment #9051404 - Attachment description: Bug 1501108 - [3.0] Add a GeckoRuntime API to delete session context data. → Bug 1501108 - [3.1] Add a GeckoRuntime API to delete session context data.
Assignee

Comment 16

2 months ago

I've added a GV workaround for bug 1537882 in patch 3.1 so that we don't necessarily need to block on that issue.

Attachment #9051404 - Attachment description: Bug 1501108 - [3.1] Add a GeckoRuntime API to delete session context data. → Bug 1501108 - [3.2] Add a StorageController API to delete session context data.
Assignee

Updated

2 months ago
Blocks: 1539429
Attachment #9051404 - Attachment description: Bug 1501108 - [3.2] Add a StorageController API to delete session context data. → Bug 1501108 - [3.3] Add a StorageController API to delete session context data.
Attachment #9051404 - Attachment description: Bug 1501108 - [3.3] Add a StorageController API to delete session context data. → Bug 1501108 - [3.4] Add a StorageController API to delete session context data.

Comment 17

a month ago
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cad2b29b79d2
[1.2] Add GeckoView Session Context ID support. r=snorp,baku,mayhemer
https://hg.mozilla.org/integration/autoland/rev/de36c9fb8c65
[2.2] Add Session Context ID test. r=snorp,baku
https://hg.mozilla.org/integration/autoland/rev/1936dde5f34c
[3.4] Add a StorageController API to delete session context data. r=baku,snorp,geckoview-reviewers

Comment 18

a month ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da28ce3af312
Removed extra spaces in order to fix eslint failure.
Assignee

Comment 19

a month ago
Posted file Bug 1501108 - Fix lint error. (obsolete) —

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=1936dde5f34c98c7eaca6d53d9826e3b40f1134c&selectedJob=240465304

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=240465304&repo=autoland&lineNumber=1775

Backout link: https://hg.mozilla.org/integration/autoland/rev/095d253f97be7112a6f0ffa0b9d892964f5264d7

[task 2019-04-15T21:47:38.562Z] 21:47:38 INFO - TEST-PASS | browser/base/content/test/caps/browser_principalSerialization_version1.js | Ensure value of firstPartyDomain is -
[task 2019-04-15T21:47:38.565Z] 21:47:38 INFO - Buffered messages finished
[task 2019-04-15T21:47:38.566Z] 21:47:38 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/caps/browser_principalSerialization_version1.js | Ensure value of geckoViewSessionContextId is undefined - Got , expected undefined
[task 2019-04-15T21:47:38.567Z] 21:47:38 INFO - Stack trace:
[task 2019-04-15T21:47:38.568Z] 21:47:38 INFO - chrome://mochikit/content/browser-test.js:test_is:1325
[task 2019-04-15T21:47:38.569Z] 21:47:38 INFO - chrome://mochitests/content/browser/browser/base/content/test/caps/browser_principalSerialization_version1.js:test_realHistoryCheck:202
[task 2019-04-15T21:47:38.570Z] 21:47:38 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1116
[task 2019-04-15T21:47:38.571Z] 21:47:38 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1144
[task 2019-04-15T21:47:38.572Z] 21:47:38 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1005
[task 2019-04-15T21:47:38.573Z] 21:47:38 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
[task 2019-04-15T21:47:38.575Z] 21:47:38 INFO - TEST-PASS | browser/base/content/test/caps/browser_principalSerialization_version1.js | Ensure value of inIsolatedMozBrowser is false -
[task 2019-04-15T21:47:38.576Z] 21:47:38 INFO - TEST-PASS | browser/base/content/test/caps/browser_principalSerialization_version1.js | Ensure value of privateBrowsingId is 0 -
[task 2019-04-15T21:47:38.577Z] 21:47:38 INFO - TEST-PASS | browser/base/content/test/caps/browser_principalSerialization_version1.js | Ensure value of userContextId is 0 -
[task 2019-04-15T21:47:38.579Z] 21:47:38 INFO - TEST-PASS | browser/base/content/test/caps/browser_principalSerialization_version1.js | Should have not have a URI for system -
[task 2019-04-15T21:47:38.582Z] 21:47:38 INFO - TEST-PASS | browser/base/content/test/caps/browser_principalSerialization_version1.js | should have CSP -
[task 2019-04-15T21:47:38.583Z] 21:47:38 INFO - TEST-PASS | browser/base/content/test/caps/browser_principalSerialization_version1.js | Ensure value of appId is 0 -
[task 2019-04-15T21:47:38.584Z] 21:47:38 INFO - TEST-PASS | browser/base/content/test/caps/browser_principalSerialization_version1.js | Ensure value of firstPartyDomain is -
[task 2019-04-15T21:47:38.585Z] 21:47:38 INFO - Not taking screenshot here: see the one that was previously logged

Flags: needinfo?(esawin)
Assignee

Updated

a month ago
Flags: needinfo?(esawin)
Attachment #9058419 - Attachment is obsolete: true
Attachment #9042554 - Attachment description: Bug 1501108 - [1.2] Add GeckoView Session Context ID support. → Bug 1501108 - [1.3] Add GeckoView Session Context ID support.
Attachment #9044311 - Attachment description: Bug 1501108 - [2.2] Add Session Context ID test. → Bug 1501108 - [2.3] Add Session Context ID test.

Comment 22

a month ago
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23a77e063257
[1.3] Add GeckoView Session Context ID support. r=snorp,baku,mayhemer
https://hg.mozilla.org/integration/autoland/rev/335cec0aacd8
[2.3] Add Session Context ID test. r=snorp,baku
https://hg.mozilla.org/integration/autoland/rev/99ba286125d1
[3.4] Add a StorageController API to delete session context data. r=baku,snorp,geckoview-reviewers
https://hg.mozilla.org/integration/autoland/rev/8f2d511ad49f
[4.0] Add empty origin attribute values to fix desktop tests. r=baku
Regressions: 1515057

Backed out for causing very frequent leaks (2000 last week on sheriffed trees) in macOS debug wpt tests - see bug 1515057:

https://hg.mozilla.org/integration/autoland/rev/3eb938b576a99000bff84b6ec314f938959e8e88

Status: RESOLVED → REOPENED
Flags: needinfo?(esawin)
Resolution: FIXED → ---
Target Milestone: mozilla68 → ---

Comment 25

21 days ago
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/2e58f3159dfb
Backed out 4 changesets for causing very frequent leaks in macOS debug wpt tests: Set API key provided by api-lint tasks. a=backout

:esawin could I ask why we are using a String here instead of an int like userContextId?

  • If we intend to use it as a domain storage it probably should be named as such.
  • If we intend to have it used for userContextId and a PWA domain it perhaps should be two fields?
  • If it's intended to be an 'app' key then perhaps have it named geckoViewAppKey?

Thanks!

If you have no idea what is going wrong here, you can set XPCOM_MEM_BLOAT_LOG=1 and XPCOM_MEM_LOG_CLASSES=nsStringBuffer. This will make the browser print out what the contents of the nsStringBuffer actually is. Other than that, I don't have many ideas. It feels like a sort of thing where we're spinning up some kind of networking runnable after shutdown has started, so we don't end up running it and just end up leaking it.

Assignee

Comment 28

18 days ago

(In reply to Jonathan Kingston [:jkt] from comment #26)

:esawin could I ask why we are using a String here instead of an int like userContextId?

This has been thoroughly discussed in this bug and during the patch review here: https://phabricator.services.mozilla.com/D19182.

  • If we intend to use it as a domain storage it probably should be named as such.
  • If we intend to have it used for userContextId and a PWA domain it perhaps should be two fields?
  • If it's intended to be an 'app' key then perhaps have it named geckoViewAppKey?

We don't restrict the nature of the mapping, this is up to the app developer.
This is a generic API exposing per-session cookie-jar separation mechanics.
It's not intended to be an app key, it's a context ID for a given GeckoSession, hence the name.

Flags: needinfo?(esawin)
Assignee

Comment 29

18 days ago

(In reply to Andrew McCreight [:mccr8] from comment #27)

If you have no idea what is going wrong here, you can set XPCOM_MEM_BLOAT_LOG=1 and XPCOM_MEM_LOG_CLASSES=nsStringBuffer. This will make the browser print out what the contents of the nsStringBuffer actually is. Other than that, I don't have many ideas. It feels like a sort of thing where we're spinning up some kind of networking runnable after shutdown has started, so we don't end up running it and just end up leaking it.

Will this work in automation? The leak is only occurring in our automation tests on MacOS and I don't have a Mac.

(In reply to Eugen Sawin [:esawin] from comment #29)

Will this work in automation? The leak is only occurring in our automation tests on MacOS and I don't have a Mac.

Yes, it should. It looks like you can set environment values for WPTs in here:
https://searchfox.org/mozilla-central/rev/b59a99943de4dd314bae4e44ab43ce7687ccbbec/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py#294

You'll want to leave the existing XPCOM_MEM_BLOAT_LOG part alone, and add in something like
env["XPCOM_MEM_LOG_CLASSES"] = "nsStringBuffer"

It will slow down the browser a lot, but hopefully not enough to interfere with reproducing the test.

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