1238183 - ForgetAboutSite needs to forget about a site for all user context ids, not just usercontextid 0

Status: RESOLVED FIXED

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

Description

[Dave Huseby [:huseby]]

This one is easy.  This code is designed to clear out any stored data about a site.  We need to change the calls to createCodebasePrincipal so that it passes in the user context id origin attribute so that we clear from the correct caches.

the consequences of this are that it will only forget the site in current context.

Comment 1

[Dave Huseby [:huseby]]

Tanvi,

Do you think that when we forget about a site that we should clear it out of all user contexts?  I'm not sure about that.

Comment 2

[Tanvi Vyas[:tanvi]]

Is this code invoked when a user finds the forget about this site button from the hamburger menu?  How often is that button clicked?  Is there telemetry around that?  If you can't tell easily from the code, you maybe to ask Bram, Francois or someone from the user research team.

I imagine it is not clicked often.  And my guess is that the user wants to clear any trace of being at the site so I would clear this for all contexts.  But needinfo'ing Bram for his thoughts on this as well.

Comment 3

[Bram Pitoyo [:bram]]

Since the forget button erases only the last 5 minutes, 2 hours or 24 hours of recorded data, then it should erase data from all contexts.

However, any time we give user the power to erase data since “the beginning of time”, we should respect context (or maybe provide an option: want to clear in all contexts, or just the context you’re currently in?).

This is so that erasing site data doesn’t accidentally erase the thing that containers is designed to keep: site login state.

That’s my hypothesis, at least. I don’t have first hand data to draw any definite decision. We should ask Ilana or Gregg about how often people uses the Forget feature, and what time range is picked the most.

Ilana or Gregg, do you have this information, or do you know of somebody who might?

Comment 4

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

(Ilana might have actual button usage stuff, but I doubt it.  For *user model* I am punting to Bill Selman, to answer based on user research.  

I somehow suspect users will expect it to do the Right Thing, which is a magical thought.  

Claim 1: overkill here is probably the safest.  
Claim 2: as implemented, the button is a low usage (both in % of users, and times per those user) feature.  

I don't have good evidence for those claims :)

Comment 5

[Ilana]

Actually, we do have that data!
Here's a report we did back in early July before the instrumentation was removed: 
https://docs.google.com/document/d/1KHCip_9WbtaPeTSdPLUVicCjGPvBNtdvnaea04fQBg8

Because this is closer to when the button was released, I would natively assume that usage is lower, if anything.

Bottom line: < 1% usage. "Day" is most common choice.

Comment 6

[Bram Pitoyo [:bram]]

According to the usage document, we get a figure of slightly less than 0.1% usage on the release channel. With so low of a number, let’s make a decision that we should erase from all contexts, like what Tanvi has proposed on comment 2, and Gregg on comment 4.

Dave, what do you think?

Comment 7

[Tanvi Vyas[:tanvi]]

One thought...
what if a user accidentally logs into "facebook" in their Work context.  They don't facebook to track them across their Work browsing habits.  So they log out of facebook in their Work context, but just to be sure, they also ask the browser to "Forget about this site".  In that case, they want the browser to "Forget about this site in the Work context", but we will forget everywhere.

So I think for v1 (and this bug) we should definitely clear facebook.com from all contexts.  But in a future version of contextual identity, we could offer advanced options to the forget button that allows the user what context(s) they want to the site forgotten about in.  It would default to "all" and the user could choose a different context.  Dave, can you file a followup for that and have it block the meta bug - https://bugzilla.mozilla.org/show_bug.cgi?id=1191418

Thanks!

Comment 8

[Bill Selman]

We did conduct a study before the Forget button launched at the end of 2014. Unfortunately, there was never a follow-up development on the feature to address the insights we uncovers.

The most interesting finding from the study was that the duration model was less meaningful to participants. Computers are great at keeping track of durations, humans are not. More relevant to the participants were contexts. For example, "Forget the last site I visited" which is what you are proposing. Also, "forgot every site I visited since I opened Firefox"–useful for public computers which came up frequently. Finally, "forget every site I visited today" or "since the beginning of time."

I'm not hugely surprised by the usage numbers, but I would attribute the usage numbers to low discoverability/poor awareness and not meeting user needs (duration based forgetfulness).

Ping me if you want me to share our report.

Comment 9

[Bram Pitoyo [:bram]]

Tanvi, your proposal for contextual forget and what Bill had found in his research does seem to align. I come to it from the angle of “user wants some data to be persistent, even as every other data gets erased during the Forget process”, and it happens to align, too.

In future versions of the forget button, erasure from any combination of contexts should be done if possible. Below is a very rough mockup of what the string could look like.

Erase data:
[x] Everywhere
[ ] Selected containers
  [ ] Default
  [ ] Home
  [ ] Work
  [ ] …

Comment 10

[Dave Huseby [:huseby]]

Attachment: Bug_1238183.patch

Comment 11

[Tanvi Vyas[:tanvi]]

(In reply to Dave Huseby [:huseby] from comment #10)
> Created attachment 8717701 [details] [diff] [review]
> Bug_1238183.patch

Dave, does this patch clear the site for all contexts?

Comment 12

[Dave Huseby [:huseby]]

Comment on attachment 8717701 [details] [diff] [review]
Bug_1238183.patch

this one is good to go.

Comment 13

[Dave Huseby [:huseby]]

Comment on attachment 8717701 [details] [diff] [review]
Bug_1238183.patch

this one is good to go.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8717701 [details] [diff] [review]
Bug_1238183.patch

Review of attachment 8717701 [details] [diff] [review]:
-----------------------------------------------------------------

This one is more complex, we should delete data for all cookie jars. And that applies to things like cookies and http cache too in this code.

Comment 15

[Tanvi Vyas[:tanvi]]

This is going to be pretty complex.  To make sure we forget all data storage types for all user contexts - https://mxr.mozilla.org/mozilla-central/source/toolkit/forgetaboutsite/ForgetAboutSite.jsm

We may have this same situation for the Forget Button.  Where we want to forget all storage mechanism for all sites in the last X amount of time.

Comment 16

[Dave Huseby [:huseby]]

This is more complicated than first thought.  I'm removing myself from the asignee so that Yoshi, Tim, or Ethan can take a look.

Yoshi, you might be the best person to look at this.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

So the original code only removes data-jars with DEFAULT_USER_CONTEXT_ID.
I think I will send a clear-origin-data notification with {} in the data field,
and {} means 'CLEAR ALL' operation.
(Previous in FirefoxOS, the data wil be {appId: x}, and it will delete all data-jars with appId is x).

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #17)
> So the original code only removes data-jars with DEFAULT_USER_CONTEXT_ID.
> I think I will send a clear-origin-data notification with {} in the data
> field,
> and {} means 'CLEAR ALL' operation.
> (Previous in FirefoxOS, the data wil be {appId: x}, and it will delete all
> data-jars with appId is x).

Or if that bug still needs a while, I'll add another interface in nsIQuotaManagerService to remove entries by origin attributes.

Comment 19

[Tanvi Vyas[:tanvi]]

This bug is for "Forget About Site".  It forgets about a specific domain in all user contexts.

This differs from bug 1250983.

Comment 20

[Andrea Marchesini [:baku]]

I suggest to use ContextualIdentityService here.

Comment 21

[Tanvi Vyas[:tanvi]]

Updating the title to reflect that we need to forget a domain in all user contexts.  So if I say forget about mozilla.org, I need to delete data from mozilla.org for

site: mozilla.org, usercontextid: 0
site: mozilla.org, usercontextid: 1
site: mozilla.org, usercontextid: 2
site: mozilla.org, usercontextid: 3
site: mozilla.org, usercontextid: 4

Comment 22

[Ethan Tseng [:ethan]]

Tim and I had an offline discussion about this bug. We went through ForgetAboutSite.jsm and checked the required actions of every item.

userContextId-aware Data   Actions
------------------------   --------------------------------------------------------------------------
- Cache                    Need to test and ensure the current impl. clears all user contexts
- Image Cache              Need to test and ensure the current impl. clears all user contexts
- Cookies                  Need to change the codes to remove all user contexts
- Permissions              The current code should be correct. Need to add test case for verification
- Offline Storages         Need to change the codes. Plan to use ContextualIdentityService

Non-userContextId-aware    Actions
------------------------   --------------------------------------------------------------------------
- Passwords                No change


For the rest data items, we are not 100% sure if they are userContextId-aware or not.
We guess most of them are not, which means we don't have to make any change for them.
- EME
- Plugin Data
- Downloads
- Content Preferences
- Predictive Network Data
- Push Notifications

Tanvi and Baku,
Can you help to verify our thoughts?

Comment 23

[Tanvi Vyas[:tanvi]]

(In reply to Ethan Tseng [:ethan] from comment #22)
> Tim and I had an offline discussion about this bug. We went through
> ForgetAboutSite.jsm and checked the required actions of every item.
> 
> userContextId-aware Data   Actions
> ------------------------  
> --------------------------------------------------------------------------
> - Cache                    Need to test and ensure the current impl. clears
> all user contexts
> - Image Cache              Need to test and ensure the current impl. clears
> all user contexts
> - Cookies                  Need to change the codes to remove all user
> contexts
> - Permissions              The current code should be correct. Need to add
> test case for verification
Permissions aren't segregated by usercontextid, so you shouldn't' have to do anything here.

> - Offline Storages         Need to change the codes. Plan to use
> ContextualIdentityService
> 
> Non-userContextId-aware    Actions
> ------------------------  
> --------------------------------------------------------------------------
> - Passwords                No change
> 
> 
> For the rest data items, we are not 100% sure if they are
> userContextId-aware or not.
> We guess most of them are not, which means we don't have to make any change
> for them.
> - EME
Unsure?
> - Plugin Data
Not segregated
> - Downloads
Not segregated
> - Content Preferences
What type of preferences?  I don't think these are segregated
> - Predictive Network Data
Unsure?
> - Push Notifications
Shouldn't be segregated, as its a permission.
> 
> Tanvi and Baku,
> Can you help to verify our thoughts?

Leaving Baku's needinfo in case he has more info.

Comment 24

[Andrea Marchesini [:baku]]

Nothing to add. Sounds a good plan, with the tanvi's comments.

Comment 25

[Tim Huang[:timhuang]]

Attachment: Part 1 - Make the ForgetAboutSite handles userContextId correctly.

Comment 26

[Tim Huang[:timhuang]]

Attachment: Part 2 - Add a test case for testing the ForgetAboutSite with userContextId.

Comment 27

[Tim Huang[:timhuang]]

Hi Tanvi,

It concerns me that this bug would be blocked for a long time because the image cache relies on the Bug 1274516 to act correctly on clearing image cache, and when will Bug 1274516 be finished is out of our control. So I am thinking that we could disable the test of clearing the image cache in this bug first. After the Bug 1274516 landed, we could enable the test of the image cache.

How do you think?

Comment 28

[Ethan Tseng [:ethan]]

(In reply to Tim Huang[:timhuang] from comment #27)

According to the Containers meeting today, we agreed to enable all test cases except for the ones of image cache.  Tanvi suggested us to use "todo()" statements of Mochitest to pass image cache test cases temporarily.

Comment 29

[Tim Huang[:timhuang]]

Attachment: Part 2 - Add a test case for testing the ForgetAboutSite with userContextId.

Make the image cache test as todo.

Comment 30

[Andrea Marchesini [:baku]]

Sounds good. Let me know if ContextualIdentityService is OK as it is or you need it as nsI<Something>

Comment 31

[Tim Huang[:timhuang]]

Attachment: Part 2 - Add a test case for testing the ForgetAboutSite with userContextId.

Fix some nits.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8755336 [details] [diff] [review]
Part 1 - Make the ForgetAboutSite handles userContextId correctly.

Review of attachment 8755336 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, but I'm not a peer of this code so I can't r+

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8758585 [details] [diff] [review]
Part 2 - Add a test case for testing the ForgetAboutSite with userContextId.

Review of attachment 8758585 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know this code well enough to review.

Comment 34

[Tim Huang[:timhuang]]

Comment on attachment 8755336 [details] [diff] [review]
Part 1 - Make the ForgetAboutSite handles userContextId correctly.

Mak, could you help on reviewing this?

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, i realized one problem with this.

We probably don't want to do this just for all userContextIds. We want to do this for all cookie jars.

So for example once the tor browser uses OAs to isolate based on extra domain names. I.e. tor will create a "jar" for each domain that the user visits, which means that it's not something that can be enumerated.

We originally created the OriginAttributesPatternDictionary [1][2] dictionary for this purpose. The idea was that various storage APIs, like Cookies and QuotaManager, would have functions for clearing data which took a OriginAttributesPatternDictionary rather than an OriginAttributes. That way it would be possible to delete data from multiple cookie jars at once.

You should check with bholley and Dave Huseby about if the current approach is fine for now, and that we add functions that take OriginAttributesPatternDictionary later, or if we should do the right solution right away.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/webidl/ChromeUtils.webidl#84
[2] http://mxr.mozilla.org/mozilla-central/source/dom/webidl/ChromeUtils.webidl#29

Comment 36

[Tanvi Vyas[:tanvi]]

Tim, please look into comment 35 and see how difficult it would be to make this patch generic to all OriginAttributes.  Since we have a bit of a deadline for containers, if making the patch more generic means that it will take longer than a week, then we can file a followup bug for that.  Bobby is on PTO so won't be able to provide his feedback on comment 35.  needinfo'ing Dave to look at it as well.

Comment 37

[Dave Huseby [:huseby]]

Jonas is right.  That is exactly what we had in mind originally.  My original patch was overly simplistic and wrong.  With the OriginAttributesPatternDictionary we can implement clearing of Tor's per-domain buckets.  This is definitely the way we want to go.

Comment 38

[Tim Huang[:timhuang]]

I think we should file a follow-up bug because the QuotaManager needs a new API for doing this job, and this may take longer than a week. Actually, there will be a way to clear data-jars which applys the OriginAttributesPattern after bug 1195930 landed. However, this will clear all data-jars match with the given pattern, but here we only want to clear data-jars related to the given host. So we have to open a new API for such cases.

For the cookies, we could use the nsICookieManager2.getCookiesWithOriginAttributes() to get all cookie-jars of the given pattern. Then, we can remove cookies accordingly by referencing the host field of cookies.

So, I think I could fix the cookie part here. But leave the QuotaManager to other bug. How do you think Tanvi?

Comment 39

[Tanvi Vyas[:tanvi]]

(In reply to Tim Huang[:timhuang] from comment #38)
> I think we should file a follow-up bug because the QuotaManager needs a new
> API for doing this job, and this may take longer than a week. Actually,
> there will be a way to clear data-jars which applys the
> OriginAttributesPattern after bug 1195930 landed. However, this will clear
> all data-jars match with the given pattern, but here we only want to clear
> data-jars related to the given host. So we have to open a new API for such
> cases.
> 
> For the cookies, we could use the
> nsICookieManager2.getCookiesWithOriginAttributes() to get all cookie-jars of
> the given pattern. Then, we can remove cookies accordingly by referencing
> the host field of cookies.
> 
> So, I think I could fix the cookie part here. But leave the QuotaManager to
> other bug. How do you think Tanvi?

Do you mean fix cookies for usercontextId only in this bug?  And fix QuotaManager for userContextId in a spearete bug?  Or do you mean fix cookies for OriginAttributes in general in this bug, and fix QuotaManager for OriginAttributes in general in another bug?

I would like to get ForgetAboutSite to work for usercontextID now, and file a followup to get it to work for all OriginAttributes.  This followup can happen during the Nightly 50 cycle and after Bug 1195930 for QuotaManager lands.

Comment 40

[Tim Huang[:timhuang]]

I mean fix cookies for OriginAttributes in general in this bug, but leave the QuotaManger to another bug. However, If this bug is targeting the usercontextId only, I think we should follow what you said, And open a follow-up bug to fix them in general.

Comment 41

[Tim Huang[:timhuang]]

The Bug 1278037 has been filed as a follow-up bug.

Comment 42

[Tim Huang[:timhuang]]

Attachment: Part 2 - Add a test case for testing the ForgetAboutSite with userContextId.

Fix the problem that the test will timeout in non-e10s mode.

Comment 43

[Tim Huang[:timhuang]]

Comment on attachment 8755336 [details] [diff] [review]
Part 1 - Make the ForgetAboutSite handles userContextId correctly.

Jimm, could you help on reviewing this?

Comment 44

[Jim Mathies [:jimm]]

Comment on attachment 8760063 [details] [diff] [review]
Part 2 - Add a test case for testing the ForgetAboutSite with userContextId.

Generally this looks good. I don't usually do reviews involving storage code or context ids though. Jonas gave a f+ so hopefully that should cover any shortcomings.

Comment 45

[Tim Huang[:timhuang]]

Attachment: Part 1 - Make the ForgetAboutSite handles userContextId correctly. r=jimm, sicking

Rebase to m-c and update the commit message.

Comment 46

[Tim Huang[:timhuang]]

Attachment: Part 2 - Add a test case for testing the ForgetAboutSite with userContextId.

Rebase to m-c, and Update the commit message.

Comment 47

[Tim Huang[:timhuang]]

Attachment: Part 2 - Add a test case for testing the ForgetAboutSite with userContextId.

Make all tests of the image cache as todo tests. After the bug 1270680 landed, we can re-enable them.

Comment 48

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33651c181c96

Comment 49

[Tim Huang[:timhuang]]

Attachment: Part 2 - Add a test case for testing the ForgetAboutSite with userContextId.

Fixing the xpcshell test failure problem.

Comment 50

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4cce5f4db75

Comment 51

[Tim Huang[:timhuang]]

Attachment: Part 2 - Add a test case for testing the ForgetAboutSite with userContextId.

Fixing another xpcshell test failure.

Comment 52

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7173481ef79b

Comment 53

[Tim Huang[:timhuang]]

Try looks good.

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/663a083774f4
Part 1 - Make the ForgetAboutSite handles userContextId correctly. r=jimm, r=sicking
https://hg.mozilla.org/integration/mozilla-inbound/rev/1207df32d737
Part 2 - Add a test case for testing the ForgetAboutSite with userContextId. r=jimm

Comment 55

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bd4f86754707 - try only looks good if you don't look at that linux64 debug bc2 with the "browser_forgetaboutsite.js | leaked 2 window(s) until shutdown [url = http://example.com/browser/browser/components/contextualidentity/test/browser/file_set_storages.html?work]" failure.

Comment 56

[Phil Ringnalda (:philor)]

Also https://treeherder.mozilla.org/logviewer.html#?job_id=31164863&repo=mozilla-inbound

Comment 57

[Phil Ringnalda (:philor)]

Also https://treeherder.mozilla.org/logviewer.html#?job_id=31159682&repo=mozilla-inbound

Comment 58

[Phil Ringnalda (:philor)]

Also https://treeherder.mozilla.org/logviewer.html#?job_id=31158097&repo=mozilla-inbound

Comment 59

[Tim Huang[:timhuang]]

Attachment: Part 2 - Add a test case for testing the ForgetAboutSite with userContextId.

Fixing the mochitest failure. Jimm, could you take a look? I will attach an interdiff to help you on review.

Comment 60

[Tim Huang[:timhuang]]

Attachment: interdiff-part2.patch

The interdiff of the part 2.

Comment 61

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7340a86d1229

Comment 62

[Tim Huang[:timhuang]]

Attachment: Part 2 - Add a test case for testing the ForgetAboutSite with userContextId.

Fixing the mochitest test failure of Windows 7.

Comment 63

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=268f4e11cab8

Comment 64

[Tim Huang[:timhuang]]

Attachment: Part 2 - Add a test case for testing the ForgetAboutSite with userContextId.

Fixing mochitest failures for windows 7 debug build.

Comment 65

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18a855a5aa9f

Comment 66

[Tim Huang[:timhuang]]

Try looks good. Please ignore the interdiff-part2.patch.

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfbbd0e32a5f
Part 1 - Make the ForgetAboutSite handles userContextId correctly. r=jimm, sicking
https://hg.mozilla.org/integration/mozilla-inbound/rev/b86836730296
Part 2 - Add a test case for testing the ForgetAboutSite with userContextId. r=jimm

Comment 68

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dfbbd0e32a5f
https://hg.mozilla.org/mozilla-central/rev/b86836730296

Comment 69

[aleth [:aleth]]

The ContextualIdentityService currently lives in browser/, which means toolkit files should not depend on it. It breaks apps which don't ship browser/ (spotted by the linked c-c test failure).

I'm not sure what the right solution is here - should the ContextualIdentityService be moved to toolkit/?

Comment 70

[Tim Huang[:timhuang]]

(In reply to aleth [:aleth] from comment #69)
> The ContextualIdentityService currently lives in browser/, which means
> toolkit files should not depend on it. It breaks apps which don't ship
> browser/ (spotted by the linked c-c test failure).
> 
> I'm not sure what the right solution is here - should the
> ContextualIdentityService be moved to toolkit/?

Baku, do you think we should move the ContextualIdentityService into toolkit/? Or should we find a way to make this right?

Comment 71

[Jonathan Hao (inactive) [:jhao]]

Bug 1279568 might have this problem, too. We use ContextualIdentityService in background page thumbnail which also lives in toolkit/

Comment 72

[Andrea Marchesini [:baku]]

> Baku, do you think we should move the ContextualIdentityService into
> toolkit/? Or should we find a way to make this right?

Yes. Let's move it. But not the tests and not the css. toolkit/contextualidentity ?

Comment 73

[Jorg K (CEST = GMT+2)]

Tim, do you have time to move ContextualIdentityService into toolkit/contextualidentity?

Comment 74

[Tim Huang[:timhuang]]

Yes, there is a bug for doing this, Bug 1285889.

Comment 75

[Jorg K (CEST = GMT+2)]

Thanks, I didn't see it referenced here, so I asked ;-)