1268889 - Implement Clear-Site-Data response header to reset origin storage

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

We should implement this:

  https://w3c.github.io/webappsec-clear-site-data/

Basically sites might get their client-side wedged in a bad state using IDB, Cache API, or Service Workers, and then not be able to gracefully recover.  The Clear-Site-Data is a kill switch they can use to reset these clients if they detect such badness.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Sites are starting to experiment with this feature:

https://twitter.com/mikewest/status/817391771508502529

Comment 3

[Andrea Marchesini [:baku]]

I filed an issue on standards-positions: https://github.com/mozilla/standards-positions/issues/90
Let's discuss there if we want to implement it, how and when.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part A - nsICookieManager::removeCookiesWithOriginAttributesNative

This is an internal method in nsCookieService but it's needed for ClearSiteData service. I need to expose it. This is similar to many other "Native" methods.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part B - new OriginAttributesPattern CTOR

nsICookieManager::removeCookiesWithOriginAttributesNative works with OriginAttributesPattern, but ClearSiteData has access to the principal's OriginAttributes. I need to create a OriginAttributesPattern from a OriginAttributes.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part C - ServiceWorkerManager::GetRegistrationsForPrincipal

I don't think we have anything similar in SWM, but maybe I'm wrong.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part D - Clear-Site-Data observer

This patch works, but I need to know from a necko point of view if this approach is correct (mayhemer?). Same for docShell/dom point of view (smaug?)

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part E - browsing contexts reload

About the ordering of contexts when reloading, I don't think it's a real issue. If a subiframe starts the reloading, and the parent one does it too, the subiframe is stopped and destroyed.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part F - prefs

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Apparently the spec isn't too well reviewed, so reviewers, please ensure the spec makes sense :)

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

I'll try to take a look today.  But note, clear-site-data doesn't even really work 100% in chrome right now:

https://bugs.chromium.org/p/chromium/issues/detail?id=795701

So I agree with comment 10's caution.

Also, my inclination is that bug 1183245 should be fixed before this bug.  The clear-site-data wipe probably needs to be as atomic as we can make it in order to function correctly.  Also, I feel like we've papered over bug 1183245 so many times we should really fix it correctly before adding a new feature on top.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Baku, please make sure we have a pref for this feature so we can switch it off if needed.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8975787 [details] [diff] [review]
part E - browsing contexts reload

>+  if (!nsCRT::strcmp(aTopic, "clear-site-data-reload-needed")) {
>+    nsIPrincipal *principal = GetPrincipal();
>+    if (!principal) {
>+      return NS_OK;
>+    }
>+
>+    nsAutoCString origin;
>+    nsresult rv = principal->GetOrigin(origin);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    NS_ConvertUTF16toUTF8 otherOrigin(aData);
>+    if (!origin.Equals(otherOrigin)) {
>+      return NS_OK;
>+    }
>+
>+    nsCOMPtr<nsIDocShell> docShell = GetDocShell();
>+    nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(docShell));
>+    if (NS_WARN_IF(!webNav)) {
>+      return NS_OK;
>+    }
>+
>+    // We don't need any special reload flags, because this notification is
>+    // dispatched by Clear-Site-Data header, which should have already cleaned
>+    // up all the needed data.
>+    rv = webNav->Reload(nsIWebNavigation::LOAD_FLAGS_NONE);
>+    NS_ENSURE_SUCCESS(rv, rv);
So this doesn't guarantee any ordering. The ordering should be defined in case of non-same origin page has same origin iframe, and in case
same origin page has same origin iframes, it is probably enough to reload just the parent.
At least we shouldn't reload iframes if parent will be reloaded too.
Please ensure there is a spec bug about this.

(I would assume iframes under non-same origin parent would be reloaded in tree order.)

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8975788 [details] [diff] [review]
part F - prefs

Use StaticPrefList.h
It is a new way to deal with *VarCache.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 2 - prefs

Comment 16

[Andrea Marchesini [:baku]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> Baku, please make sure we have a pref for this feature so we can switch it
> off if needed.

It's already behind pref. See patch 'F'.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8975843 [details] [diff] [review]
part 2 - prefs

>+VARCACHE_PREF(
>+  "dom.clearSiteData.enabled",
>+  ClearSiteDataEnabled,
per documentation in the file, the method name should be
dom_clearSiteData_enabled
See the documentation about <pref-name-id>
(I don't really like that naming pattern since it doesn't follow Gecko coding style. But for now, better to follow the suggested style)


>diff --git a/testing/web-platform/meta/clear-site-data/navigation-insecure.html.ini b/testing/web-platform/meta/clear-site-data/navigation-insecure.html.ini
>new file mode 100644
>--- /dev/null
>+++ b/testing/web-platform/meta/clear-site-data/navigation-insecure.html.ini
>@@ -0,0 +1,2 @@
>+[storage.https.html]
>+  prefs: [dom.clearSiteData.enabled:true]
>diff --git a/testing/web-platform/meta/clear-site-data/navigation.https.html.ini b/testing/web-platform/meta/clear-site-data/navigation.https.html.ini
>new file mode 100644
>--- /dev/null
>+++ b/testing/web-platform/meta/clear-site-data/navigation.https.html.ini
>@@ -0,0 +1,2 @@
>+[navigation.https.html]
>+  prefs: [dom.clearSiteData.enabled:true]
>diff --git a/testing/web-platform/meta/clear-site-data/resource.html.html.ini b/testing/web-platform/meta/clear-site-data/resource.html.html.ini
>new file mode 100644
>--- /dev/null
>+++ b/testing/web-platform/meta/clear-site-data/resource.html.html.ini
>@@ -0,0 +1,2 @@
>+[storage.https.html]
>+  prefs: [dom.clearSiteData.enabled:true]
>diff --git a/testing/web-platform/meta/clear-site-data/storage.https.html.ini b/testing/web-platform/meta/clear-site-data/storage.https.html.ini
>new file mode 100644
>--- /dev/null
>+++ b/testing/web-platform/meta/clear-site-data/storage.https.html.ini
>@@ -0,0 +1,2 @@
>+[storage.https.html]
>+  prefs: [dom.clearSiteData.enabled:true]
Couldn't you add __dir__.ini  to the directory and have the pref set there.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8975786 [details] [diff] [review]
part D - Clear-Site-Data observer

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

::: toolkit/components/clearsitedata/ClearSiteData.cpp
@@ +134,5 @@
> +      return;
> +    }
> +
> +    NS_ConvertUTF8toUTF16 origin(mContextsReloadOrigin);
> +    nsresult rv = obs->NotifyObservers(nullptr, "clear-site-data-reload-needed",

Consider adding a method to ClientManager to force reload clients for a given principal.  We have an explicit list of what these clients are, so we can ask them to reload individually instead of using a broadcast observer topic to all processes.

As an added benefit we could also make the clear-site-data understand when this reload is complete.  Consider trying to handle two responses that arrive nearly at the same time and both have clear-site-data.  We probably don't want to start two clear operations.

The ClientManager approach would also allow us to add a stateful approach where we also reload clients that appear after the reload messages are sent, but before the reloads complete.  I think you will be hard pressed to handle that edge case right with a broadcast observer.

@@ +511,5 @@
> +  // sessionStorage/localStorage
> +  ClearSessionAndLocalStorage(aPrincipal);
> +
> +  // ServiceWorker
> +  ClearServiceWorkers(aHolder, aPrincipal);

As we discussed in IRC, I would prefer if we fixed bug 1183245 first so that service worker registrations clear with quota storage.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8975784 [details] [diff] [review]
part C - ServiceWorkerManager::GetRegistrationsForPrincipal

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

As mentioned in the other review I'd prefer that we fix bug 1183245 instead of doing this.

Comment 20

[Andrea Marchesini [:baku]]

I'm OK with waiting for bug 1183245, if this doesn't require too much time.

I think we should decide the priority for this bug and for bug 1183245. From a DOM-Security point of view, this is 4 of 5, where 5 is the highest priority. But I guess the first step would be to have a proper spec review.

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 8975786 [details] [diff] [review]
part D - Clear-Site-Data observer

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

I'm overall not happy with the spec as it is now.

::: toolkit/components/clearsitedata/ClearSiteData.cpp
@@ +206,5 @@
> +  if (NS_WARN_IF(!channel)) {
> +    return NS_OK;
> +  }
> +
> +  // No needs to propagate the error.

what exactly do you mean?  assuming that when something goes wrong during the deletion, what should you do with the error?  display somewhere? - that can be done here.  fail the response?

@@ +279,5 @@
> +  }
> +
> +  bool isData = false;
> +  if (NS_SUCCEEDED(aURI->SchemeIs("data", &isData)) && isData) {
> +    return true;

wow!

@@ +430,5 @@
> +void
> +ClearSiteData::ClearCache() const
> +{
> +  // There is no way to clear just that domain, so we clear out
> +  // everything)

no, sorry, you can't do this.  there is a way to delete by origin from the http cache, see[1].  

I'm really surprised the spec allows delete by host, so that it allows insecure or even non-standard port servers to trigger deletion of the whole domain.  and what about subdomains?  what if a 3rd party iframe (either the doc or its subresource) gets this response header (didn't read the spec that deeply)?

the way you wrote it it could be used for ddos attacking the whole internet when all caches for everything in a huge number of firefox sessions are deleted at once by whatever e.g. malicious ad response.


[1] https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/netwerk/cache2/CacheObserver.cpp#479-490

@@ +453,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return;
> +  }
> +
> +  imageCache->ClearCache(false); // true=chrome, false=content

there is also media cache, dom cache.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 1 - Clear-Site-Data implementation

It turned out that we have to block per origin. This means nsIPrincipal.
There are a couple of changes needed in nsIClearDataService in order to cleanup necko and image caches: see next patches.
I don't ask for a review request because maybe we still have something to fix at a spec level first.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 3 - browsing contexts reload

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 4 - necko cache cleanup by principal

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 5 - image cache clean up

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8983879 [details] [diff] [review]
part 3 - browsing contexts reload

>+nsGlobalWindowInner::PropagateClearSiteDataReload(const nsACString& aOrigin)
>+{
>+  nsIPrincipal *principal = GetPrincipal();
  nsIPrincipal* principal

>+
>+  // If the URL of this window matches, let's refresh this window only.
not URL, but origin

>@@ -3005,16 +3006,20 @@ ContentParent::Observe(nsISupports* aSub
>     NS_ASSERTION(xpcCookie, "couldn't get cookie");
>     if (!nsCRT::strcmp(aData, u"deleted")) {
>       cs->RemoveCookie(xpcCookie);
>     } else if ((!nsCRT::strcmp(aData, u"added")) ||
>                (!nsCRT::strcmp(aData, u"changed"))) {
>       cs->AddCookie(xpcCookie);
>     }
>   }
>+  else if (!strcmp(aTopic, "clear-site-data-reload-needed")) {
} else ...

Comment 27

[Honza Bambas (:mayhemer)]

Comment on attachment 8983880 [details] [diff] [review]
part 4 - necko cache cleanup by principal

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

::: toolkit/components/cleardata/ClearDataService.js
@@ +88,5 @@
> +      return new Promise(aResolve => {
> +        let loadContextInfo = Services.loadContextInfo.custom(aAnonymous,
> +                                                              aPrincipal.originAttributes);
> +        let storage = Services.cache2.diskCacheStorage(loadContextInfo, false);
> +        storage.asyncDoomURI(aPrincipal.URI, "", { onCacheEntryDoomed(aResult) { aResolve(); } });

the method you want is AsyncEvictStorage.  have you read the idl docs?

note that the callback passed to AsyncEvictStorage is just artificially called (dispatch to main thread) for compatibility reasons (and may be removed one day).  it doesn't wait for clearing.  it doesn't have to, since it's ensured any entry for the origin (scope) is inaccessible after AsyncEvictStorage returned.  actual disk cleanup happens in background.

also, you could just trigger the "clear-origin-attributes-data" topic directly on nsICacheStorageService QI'ed to nsIObserver with OA serialization so that you don't need to duplicate the code here.  You could also just expose an API on nsICacheStorageService to do per-origin cleanup to be more explicit and clear. (preferred)

Comment 28

[Honza Bambas (:mayhemer)]

Comment on attachment 8983878 [details] [diff] [review]
part 1 - Clear-Site-Data implementation

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

::: toolkit/components/clearsitedata/ClearSiteData.cpp
@@ +24,5 @@
> +using namespace mozilla;
> +
> +namespace {
> +
> +StaticRefPtr<ClearSiteData> gClearSiteData;

please assert a single thread access (main thread?) on every read/write to this global

@@ +44,5 @@
> +  nsresult
> +  Start()
> +  {
> +    MOZ_ASSERT(!mPendingOp);
> +    nsresult rv = mChannel->Suspend();

I'd appreciate some logging added here ("clearsitedata" module?) to track ownership and operations made on the channel

@@ +53,5 @@
> +    mPendingOp = true;
> +    return NS_OK;
> +  }
> +
> +  // This method must be called after any IncreaseOp() call.

IncreaseOp ?

@@ +71,5 @@
> +  {
> +    MOZ_ASSERT(mPendingOp);
> +    mPendingOp = false;
> +
> +    mChannel->Resume();

can you throw the channel away here if no longer needed?

@@ +79,5 @@
> +
> +private:
> +  ~PendingCleanupHolder()
> +  {
> +    MOZ_ASSERT(!mPendingOp);

resume the channel when still pending, please.

@@ +172,5 @@
> +ClearSiteData::ClearDataFromChannel(nsIHttpChannel* aChannel)
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsIURI> uri;
> +  rv = aChannel->GetURI(getter_AddRefs(uri));

please document carefully why channel.URI is the right thing to use here.

@@ +177,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return;
> +  }
> +
> +  // If response’s url is not an a priori authenticated URL, then break.

please don't add comments that just describe what you do and is clear from the code itself.  if you want a comment here, say WHY you do what you do

@@ +204,5 @@
> +  RefPtr<PendingCleanupHolder> holder = new PendingCleanupHolder(aChannel);
> +
> +  if (flags & eCache) {
> +    LogOpToConsole(aChannel, uri, eCache);
> +    cleanFlags |= nsIClearDataService::CLEAR_ALL_CACHES;

ok, if this goes away in following patches that land together, then I can accept this.

f+ only if this will never land alone.  to be honest, I'm kinda wondering why the patch that introduces deletion by origin is not blocking this one.  sometimes I'm getting confused by how you split your patches.

@@ +229,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return;
> +    }
> +
> +    rv = holder->Start();

should holder->Start() be called before DeleteDataFromPrincipal() ?  In case the deletion operation is done synchronously or something happens to call the callback (e.g. with a failure) from inside DeleteDataFromPrincipal() the holder is notified before Start() is called and we screw up.

@@ +242,5 @@
> +  }
> +}
> +
> +bool
> +ClearSiteData::IsPrioriAuthenticatedURI(nsIURI* aURI) const

Please rename this to IsSecureURI().  "authentication" terminology is usually used when a client has provided authentication (basic auth, client cert auth etc..)

@@ +297,5 @@
> +    }
> +
> +    if (value.EqualsLiteral("\"*\"")) {
> +      flags = eCache | eCookies | eStorage | eExecutionContexts;
> +      continue;

break?

::: toolkit/components/clearsitedata/ClearSiteData.h
@@ +15,5 @@
> +class nsIURI;
> +
> +namespace mozilla {
> +
> +class ClearSiteData final : public nsIObserver

baku, please always add comments to new classes, its methods and members, even if you asking only for feedback

Comment 29

[Andrea Marchesini [:baku]]

> the method you want is AsyncEvictStorage.  have you read the idl docs?

Yes, and it seems it doesn't do what I want:

https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/netwerk/cache2/nsICacheStorage.idl#132-136

   * Asynchronously removes all cached entries under this storage.
   * NOTE: Disk storage also evicts memory storage.

But, what I want is to delete entries related to particular origin. This is why I use asyncoomURI.

  /**
   * Asynchronously removes an entry belonging to the URI from the cache.
   */

I don't think we want to remove all the cached entries when Clear-Site-Data is used for foobar.com. We probably want to remove entries beloging to foobar.com.

Comment 30

[Honza Bambas (:mayhemer)]

(In reply to Andrea Marchesini [:baku] from comment #29)
> > the method you want is AsyncEvictStorage.  have you read the idl docs?
> 
> Yes, and it seems it doesn't do what I want:
> 
> https://searchfox.org/mozilla-central/rev/
> cf464eabfeba64e866c1fa36b9fefd674dca9c51/netwerk/cache2/nsICacheStorage.
> idl#132-136
> 
>    * Asynchronously removes all cached entries under this storage.
>    * NOTE: Disk storage also evicts memory storage.

Ah!!  Yes, you are right, this is for distinction only by origin attributes, but those don't keep the origin :((  The code was there for times we had apps (b2g).

> 
> But, what I want is to delete entries related to particular origin. This is
> why I use asyncoomURI.
> 
>   /**
>    * Asynchronously removes an entry belonging to the URI from the cache.
>    */
> 
> I don't think we want to remove all the cached entries when Clear-Site-Data
> is used for foobar.com. We probably want to remove entries beloging to
> foobar.com.

This is not the way.  This will delete a single entry for the specific URI ("an entry belonging to the URI").  Not everything that belongs to the origin.

Sigh, I don't think we have an API :((  you will have to walk all entries in the cache (visit) and delete all that belong to the origin (and maybe some other criteria).  This is asynchronous but walking happens on the main thread and can easily flood it.

No one ever needed this so far, hence it has never been implemented :/  Please file a bug to have an API for this.  I don't want this to be a main thread thing (visitor over EVERY entry implemented in JS).  I want this happen in background with low priority.  Stupid is that the response with such a header will be blocked and that operation (delete cache by origin) can be pretty slow.

The spec is so aggressive...

Comment 31

[Johann Hofmann [:johannh]]

Comment on attachment 8983881 [details] [diff] [review]
part 5 - image cache clean up

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

I don't see how this works given that this function is [noscript], what am I missing? :)

https://searchfox.org/mozilla-central/rev/bf81d741ff5dd11bb364ef21306da599032fd479/image/imgICache.idl#42

Comment 32

[Andrea Marchesini [:baku]]

Attachment: part 1 - Clear-Site-Data implementation

Bug 1468501 is about introducing a method to cleanup network cache. With this patch, Clear-Site-Data will not be able to delete network cache, but I still want to have this code reviewed, and maybe land in m-i, disabled by pref.
In the meantime, I (or somebody else from necko) could work on bug 1468501.
Mayhemer, are you OK with this approach?

Comment 33

[Andrea Marchesini [:baku]]

Attachment: part 2 - prefs

Comment 34

[Andrea Marchesini [:baku]]

Attachment: part 2 - prefs

Comment 35

[Andrea Marchesini [:baku]]

Attachment: part 3 - browsing contexts reload

Comment 36

[Honza Bambas (:mayhemer)]

(In reply to Andrea Marchesini [:baku] from comment #32)
> Created attachment 8985201 [details] [diff] [review]
> part 1 - Clear-Site-Data implementation
> 
> Bug 1468501 is about introducing a method to cleanup network cache. With
> this patch, Clear-Site-Data will not be able to delete network cache, but I
> still want to have this code reviewed, and maybe land in m-i, disabled by
> pref.
> In the meantime, I (or somebody else from necko) could work on bug 1468501.
> Mayhemer, are you OK with this approach?

Yes!  Thanks.  I will look at the patch likely tomorrow (in allhands now, today kinda ran out review power...)

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8985221 [details] [diff] [review]
part 3 - browsing contexts reload


>+++ b/dom/base/nsGlobalWindowInner.cpp
>@@ -936,16 +936,18 @@ nsGlobalWindowInner::nsGlobalWindowInner
>       nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>       if (os) {
>         // Watch for online/offline status changes so we can fire events. Use
>         // a strong reference.
>         os->AddObserver(mObserver, NS_IOSERVICE_OFFLINE_STATUS_TOPIC,
>                         false);
> 
>         os->AddObserver(mObserver, MEMORY_PRESSURE_OBSERVER_TOPIC, false);
>+
>+        os->AddObserver(mObserver, "clear-site-data-reload-needed", false);
>       }
> 
>       Preferences::AddStrongObserver(mObserver, "intl.accept_languages");
> 
>       // Watch for storage notifications so we can fire storage events.
>       RefPtr<StorageNotifierService> sns =
>         StorageNotifierService::GetOrCreate();
>       if (sns) {
>@@ -1263,16 +1265,17 @@ nsGlobalWindowInner::FreeInnerObjects()
> 
>   DisconnectEventTargetObjects();
> 
>   if (mObserver) {
>     nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>     if (os) {
>       os->RemoveObserver(mObserver, NS_IOSERVICE_OFFLINE_STATUS_TOPIC);
>       os->RemoveObserver(mObserver, MEMORY_PRESSURE_OBSERVER_TOPIC);
>+      os->RemoveObserver(mObserver, "clear-site-data-reload-needed");
>     }
Couldn't you add/remove the observer only when dealing with top level window?
That would reduce the number of observers quite a bit.
I think I'd like to see a patch doing that, unless there is some good reason to not do it.


>+nsGlobalWindowInner::PropagateClearSiteDataReload(const nsACString& aOrigin)
>+{
>+  nsIPrincipal* principal = GetPrincipal();
>+  if (!principal) {
>+    return;
>+  }
>+
>+  nsAutoCString origin;
>+  nsresult rv = principal->GetOrigin(origin);
>+  NS_ENSURE_SUCCESS_VOID(rv);
>+
>+  // If the URL of this window matches, let's refresh this window only.
>+  // We don't need to traverse the DOM tree.
>+  if (origin.Equals(aOrigin)) {
>+    nsCOMPtr<nsIDocShell> docShell = GetDocShell();
>+    nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(docShell));
>+    if (NS_WARN_IF(!webNav)) {
>+      return;
>+    }
>+
>+    // We don't need any special reload flags, because this notification is
>+    // dispatched by Clear-Site-Data header, which should have already cleaned
>+    // up all the needed data.
>+    rv = webNav->Reload(nsIWebNavigation::LOAD_FLAGS_NONE);
>+    NS_ENSURE_SUCCESS_VOID(rv);
>+
>+    return;
ok, so we try to trigger reload only on the topmost same-origin page. Makes sense, but ensure the
spec actually defines this. And what happens if beforeunload listeners try to prevent reload?



>@@ -3006,16 +3007,19 @@ ContentParent::Observe(nsISupports* aSub
>     nsCOMPtr<nsICookie> xpcCookie = do_QueryInterface(aSubject);
>     NS_ASSERTION(xpcCookie, "couldn't get cookie");
>     if (!nsCRT::strcmp(aData, u"deleted")) {
>       cs->RemoveCookie(xpcCookie);
>     } else if ((!nsCRT::strcmp(aData, u"added")) ||
>                (!nsCRT::strcmp(aData, u"changed"))) {
>       cs->AddCookie(xpcCookie);
>     }
>+  } else if (!strcmp(aTopic, "clear-site-data-reload-needed")) {
} else if (...) {

Comment 38

[Andrea Marchesini [:baku]]

> ok, so we try to trigger reload only on the topmost same-origin page. Makes
> sense, but ensure the
> spec actually defines this. And what happens if beforeunload listeners try
> to prevent reload?

The spec doesn't say anything special for the beforeonload.
And about the ordering, I filed a spec bug.

Comment 39

[Andrea Marchesini [:baku]]

Attachment: part 4 - image cache clean up

Comment 40

[Johann Hofmann [:johannh]]

Comment on attachment 8985384 [details] [diff] [review]
part 4 - image cache clean up

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

The browser parts are good, though I can't say for sure what the reason was for making removeEntry non-scriptable in the first place. It looks fine to make scriptable to me...

Comment 41

[Andrea Marchesini [:baku]]

Attachment: part 3 - browsing contexts reload

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8985676 [details] [diff] [review]
part 3 - browsing contexts reload

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  959853dff54fab03295fae778165994bd7bd1a59
>
>diff --git a/dom/base/nsGlobalWindowInner.cpp b/dom/base/nsGlobalWindowInner.cpp
>--- a/dom/base/nsGlobalWindowInner.cpp
>+++ b/dom/base/nsGlobalWindowInner.cpp
>@@ -936,16 +936,20 @@ nsGlobalWindowInner::nsGlobalWindowInner
>       nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>       if (os) {
>         // Watch for online/offline status changes so we can fire events. Use
>         // a strong reference.
>         os->AddObserver(mObserver, NS_IOSERVICE_OFFLINE_STATUS_TOPIC,
>                         false);
> 
>         os->AddObserver(mObserver, MEMORY_PRESSURE_OBSERVER_TOPIC, false);
>+
>+        if (aOuterWindow->IsTopLevelWindow()) {
>+          os->AddObserver(mObserver, "clear-site-data-reload-needed", false);
>+        }
>       }
> 
>       Preferences::AddStrongObserver(mObserver, "intl.accept_languages");
> 
>       // Watch for storage notifications so we can fire storage events.
>       RefPtr<StorageNotifierService> sns =
>         StorageNotifierService::GetOrCreate();
>       if (sns) {
>@@ -1263,16 +1267,21 @@ nsGlobalWindowInner::FreeInnerObjects()
> 
>   DisconnectEventTargetObjects();
> 
>   if (mObserver) {
>     nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>     if (os) {
>       os->RemoveObserver(mObserver, NS_IOSERVICE_OFFLINE_STATUS_TOPIC);
>       os->RemoveObserver(mObserver, MEMORY_PRESSURE_OBSERVER_TOPIC);
>+
>+      if (GetOuterWindowInternal() &&
>+          GetOuterWindowInternal()->IsTopLevelWindow()) {
>+        os->RemoveObserver(mObserver, "clear-site-data-reload-needed");
Is there any case where eGetOuterWindowInternal return null here?
Since if it does return null and we've managed to call AddObserver, we leak.
So, either explain why it can't return null and remove the null check, or unconditionally
call os->RemoveObserver

>+  if (!nsCRT::strcmp(aTopic, "clear-site-data-reload-needed")) {
>+    // The reload is propagated from the top-level window only.
Could you assert that we're top level

Comment 43

[Honza Bambas (:mayhemer)]

Comment on attachment 8985201 [details] [diff] [review]
part 1 - Clear-Site-Data implementation

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

::: toolkit/components/clearsitedata/ClearSiteData.cpp
@@ +72,5 @@
> +    MOZ_ASSERT(mPendingOp);
> +    mPendingOp = false;
> +
> +    mChannel->Resume();
> +    mChannel = nullptr;

I'd rather swap to local, just in case

@@ +82,5 @@
> +private:
> +  ~PendingCleanupHolder()
> +  {
> +    if (mPendingOp) {
> +      mChannel->Resume();

maybe also check mChannel not null

@@ +181,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsIURI> uri;
> +
> +  // We want to use the final URI to check if Clear-Site-Data should be allowed
> +  // or not.

and why?  also, "final channel URI" is something else than channel->URI.  but thinking about it, this is probably correct.  final URI and channel URI will be different only when the http channel is wrapped by some other protocol.  we probably still want to delete the data for such a response, so channel->URI is fine.

@@ +305,5 @@
> +      continue;
> +    }
> +
> +    if (value.EqualsLiteral("\"*\"")) {
> +      flags = eCache | eCookies | eStorage | eExecutionContexts;

nit: maybe have eAll flag defined, just in case we add a new flag to not forget to return it here?

Comment 44

[Andrea Marchesini [:baku]]

Attachment: part 4 - cached images removed from principal

part 4 was not enough because it removes just the passed entry. We need to remove all the entries matching origin + originAttributes.

Comment 45

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8986350 [details] [diff] [review]
part 4 - cached images removed from principal

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

LGTM.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd3b7047ea6f
Implement Clear-Site-Data header - part 1, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2320061fbcb
Implement Clear-Site-Data header - part 2 - pref, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/97528847b7b7
Implement Clear-Site-Data header - part 3 - reload contexts, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/da427a67372e
Implement Clear-Site-Data header - part 4 - cleanup image cache, r=aosmond

Comment 47

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/dd3b7047ea6f
https://hg.mozilla.org/mozilla-central/rev/b2320061fbcb
https://hg.mozilla.org/mozilla-central/rev/97528847b7b7
https://hg.mozilla.org/mozilla-central/rev/da427a67372e

Comment 48

[Francesco Lodolo [:flod]]

Comment on attachment 8985201 [details] [diff] [review]
part 1 - Clear-Site-Data implementation

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

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +357,5 @@
>  ProximityEventWarning=Use of the proximity sensor is deprecated.
>  AmbientLightEventWarning=Use of the ambient light sensor is deprecated.
>  # LOCALIZATION NOTE: Do not translate "storage", "indexedDB.open" and "navigator.storage.persist()".
>  IDBOpenDBOptions_StorageTypeWarning=The ‘storage’ attribute in options passed to indexedDB.open is deprecated and will soon be removed. To get persistent storage, please use navigator.storage.persist() instead.
> +RunningClearSiteDataValue=Clear-Site-Data header forces the clean up of “%S” data.

For the future, please add comments explaining what replaces placeholders. It's really hard to translate a string like this without the context given by code.

Comment 49

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #48)
> > +RunningClearSiteDataValue=Clear-Site-Data header forces the clean up of “%S” data.

Side question: should this message say "forced"?

Comment 50

[Andrea Marchesini [:baku]]

> Side question: should this message say "forced"?

Yes. Do you want me to file a follow up bug for this and comment 48?

Comment 51

[Francesco Lodolo [:flod]]

(In reply to Andrea Marchesini [:baku] from comment #50)
> > Side question: should this message say "forced"?
> 
> Yes. Do you want me to file a follow up bug for this and comment 48?

I'm on it. Will file the follow up and ask you for review.

Comment 52

[Narcis Beleuzu [:NarcisB]]

Backed out for windows GTest failures [@ LateWriteObserver::Observe(mozilla::IOInterposeObserver::Observation &)]

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/59322b179c0fdb4423d42b624da7bf64ee5a2327
Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=da427a67372e19b219eaec6fada813babfd6e4b2
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=184133697&repo=mozilla-inbound&lineNumber=837

Comment 53

[Francesco Lodolo [:flod]]

I've asked to back out bug 1470138 from autoland. At this point, it probably makes more sense to add the fixes into this bug directly.

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/015ed5271d5d
Implement Clear-Site-Data header - part 1, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/d33b51527821
Implement Clear-Site-Data header - part 2 - pref, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6a2857d3e21
Implement Clear-Site-Data header - part 3 - reload contexts, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a94bdf78194
Implement Clear-Site-Data header - part 4 - cleanup image cache, r=aosmond

Comment 55

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/015ed5271d5d
https://hg.mozilla.org/mozilla-central/rev/d33b51527821
https://hg.mozilla.org/mozilla-central/rev/d6a2857d3e21
https://hg.mozilla.org/mozilla-central/rev/2a94bdf78194

Comment 56

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8985203 [details] [diff] [review]
part 2 - prefs

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

::: modules/libpref/init/StaticPrefList.h
@@ +108,5 @@
>  //---------------------------------------------------------------------------
> +// Clear-Site-Data prefs
> +//---------------------------------------------------------------------------
> +
> +#ifdef NIGHTLY

Is this supposed to be true for trunk/nightly builds?

It doesn't seem to be true when I do a local build, and checking the value of dom.clearSiteData.enabled on my nightly build reveals it is false.

Comment 57

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Andrea, I was just wondering about comment 56? ^

Did I misunderstand?

Comment 58

[Andrea Marchesini [:baku]]

Brian, thanks for you comment. You are right. Fixing in a follow up.

Comment 59

[Florian Scholz (Open Web Docs)]

Documented here https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Clear-Site-Data
As it is not yet enabled, I haven't added it to release notes, but added ddn tracking to bug 1470111 for that.

Does this look alright to you, :baku?

Comment 60

[Andrea Marchesini [:baku]]

Perfect! Thanks.