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.

Sites are starting to experiment with this feature: https://twitter.com/mikewest/status/817391771508502529

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.

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.

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

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

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

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.

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

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

Comment on attachment 8975788 [details] [diff] [review] part F - prefs Use StaticPrefList.h It is a new way to deal with *VarCache.

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

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

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.

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

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

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

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?

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

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

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?

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

Comment on attachment 8986350 [details] [diff] [review] part 4 - cached images removed from principal Review of attachment 8986350 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

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

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

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

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

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.

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

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

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

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?

Perfect! Thanks.