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.

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.

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.

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?

(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 :)

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.

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?

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!

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.

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 [ ] …

(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 on attachment 8717701 [details] [diff] [review] Bug_1238183.patch this one is good to go.

Comment on attachment 8717701 [details] [diff] [review] Bug_1238183.patch this one is good to go.

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.

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.

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.

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).

(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.

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

I suggest to use ContextualIdentityService here.

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

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?

(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.

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

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?

(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.

Make the image cache test as todo.

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

Fix some nits.

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 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 on attachment 8755336 [details] [diff] [review] Part 1 - Make the ForgetAboutSite handles userContextId correctly. Mak, could you help on reviewing this?

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

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.

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.

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?

(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.

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.

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

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

Comment on attachment 8755336 [details] [diff] [review] Part 1 - Make the ForgetAboutSite handles userContextId correctly. Jimm, could you help on reviewing this?

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.

Rebase to m-c and update the commit message.

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

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

Fixing the xpcshell test failure problem.

Fixing another xpcshell test failure.

Try looks good.

Pushed by cbook@mozilla.com: 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

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.

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

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

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

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

The interdiff of the part 2.

Fixing the mochitest test failure of Windows 7.

Fixing mochitest failures for windows 7 debug build.

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

Pushed by cbook@mozilla.com: 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

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

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/?

(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?

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

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

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

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

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