Closed Bug 1278037 Opened 7 years ago Closed 7 years ago

Make the ForgetAboutSite to forget a site not only for all userContextIds, but also for all originAttributes in general.

Categories

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

Product:
Core
Component:
DOM: Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: timhuang, Assigned: timhuang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [OA][domsecurity-active])

Attachments

(6 files, 10 obsolete files)

Part 1 : Modify the getCookiesWithOriginAttributes of the nsICookieManager2 to take the host as an optional argument.
7.88 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
Part 4 : Modify the mozIGeckoMediaPluginChromeService.forgetThisSite() to take the originAttributes pattern as an argument.
11.87 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
Part 2 : Add a new type, the Prefix, of OriginScope for the Quota Manager.
7.16 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
Part 3 : Modify the nsIQuotaManagerService.clearStoragesForPrincipal() for clearing all storages under the given origin.
5.40 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
Part 5 : Modify the ForgetAboutSite.jsm to clear a site for all originAttributes in general.
5.46 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
Part 6 : Add a test case for testing forgetting APIs, including nsICookieManager2.getCookiesWithOriginAttributes(), mozIGeckoMediaPluginChromeService.forgetThisSite() and nsIQuotaManagerService.clearStoragesForPrincipal().
17.04 KB, patch
timhuang
: review+
Details | Diff | Splinter Review 
We had made the ForgetAboutSite to clear a site for all userContextIds in Bug 1238183. But this is not enough, we should make this clearing applys to all OAs for the given site. This is important for Tor, because the Tor will create a jar for each domain that the user visits. Therefore, we should apply the OriginAttributesPattern to clear a site to make sure everything has been cleared.
Depends on: 1238183, 1195930
See Bug 1238183 Comment 35 for more details. To allow QuotaManager can clear all data-jars for a given host by the given OriginAttributesPattern, we should add a new API in nsIQuotaManagerService to achieve this.
This should block the tor integration meta bug, but I'm not sure what bug number that is.
Priority: -- → P2
Whiteboard: [userContextId][OA][domsecurity-backlog] → [OA][domsecurity-backlog]
Assignee: nobody → tihuang
Blocks: meta_tor
I am planning to add a new API which is called 'clearStoragesWithOriginAttributes' in the nsIQuotaManagerService. This API will take two arguments, the Origin and the OriginAttributes pattern, and clears all data-jars for the given origin that match with the OriginAttributes pattern. 

I will add a new type of OriginScope which might be called 'OriginPattern' to achieve this. This kind of OriginScope has the origin and the OriginAttributes pattern, and it will match to an Origin when both origins, the one without suffix, are the same and the pattern matches with the OriginAttributes. 

Janv, how do you think about this?
Flags: needinfo?(jvarga)
Extending OriginScope is not trivial, I would rather avoid it if possible.
What exactly needs to be cleared, can you provide some examples of origin strings and origin attributes that specify this new clear operation ?
The purpose of this new clear API is for the upcoming tor browser patches uplifting. We are going to add the first party isolation feature, not by default but by perf, the Bug 1260931 is going to do this.

When this first party isolation has been prefed on. It will create a "jar" for each domain that the user visits. For example, When a user visits the cnn.com and there is a like button in this CNN's page. When the user clicks this like button, it will set some data, cookies or the indexedDB, for the facebook.com. And then the user goes to visit the facebook.com which will also set some data. At this point, there will be two data-jars for the facebook.com:

    1. origin: https://facebook.com, OA: mFirstPartyUri=cnn.com
    2. origin: https://facebook.com, OA: mFirstPartyUri=facebook.com

When the user wants to forget the facebook.com, it needs to clear both these data-jars. Here, we call the new API with origin 'https://facebook.com' and the pattern '{}' to clear all data-jars for the given origin.
Ok, I'll think about it a bit.
Here is the another proposal which does not modify the OriginScope, but to modify the way that OriginClearOp clears. I will add an additional field in the ClearOriginsParams which would be called 'origin' to indicate which origin is going to be cleared. If this field is set, the OriginClearOp will only clear data-jars that match the 'origin' and the OriginAttributes pattern. If this field is not set, then the OringinClearOp acts as the original way.

How do you think, Janv?
The problem is that, the clear operation must be protected by a directory lock. Directory locks consist of several properties which are checked in DirectoryLockImpl::MustWaitFor().
Flags: needinfo?(jvarga)
I'm going to do some experiments with the code ...
So let's say we have two origin directories:
1. https+++facebook.com^mFirstPartyUri=cnn.com
2. https+++facebook.com^mFirstPartyUri=facebook.com

Now you want to clear anything beginning with https+++facebook.com, is that right ?
Even origins with other origin attributes, like userContextId=N ?
(In reply to Jan Varga [:janv] from comment #9)
> I'm going to do some experiments with the code ...
> So let's say we have two origin directories:
> 1. https+++facebook.com^mFirstPartyUri=cnn.com
> 2. https+++facebook.com^mFirstPartyUri=facebook.com
> 
> Now you want to clear anything beginning with https+++facebook.com, is that
> right ?
> Even origins with other origin attributes, like userContextId=N ?

Yes, the idea is to clear everything beginning with https+++facebook.com. Because forget a site means clear all jars of the given site. So we want to use originAttributes pattern to match all data jars of the given site, and clear them.
Component: Security → DOM: Security
For the purpose of clearing cookie-jars for a given host in general, I modify the nsICookieManager2.getCookiesWithOriginAttributes() to take an optional argument, the host, to indicate which host is going to be cleared. If this argument is not given, then this function remains the same functionality as before.
Attachment #8767900 - Flags: review?(josh)
Whiteboard: [OA][domsecurity-backlog] → [OA][domsecurity-active]
(In reply to Tim Huang[:timhuang] from comment #3)
> I am planning to add a new API which is called
> 'clearStoragesWithOriginAttributes' in the nsIQuotaManagerService. This API
> will take two arguments, the Origin and the OriginAttributes pattern, and
> clears all data-jars for the given origin that match with the
> OriginAttributes pattern. 
> 
> I will add a new type of OriginScope which might be called 'OriginPattern'
> to achieve this. This kind of OriginScope has the origin and the
> OriginAttributes pattern, and it will match to an Origin when both origins,
> the one without suffix, are the same and the pattern matches with the
> OriginAttributes. 
> 
> Janv, how do you think about this?

Ok, it seems we have to add a new type to OriginScope.
Comment on attachment 8767900 [details] [diff] [review]
Part 1 : Modify the getCookiesWithOriginAttributes of the nsICookieManager2 to take the host as an optional argument.

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

This looks fine, but I'd like to see tests for the new functionality. It would also be nice to see how it will be used, since there aren't any callers of these APIs outside of tests in mozilla-central right now.

::: netwerk/cookie/nsCookieService.cpp
@@ +580,5 @@
>      nsCOMPtr<nsICookieManager2> cookieManager
>        = do_GetService(NS_COOKIEMANAGER_CONTRACTID);
>      MOZ_ASSERT(cookieManager);
>  
> +    return cookieManager->RemoveCookiesWithOriginAttributes(nsDependentString(aData), nsCString());

EmptyCString()

@@ +4431,5 @@
>  
>  nsresult
>  nsCookieService::GetCookiesWithOriginAttributes(
>      const mozilla::OriginAttributesPattern& aPattern,
> +    const nsCString&    aBaseDomain,

nit: remove the extra spaces here.
Attachment #8767900 - Flags: review?(josh) → review+
Attachment #8767900 - Attachment is obsolete: true
Status: NEW → ASSIGNED
The EME is going to be segregated by the originAttributes after Bug 1283325 landed. So I think we should also consider the EME here. Right now, we use the mozIGeckoMediaPluginChromeService.forgetThisSite() to clear all EME storages regarding to a specific domain. For making this API could clear all originAttributes in general, I propose that we could add a new argument which is the originAttributes pattern for this API, and making the API clears EME storages that match with the given domain and the originAttributes pattern.

How do you think, cpearce?
Flags: needinfo?(cpearce)
(In reply to Tim Huang[:timhuang] from comment #17)
> The EME is going to be segregated by the originAttributes after Bug 1283325
> landed. So I think we should also consider the EME here. Right now, we use
> the mozIGeckoMediaPluginChromeService.forgetThisSite() to clear all EME
> storages regarding to a specific domain. For making this API could clear all
> originAttributes in general, I propose that we could add a new argument
> which is the originAttributes pattern for this API, and making the API
> clears EME storages that match with the given domain and the
> originAttributes pattern.
> 
> How do you think, cpearce?

Yes, we'll need something like that for EME. We have gtests for clearing storage here:
https://dxr.mozilla.org/mozilla-central/source/dom/media/gtest/TestGMPCrossOrigin.cpp#711
Flags: needinfo?(cpearce)
Modify the mozIGeckoMediaPluginChromeService.forgetThisSite() and the gtest for the originAttributes pattern.
Attachment #8775544 - Flags: review?(cpearce)
Comment on attachment 8775544 [details] [diff] [review]
Part 4 : Modify the mozIGeckoMediaPluginChromeService.forgetThisSite() to take the originAttributes pattern as an argument.

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

Please use MozReview for reviews. It's a lot easier as a reviewer.

r+ with comments addressed.

::: dom/media/gmp/GMPServiceParent.cpp
@@ +1568,2 @@
>    rv = ReadFromFile(aPath, NS_LITERAL_CSTRING("origin"), str, MaxDomainLength);
> +  originAttributes.PopulateFromOrigin(str, originNoSuffix);

Should you be checking PopulateFromOrigin() for failure here? And again below.

If it fails, do we assume "no container"? It seems that could end up with us deleting the wrong data if that happens.

Please either check for errors here and assume a non-match on error, or add a comment explaining why we don't need to check for errors.

@@ +1813,5 @@
> +
> +  return ForgetThisSiteNative(aSite, pattern);
> +}
> +
> +NS_IMETHODIMP_(nsresult)

You won't need the NS_IMETHODIMP_(nsresult) here once you move the function into GMPServiceParent.h.

::: dom/media/gmp/mozIGeckoMediaPluginChromeService.idl
@@ +44,5 @@
> +  void forgetThisSite(in AString site,
> +                      in DOMString aPattern);
> +
> +  [notxpcom]
> +  nsresult forgetThisSiteNative(in AString aSite,

Only functions that are called from Chrome JavaScript actually need to be in mozIGeckoMediaPlugin[Chrome]Service.idl.

So please declare this in the GeckoMediaPluginServiceParent class directly as a private function.

Also, can aPattern be const?
Attachment #8775544 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #20)
> Comment on attachment 8775544 [details] [diff] [review]
> Part 4 : Modify the mozIGeckoMediaPluginChromeService.forgetThisSite() to
> take the originAttributes pattern as an argument.
> 
> Review of attachment 8775544 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please use MozReview for reviews. It's a lot easier as a reviewer.
> 

Thanks, I will use the MozReview for sure, but I will still use the old review tool for this bug.

> r+ with comments addressed.
> 
> ::: dom/media/gmp/GMPServiceParent.cpp
> @@ +1568,2 @@
> >    rv = ReadFromFile(aPath, NS_LITERAL_CSTRING("origin"), str, MaxDomainLength);
> > +  originAttributes.PopulateFromOrigin(str, originNoSuffix);
> 
> Should you be checking PopulateFromOrigin() for failure here? And again
> below.
> 
> If it fails, do we assume "no container"? It seems that could end up with us
> deleting the wrong data if that happens.
> 
> Please either check for errors here and assume a non-match on error, or add
> a comment explaining why we don't need to check for errors.

I will add a check for errors here, and treat it as a non-match.

> 
> @@ +1813,5 @@
> > +
> > +  return ForgetThisSiteNative(aSite, pattern);
> > +}
> > +
> > +NS_IMETHODIMP_(nsresult)
> 
> You won't need the NS_IMETHODIMP_(nsresult) here once you move the function
> into GMPServiceParent.h.
> 
> ::: dom/media/gmp/mozIGeckoMediaPluginChromeService.idl
> @@ +44,5 @@
> > +  void forgetThisSite(in AString site,
> > +                      in DOMString aPattern);
> > +
> > +  [notxpcom]
> > +  nsresult forgetThisSiteNative(in AString aSite,
> 
> Only functions that are called from Chrome JavaScript actually need to be in
> mozIGeckoMediaPlugin[Chrome]Service.idl.
> 
> So please declare this in the GeckoMediaPluginServiceParent class directly
> as a private function.

It looks like that there will be an error on the 'private exports' that we export 'ForgetThisSiteNative' as a private function in the TestGMPCrossOrigin.cpp. So I will put 'ForgetThisSiteNative' at the public section.

> 
> Also, can aPattern be const?

Yes, this could be done after the forgetThisSiteNative be moved into the GMPServiceParent.h.
Attachment #8775544 - Attachment is obsolete: true
I sat down with Tim and I suggested to simplify the patches a bit since we currently don't have a use case for clearing a specific origin using origin attributes pattern. All we need is to just delete everything beginning with say https+++facebook.com
We still need to update OriginScope, but the code should be simpler. I suggested to create a new OriginScope type called OriginPrefix which will covert the https+++facebook.com* case.
Attachment #8774241 - Flags: review?(jvarga)
Attachment #8774244 - Flags: review?(jvarga)
Per offline discussion with Janv, we decided to create a new type of OriginScope which is called 'OriginPrefix'. This 'OriginPrefix' will match to all storages under the given origin. Using 'OriginPrefix', we can clear storages for all originAttributes of a given site.
Attachment #8781900 - Flags: review?(jvarga)
Attachment #8774241 - Attachment is obsolete: true
Attachment #8774244 - Attachment is obsolete: true
MattN, could you review this for me? Thanks.
Attachment #8781919 - Flags: review?(MattN+bmo)
Comment on attachment 8781919 [details] [diff] [review]
Part 5 : Modify the ForgetAboutSite.jsm to clear a site for all originAttributes in general.

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

Please use mozreview in the future.

It seems like there should be tests added in the other patches for each of the APIs you are changing then I'm fine without integration tests in this patch.

r=me assuming you will write unit tests that use the same arguments that are used in ForgetAboutSite.jsm and test that the userContextId and other originAttributes are ignored when clearing.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm
@@ +82,5 @@
>  
>      // EME
>      let mps = Cc["@mozilla.org/gecko-media-plugin-service;1"].
>                 getService(Ci.mozIGeckoMediaPluginChromeService);
> +    mps.forgetThisSite(aDomain, JSON.stringify({}));

Passing `JSON.stringify({})` as an argument is unusual. It seems like there should be an XPIDL interface for OriginAttributesPattern instead and I'm not sure why that wasn't done.

@@ +159,5 @@
>      let httpURI = caUtils.makeURI("http://" + aDomain);
>      let httpsURI = caUtils.makeURI("https://" + aDomain);
> +    let httpPrincipal = Services.scriptSecurityManager
> +                                .createCodebasePrincipal(httpURI, {});
> +    let httpsPrincipal = Services.scriptSecurityManager

Noting here that this hunk is just reverting to its pre-bug 1238183 state except for the change in arguments to clearStoragesForPrincipal
Attachment #8781919 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8781900 [details] [diff] [review]
Part 2 : Add a new type, the OriginPrefix, of OriginScope for the Quota Manager.

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

This mostly looks good, I'll give you some comments in few minutes
Comment on attachment 8781900 [details] [diff] [review]
Part 2 : Add a new type, the OriginPrefix, of OriginScope for the Quota Manager.

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

::: dom/quota/OriginScope.h
@@ +18,5 @@
>  public:
>    enum Type
>    {
>      eOrigin,
> +    eOriginPrefix,

maybe just ePrefix

@@ +31,5 @@
>    }
>  
>    static OriginScope
> +  FromOriginPrefix(const nsACString& aOrigin)
> +  {

Maybe just FromPrefix()
I don't think we should reuse mOrigin for both types.
I'm going to file a new bug and attach a patch to use a union in OriginScope. So adding a new type will be more efficient since specific types only need a subset of member variables.

I also propose to modify existing |OriginScope(const nsACString& aOrigin)| constructor to look like this:

OriginScope(const nsACString& aOriginOrPrefix, bool aOrigin)
{
  if (aOrigin) {
    mOriginAndAttributes = new OriginAndAttributes(aOriginOrPrefix);
    mType = eOrigin;
  } else {
    mPrefix = new nsCString(aOriginOrPrefix);
    mType = ePrefix;
  }
}

So you can just |return OriginScope(aPrefix, false);| here in FromPrefix()

@@ +104,5 @@
>    }
>  
>    void
> +  SetFromOriginPrefix(const nsACString& aOrigin)
> +  {

This will be easier when you apply my patch.

@@ +136,5 @@
>  
>    const nsACString&
>    GetOrigin() const
>    {
> +    MOZ_ASSERT(IsOrigin() || IsOriginPrefix());

Again, I think we shouldn't reuse this for both types to make the code easier to read and maintain.

@@ +170,5 @@
>  
>      if (IsOrigin()) {
>        match = mOrigin.Equals(aOther.mOrigin);
> +    } else if (IsOriginPrefix()) {
> +      match = StringBeginsWith(aOther.mOrigin, mOrigin);

here, you will use mOrigin/mPrefix not mOrigin/mOrigin

@@ +187,5 @@
> +
> +    bool match;
> +
> +    if (IsOrigin()) {
> +      match = StringBeginsWith(mOrigin, aOther.mOrigin);

dtto

@@ +194,5 @@
> +    } else if (IsPattern()) {
> +      // The match will be always true here because the origin prefix targets
> +      // all storages under this origin, which means it overlaps all
> +      // originAttributes patterns.
> +      match = true;

Is this really true ?
The important part is "under this origin". What about comparison of two OriginScope objects with different origins/prefixes ?

The last branch here just sets match to true, because that's the case when we are null origin scope and null covers everything, all origins.

@@ +214,5 @@
> +    } else if (IsOriginPrefix()) {
> +      // The match will be always true here because the origin prefix targets
> +      // all storages under this origin, which means it overlaps all
> +      // originAttributes patterns.
> +      match = true;

dtto
Attachment #8781900 - Flags: review?(jvarga)
Comment on attachment 8781901 [details] [diff] [review]
Part 3 : Modify the nsIQuotaManagerService.clearStoragesForPrincipal() for clearing all storages under the given origin.

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

::: dom/quota/ActorsParent.cpp
@@ +6290,5 @@
>      return;
>    }
>  
>    OriginScope originScope = mOriginScope.Clone();
> +  if (originScope.IsOrigin() || originScope.IsOriginPrefix()) {

This will need to be changed to call GetOrigin()/SetOrigin() and GetPrefix()/SetPrefix() accordingly.

::: dom/quota/QuotaManagerService.cpp
@@ +566,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(aPrincipal);
>    MOZ_ASSERT(nsContentUtils::IsCallerChrome());
>  

I think when the new flag is true, we should check here that the principal doesn't have any origin attributes and warn and bail out early.

::: dom/quota/nsIQuotaManagerService.idl
@@ +53,5 @@
>     */
>    nsIQuotaRequest
>    clearStoragesForPrincipal(in nsIPrincipal aPrincipal,
> +                            [optional] in ACString aPersistenceType,
> +                            [optional] in boolean aClearAllOriginAttributes);

aClearAllOriginAttributes sounds a bit weird to me
aClearStoragesForAllOriginsIgnoringOriginAttributes :)

I don't know, I'll try to figure it out tomorrow.
Attachment #8781901 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #29)
> @@ +194,5 @@
> > +    } else if (IsPattern()) {
> > +      // The match will be always true here because the origin prefix targets
> > +      // all storages under this origin, which means it overlaps all
> > +      // originAttributes patterns.
> > +      match = true;
> 
> Is this really true ?
> The important part is "under this origin". What about comparison of two
> OriginScope objects with different origins/prefixes ?
> 
> The last branch here just sets match to true, because that's the case when
> we are null origin scope and null covers everything, all origins.
> 

let me think about this a bit more
(In reply to Jan Varga [:janv] from comment #33)
> (In reply to Jan Varga [:janv] from comment #29)
> > @@ +194,5 @@
> > > +    } else if (IsPattern()) {
> > > +      // The match will be always true here because the origin prefix targets
> > > +      // all storages under this origin, which means it overlaps all
> > > +      // originAttributes patterns.
> > > +      match = true;
> > 
> > Is this really true ?
> > The important part is "under this origin". What about comparison of two
> > OriginScope objects with different origins/prefixes ?
> > 
> > The last branch here just sets match to true, because that's the case when
> > we are null origin scope and null covers everything, all origins.
> > 
> 
> let me think about this a bit more

You are actually right here, it sounds weird but yes any pattern matches/overlaps any origin prefix.
(In reply to Jan Varga [:janv] from comment #34)
> (In reply to Jan Varga [:janv] from comment #33)
> > (In reply to Jan Varga [:janv] from comment #29)
> > > @@ +194,5 @@
> > > > +    } else if (IsPattern()) {
> > > > +      // The match will be always true here because the origin prefix targets
> > > > +      // all storages under this origin, which means it overlaps all
> > > > +      // originAttributes patterns.
> > > > +      match = true;
> > > 
> > > Is this really true ?
> > > The important part is "under this origin". What about comparison of two
> > > OriginScope objects with different origins/prefixes ?
> > > 
> > > The last branch here just sets match to true, because that's the case when
> > > we are null origin scope and null covers everything, all origins.
> > > 
> > 
> > let me think about this a bit more
> 
> You are actually right here, it sounds weird but yes any pattern
> matches/overlaps any origin prefix.

But I would change the comment to something like this:
The match will be always true here because any origin attributes pattern
overlaps any origin prefix (an origin prefix targets all origin attributes).
The new version patch which based on Janv's patch.
Attachment #8782739 - Flags: review?(jvarga)
Comment on attachment 8781901 [details] [diff] [review]
Part 3 : Modify the nsIQuotaManagerService.clearStoragesForPrincipal() for clearing all storages under the given origin.

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

::: dom/quota/ActorsParent.cpp
@@ +6253,5 @@
>                                              nullptr);
>      if (NS_WARN_IF(NS_FAILED(rv))) {
>        return rv;
>      }
> +    if (!params.clearAllOriginAttributes()) {

You can remove the negation and just reverse following two calls.
Comment on attachment 8782739 [details] [diff] [review]
Part 2 : Add a new type, the Prefix, of OriginScope for the Quota Manager.

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

Thanks!

::: dom/quota/OriginScope.h
@@ +59,5 @@
>      // ePattern
>      mozilla::OriginAttributesPattern* mPattern;
>  
> +    // ePrefix
> +    nsCString *mPrefix;

Nit: nsCString* mPrefix

@@ +294,5 @@
> +    } else if (IsPrefix()) {
> +      MOZ_ASSERT(mPrefix);
> +      // The match will be always true here because the origin prefix targets
> +      // all storages under this origin, which means it overlaps all
> +      // originAttributes patterns.

Nit: just update the comment as I suggested

@@ +317,5 @@
> +    } else if (IsPattern()) {
> +      MOZ_ASSERT(mPattern);
> +      // The match will be always true here because the origin prefix targets
> +      // all storages under this origin, which means it overlaps all
> +      // originAttributes patterns.

dtto
Attachment #8782739 - Flags: review?(jvarga) → review+
Depends on: 1296512
Comment on attachment 8781901 [details] [diff] [review]
Part 3 : Modify the nsIQuotaManagerService.clearStoragesForPrincipal() for clearing all storages under the given origin.

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

::: dom/quota/PQuota.ipdl
@@ +32,5 @@
>  {
>    PrincipalInfo principalInfo;
>    PersistenceType persistenceType;
>    bool persistenceTypeIsExplicit;
> +  bool clearAllOriginAttributes;

just clearAll

::: dom/quota/nsIQuotaManagerService.idl
@@ +53,5 @@
>     */
>    nsIQuotaRequest
>    clearStoragesForPrincipal(in nsIPrincipal aPrincipal,
> +                            [optional] in ACString aPersistenceType,
> +                            [optional] in boolean aClearAllOriginAttributes);

Ok, let's just use aClearAll. We can add a comment for it later or if you have time do it in this patch (@aparam aClearAll ...)
Attachment #8781900 - Attachment is obsolete: true
Attachment #8782739 - Attachment is obsolete: true
Attachment #8781901 - Attachment is obsolete: true
Comment on attachment 8782820 [details] [diff] [review]
Part 3 : Modify the nsIQuotaManagerService.clearStoragesForPrincipal() for clearing all storages under the given origin.

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

Thanks!

::: dom/quota/ActorsParent.cpp
@@ +6293,5 @@
>      SanitizeOriginString(originSanitized);
>      originScope.SetOrigin(originSanitized);
>    }
>  
> +  if (originScope.IsPrefix()) {

Nit: else if (...

::: dom/quota/nsIQuotaManagerService.idl
@@ +50,5 @@
>     *
>     * @param aPrincipal
>     *        A principal for the origin whose storages are to be cleared.
> +   * @param aPersistenceType
> +   *        An optional string tells that what persistence type of storages

Nit: s/tells that/that tells
Attachment #8782820 - Flags: review?(jvarga) → review+
Attachment #8782820 - Attachment is obsolete: true
Comment on attachment 8784372 [details] [diff] [review]
Part 6 : Add a test case for testing forgetting APIs, including nsICookieManager2.getCookiesWithOriginAttributes(), mozIGeckoMediaPluginChromeService.forgetThisSite() and nsIQuotaManagerService.clearStoragesForPrincipal().

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

To me they can easily be 3 separate tests. up to you.

::: browser/components/contextualidentity/test/browser/browser_forgetAPIs.js
@@ +11,5 @@
> +  "default",
> +  "personal",
> +];
> +
> +const TESTEMEKEY = {

which name is this? TEST_EME_KEY

@@ +41,5 @@
> +  return Services.cookies.getCookiesFromHost(host, {userContextId});
> +}
> +
> +function HexToBase64(hex)
> +{

choose: this style or the getCookiesForOA style. { in a new line or not?

@@ +50,5 @@
> +  return window.btoa(bin).replace(/=/g, "").replace(/\+/g, "-").replace(/\//g, "_");
> +}
> +
> +function Base64ToHex(str)
> +{

ditto.
Attachment #8784372 - Flags: review?(amarchesini) → review+
Attachment #8781919 - Attachment is obsolete: true
Attachment #8784372 - Attachment is obsolete: true
Try looks good.
Keywords: checkin-needed
Tim, thank you for getting this resolved!  Great job!
You need to log in before you can comment on or make changes to this bug.