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

RESOLVED FIXED in Firefox 62

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: bkelly, Assigned: baku)

Tracking

(Blocks 1 bug, {dev-doc-complete})

48 Branch
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: btpp-backlog, [domsecurity-backlog1] )

Attachments

(4 attachments, 14 obsolete attachments)

19.58 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
2.75 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.23 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.35 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
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.
Blocks: 1147820
Whiteboard: btpp-backlog
Sites are starting to experiment with this feature:

https://twitter.com/mikewest/status/817391771508502529
Priority: -- → P3
Duplicate of this bug: 1391250
Component: DOM → DOM: Security
Whiteboard: btpp-backlog → btpp-backlog, [domsecurity-backlog1]
Assignee

Comment 3

Last year
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.
Assignee

Comment 4

Last year
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.
Assignee: nobody → amarchesini
Attachment #8975782 - Flags: review?(valentin.gosu)
Assignee

Comment 5

Last year
nsICookieManager::removeCookiesWithOriginAttributesNative works with OriginAttributesPattern, but ClearSiteData has access to the principal's OriginAttributes. I need to create a OriginAttributesPattern from a OriginAttributes.
Attachment #8975783 - Flags: review?(tanvi)
Assignee

Updated

Last year
Attachment #8975783 - Attachment description: part 2 - new OriginAttributesPattern CTOR → part B - new OriginAttributesPattern CTOR
Assignee

Comment 6

Last year
I don't think we have anything similar in SWM, but maybe I'm wrong.
Attachment #8975784 - Flags: review?(bkelly)
Assignee

Comment 7

Last year
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?)
Attachment #8975786 - Flags: feedback?(honzab.moz)
Attachment #8975786 - Flags: feedback?(bugs)
Assignee

Comment 8

Last year
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.
Attachment #8975787 - Flags: feedback?(bugs)
Assignee

Comment 9

Last year
Posted patch part F - prefs (obsolete) — Splinter Review
Attachment #8975788 - Flags: feedback?(bugs)
Apparently the spec isn't too well reviewed, so reviewers, please ensure the spec makes sense :)
Assignee

Updated

Last year
Attachment #8975786 - Flags: feedback?(bugs) → feedback?(bkelly)
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.
Baku, please make sure we have a pref for this feature so we can switch it off if needed.
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.)
Attachment #8975787 - Flags: feedback?(bugs)
Comment on attachment 8975788 [details] [diff] [review]
part F - prefs

Use StaticPrefList.h
It is a new way to deal with *VarCache.
Attachment #8975788 - Flags: feedback?(bugs) → feedback-
Assignee

Comment 15

Last year
Posted patch part 2 - prefs (obsolete) — Splinter Review
Attachment #8975788 - Attachment is obsolete: true
Attachment #8975843 - Flags: feedback?(bugs)
Assignee

Comment 16

Last year
(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 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.
Attachment #8975843 - Flags: feedback?(bugs) → feedback+
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.
Attachment #8975786 - Flags: feedback?(bkelly) → feedback-
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.
Attachment #8975784 - Flags: review?(bkelly)
Assignee

Comment 20

Last year
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 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.
Attachment #8975786 - Flags: feedback?(honzab.moz) → feedback-
Assignee

Updated

Last year
Attachment #8975782 - Attachment is obsolete: true
Attachment #8975782 - Flags: review?(valentin.gosu)
Assignee

Updated

Last year
Attachment #8975783 - Attachment is obsolete: true
Attachment #8975783 - Flags: review?(tanvi)
Assignee

Updated

Last year
Attachment #8975784 - Attachment is obsolete: true
Assignee

Updated

Last year
Depends on: 1422365
Assignee

Updated

Last year
Depends on: 1328695
Assignee

Comment 22

Last year
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.
Attachment #8975786 - Attachment is obsolete: true
Attachment #8983878 - Flags: feedback?(honzab.moz)
Assignee

Updated

Last year
Attachment #8975843 - Attachment description: part F - prefs → part 2 - prefs
Assignee

Comment 23

Last year
Attachment #8975787 - Attachment is obsolete: true
Attachment #8983879 - Flags: feedback?(bugs)
Assignee

Comment 24

Last year
Attachment #8983880 - Flags: review?(honzab.moz)
Assignee

Comment 25

Last year
Posted patch part 5 - image cache clean up (obsolete) — Splinter Review
Attachment #8983881 - Flags: review?(jhofmann)
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 ...
Attachment #8983879 - Flags: feedback?(bugs) → feedback+
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)
Attachment #8983880 - Flags: review?(honzab.moz) → review-
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
Attachment #8983878 - Flags: feedback?(honzab.moz) → feedback+
Assignee

Comment 29

Last year
> 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.
Assignee

Updated

Last year
Flags: needinfo?(honzab.moz)
(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...
Flags: needinfo?(honzab.moz)
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
Attachment #8983881 - Flags: review?(jhofmann) → review-
Assignee

Updated

Last year
Depends on: 1468501
Assignee

Comment 32

Last year
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?
Attachment #8983878 - Attachment is obsolete: true
Attachment #8985201 - Flags: review?(honzab.moz)
Assignee

Comment 33

Last year
Posted patch part 2 - prefs (obsolete) — Splinter Review
Attachment #8975843 - Attachment is obsolete: true
Attachment #8985202 - Flags: review?(bugs)
Assignee

Comment 34

Last year
Attachment #8985202 - Attachment is obsolete: true
Attachment #8985202 - Flags: review?(bugs)
Attachment #8985203 - Flags: review?(bugs)
Attachment #8985203 - Flags: review?(bugs) → review+
Assignee

Comment 35

Last year
Attachment #8983879 - Attachment is obsolete: true
Attachment #8985221 - Flags: review?(bugs)
Assignee

Updated

Last year
Attachment #8983880 - Attachment is obsolete: true
(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 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 (...) {
Attachment #8985221 - Flags: review?(bugs) → review-
Assignee

Comment 38

Last year
> 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.
Assignee

Comment 39

Last year
Posted patch part 4 - image cache clean up (obsolete) — Splinter Review
Attachment #8983881 - Attachment is obsolete: true
Attachment #8985384 - Flags: review?(jhofmann)
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...
Attachment #8985384 - Flags: review?(jhofmann) → review+
Assignee

Comment 41

Last year
Attachment #8985221 - Attachment is obsolete: true
Attachment #8985676 - Flags: review?(bugs)
Comment on attachment 8985676 [details] [diff] [review]
part 3 - browsing contexts reload

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># 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
Attachment #8985676 - Flags: review?(bugs) → review+
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?
Attachment #8985201 - Flags: review?(honzab.moz) → review+
Assignee

Comment 44

Last year
part 4 was not enough because it removes just the passed entry. We need to remove all the entries matching origin + originAttributes.
Attachment #8985384 - Attachment is obsolete: true
Attachment #8986350 - Flags: review?(aosmond)
Comment on attachment 8986350 [details] [diff] [review]
part 4 - cached images removed from principal

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

LGTM.
Attachment #8986350 - Flags: review?(aosmond) → review+

Comment 46

Last year
Pushed by amarchesini@mozilla.com:
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 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.
(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"?
Assignee

Comment 50

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

Yes. Do you want me to file a follow up bug for this and comment 48?
(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.
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

Last year
Pushed by amarchesini@mozilla.com:
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
Assignee

Updated

Last year
Flags: needinfo?(amarchesini)
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.
Andrea, I was just wondering about comment 56? ^

Did I misunderstand?
Flags: needinfo?(amarchesini)
Assignee

Comment 58

Last year
Brian, thanks for you comment. You are right. Fixing in a follow up.
Flags: needinfo?(amarchesini)
Assignee

Updated

Last year
Blocks: 1476190
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?
Flags: needinfo?(amarchesini)
Assignee

Comment 60

11 months ago
Perfect! Thanks.
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.