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

NEW
Assigned to

Status

()

Core
DOM: Security
P3
normal
2 years ago
2 days ago

People

(Reporter: bkelly, Assigned: baku)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed})

48 Branch
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(4 attachments, 13 obsolete attachments)

19.58 KB, patch
baku
: review?
mayhemer
Details | Diff | Splinter Review
2.75 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.19 KB, patch
johannh
: review+
Details | Diff | Splinter Review
9.23 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Blocks: 1147820
Whiteboard: btpp-backlog
(Reporter)

Comment 1

2 years ago
Sites are starting to experiment with this feature:

https://twitter.com/mikewest/status/817391771508502529
Keywords: dev-doc-needed
Priority: -- → P3
(Reporter)

Updated

10 months ago
Duplicate of this bug: 1391250
(Reporter)

Updated

10 months ago
Component: DOM → DOM: Security
(Reporter)

Updated

10 months ago
Whiteboard: btpp-backlog → btpp-backlog, [domsecurity-backlog1]
(Assignee)

Comment 3

a month ago
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

a month ago
Created attachment 8975782 [details] [diff] [review]
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.
Assignee: nobody → amarchesini
Attachment #8975782 - Flags: review?(valentin.gosu)
(Assignee)

Comment 5

a month ago
Created attachment 8975783 [details] [diff] [review]
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.
Attachment #8975783 - Flags: review?(tanvi)
(Assignee)

Updated

a month ago
Attachment #8975783 - Attachment description: part 2 - new OriginAttributesPattern CTOR → part B - new OriginAttributesPattern CTOR
(Assignee)

Comment 6

a month ago
Created attachment 8975784 [details] [diff] [review]
part C - ServiceWorkerManager::GetRegistrationsForPrincipal

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

Comment 7

a month ago
Created attachment 8975786 [details] [diff] [review]
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?)
Attachment #8975786 - Flags: feedback?(honzab.moz)
Attachment #8975786 - Flags: feedback?(bugs)
(Assignee)

Comment 8

a month ago
Created attachment 8975787 [details] [diff] [review]
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.
Attachment #8975787 - Flags: feedback?(bugs)
(Assignee)

Comment 9

a month ago
Created attachment 8975788 [details] [diff] [review]
part F - prefs
Attachment #8975788 - Flags: feedback?(bugs)
Apparently the spec isn't too well reviewed, so reviewers, please ensure the spec makes sense :)
(Assignee)

Updated

a month ago
Attachment #8975786 - Flags: feedback?(bugs) → feedback?(bkelly)
(Reporter)

Comment 11

a month ago
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

a month ago
Created attachment 8975843 [details] [diff] [review]
part 2 - prefs
Attachment #8975788 - Attachment is obsolete: true
Attachment #8975843 - Flags: feedback?(bugs)
(Assignee)

Comment 16

a month ago
(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+
(Reporter)

Comment 18

a month ago
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-
(Reporter)

Comment 19

a month ago
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

a month ago
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

29 days ago
Attachment #8975782 - Attachment is obsolete: true
Attachment #8975782 - Flags: review?(valentin.gosu)
(Assignee)

Updated

29 days ago
Attachment #8975783 - Attachment is obsolete: true
Attachment #8975783 - Flags: review?(tanvi)
(Assignee)

Updated

29 days ago
Attachment #8975784 - Attachment is obsolete: true
(Assignee)

Updated

29 days ago
Depends on: 1422365
(Assignee)

Updated

29 days ago
Depends on: 1328695
(Assignee)

Comment 22

13 days ago
Created attachment 8983878 [details] [diff] [review]
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.
Attachment #8975786 - Attachment is obsolete: true
Attachment #8983878 - Flags: feedback?(honzab.moz)
(Assignee)

Updated

13 days ago
Attachment #8975843 - Attachment description: part F - prefs → part 2 - prefs
(Assignee)

Comment 23

13 days ago
Created attachment 8983879 [details] [diff] [review]
part 3 - browsing contexts reload
Attachment #8975787 - Attachment is obsolete: true
Attachment #8983879 - Flags: feedback?(bugs)
(Assignee)

Comment 24

13 days ago
Created attachment 8983880 [details] [diff] [review]
part 4 - necko cache cleanup by principal
Attachment #8983880 - Flags: review?(honzab.moz)
(Assignee)

Comment 25

13 days ago
Created attachment 8983881 [details] [diff] [review]
part 5 - image cache clean up
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

11 days ago
> 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

11 days ago
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

6 days ago
Depends on: 1468501
(Assignee)

Comment 32

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

Comment 33

6 days ago
Created attachment 8985202 [details] [diff] [review]
part 2 - prefs
Attachment #8975843 - Attachment is obsolete: true
Attachment #8985202 - Flags: review?(bugs)
(Assignee)

Comment 34

6 days ago
Created attachment 8985203 [details] [diff] [review]
part 2 - prefs
Attachment #8985202 - Attachment is obsolete: true
Attachment #8985202 - Flags: review?(bugs)
Attachment #8985203 - Flags: review?(bugs)

Updated

6 days ago
Attachment #8985203 - Flags: review?(bugs) → review+
(Assignee)

Comment 35

6 days ago
Created attachment 8985221 [details] [diff] [review]
part 3 - browsing contexts reload
Attachment #8983879 - Attachment is obsolete: true
Attachment #8985221 - Flags: review?(bugs)
(Assignee)

Updated

6 days ago
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

5 days ago
> 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

5 days ago
Created attachment 8985384 [details] [diff] [review]
part 4 - image cache clean up
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

4 days ago
Created attachment 8985676 [details] [diff] [review]
part 3 - browsing contexts reload
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+
You need to log in before you can comment on or make changes to this bug.