Possibly related to Issue 650509, hence the QUIC label. However, CSD implies that the Channel ID will be flushed, which means that the TLS session(s) need to be flushed, as well as the QUIC streams and HTTP/2 connections. So this is not at all unsurprising to me, and to a degree, working as expected - clearing that information requires all connections (including any in-progress connections) to be aborted.

Browsing data remover has historically been used in response to the user to explicitly clearing data, so breaking pages currently being loaded has been "OK" from its standpoint.  As more types of user data have been added, what option clears them has been based around what pre-existing type of user data they most resemble (In terms of what sort of data they contain, and how they're used).  It's not at all surprising that this broader behavior has lead to some issues when hooked up to a web API with very specific expectations.

I'm not sure that Clear-Site-Data should even be using BrowsingDataRemover, because the use cases are so different.

Thanks for sparing me the debugging, Ryan :)

I'll have to admit that the spec prescribes that Clear-Site-Data should behave as if certain JavaScript APIs were called on the data storage backends, which means it would be cleaner to also implement it that way, and not go through BrowsingDataRemover.

However, if you consider what Clear-Site-Data needs to do: know which data storage backends belong to which data type, execute deletions on those, do some thread jumps, wait until all deletions are finished. That's pretty much what BrowsingDataRemover is for. From my perspective as maintainer of that code, it's much better to have user- and web-initiated deletions go through the same place.

Now, it's true that this leads to some unexpected behavior where data types have different meaning for the user than for the web. But that's actually pretty easy to capture in the data type enum passed to BrowsingDataRemover, and we indeed have introduced new enum values because of Clear-Site-Data.

However, that's not the problem we're facing here. Clear-Site-Data is supposed to reset the state of the website, and Channel IDs are a cookie like mechanism, therefore I'd argue that it is indeed desirable to include them in the "cookie" datatype, and it's not just an artifact of our current infrastructure. Of course, we need to rethink how we do that, or whether we want to change the API to also allow users to delete only the regular cookies and not Channel IDs, etc.

I think there may have been confusion - I think CSD clearing Channel IDs is perfectly reasonable, and in line with clearing Cookie-like things. However, in doing so, all of the behaviours that come with clearing cookies - and all the guarantees (or lack thereof) we make with BrowsingDataRemover - come with it.

We've still got CSD behind a flag, right? As it stands, someone flushing CSD should be able to flush all connections to all sites (IIRC), because of the considerable complexity involved with trying to target just affected connections, and the lack of such complexity when only considering BrowsingDataRemover's use cases.

Alright, by elimination method I found out that there are actually two datatypes at fault:
- Channel IDs
- HTTP auth cache

Unsurprisingly, those are exactly the two datatypes in BrowsingDataRemover that access RequestContext. The culprit code is:

void QuicStreamFactory::OnSSLConfigChanged() {
  CloseAllSessions(ERR_CERT_DATABASE_CHANGED, QUIC_CONNECTION_CANCELLED);
}

and

HttpNetworkSession::CloseAllConnections();

respectively.


Ryan - no, Clear-Site-Data is already shipping. The reason this wasn't uncovered before is that our browser tests, web platform tests, as well as my manual testing verified that the data is deleted when the request finishes, but not that the request suceeds. The real-world usage is currently very low and the few customers of the API that we talked to also didn't report this issue.

Options are:
1. Consider this WAI, because the implementation really does pass tests. Note that this works perfectly fine for the canonical usecase where example.com/logout calls a subresource example.com/clear-site-data and doesn't care anymore. The spec can explain the caveat that if you're deleting data that affects connections, you'll lose your connections.
2. At least try to be smarter and don't kill the current connection - requires adding some complexity to the //net code.
3. Remove those two data storage backends from "cookies". But that misses the goal of the header.
4. Split "cookies" into "cookies" and something like "connection auth".

I'll discuss this with mkwst@ again and update this bug as well as blink-dev. If this turns out to require deeper design discussions, we may have to unship in the meantime.

Apologies that I didn't save you effort - I was trying to say exactly that in Comment #2. :)

I believe #1 is the correct solution. I do not believe we can achieve #2, and #3/#4 are explicit non-goals from a privacy angle :)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2ce63988391f99b38260da560abae182a789dcff

commit 2ce63988391f99b38260da560abae182a789dcff
Author: Martin Sramek <msramek@chromium.org>
Date: Thu Jan 18 20:52:52 2018

Ensure that the Clear-Site-Data "cookies" datatype doesn't break loading

By excluding channel IDs and HTTP auth cache, the two data storage
backends whose deletion results in closing connections.

Note that this is a temporary solution - the spec mandates that these
backends should be deleted.

It has been empirically determined that the above are the only two
storage backends with this behavior. A proof in the form of a test
will be provided in a follow-up CL.

Bug: 798760
Change-Id: Ia9ae0e0632d7c41a978fc0e15b8371826196c0c1
Reviewed-on: https://chromium-review.googlesource.com/873311
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Commit-Queue: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530262}
[modify] https://crrev.com/2ce63988391f99b38260da560abae182a789dcff/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
[modify] https://crrev.com/2ce63988391f99b38260da560abae182a789dcff/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h
[modify] https://crrev.com/2ce63988391f99b38260da560abae182a789dcff/content/browser/browsing_data/browsing_data_remover_impl.cc
[modify] https://crrev.com/2ce63988391f99b38260da560abae182a789dcff/content/browser/browsing_data/browsing_data_remover_impl_unittest.cc
[modify] https://crrev.com/2ce63988391f99b38260da560abae182a789dcff/content/browser/browsing_data/clear_site_data_throttle.cc
[modify] https://crrev.com/2ce63988391f99b38260da560abae182a789dcff/content/browser/browsing_data/clear_site_data_throttle_browsertest.cc
[modify] https://crrev.com/2ce63988391f99b38260da560abae182a789dcff/content/browser/browsing_data/clear_site_data_throttle_unittest.cc
[modify] https://crrev.com/2ce63988391f99b38260da560abae182a789dcff/content/public/browser/browsing_data_remover.h

It sounds like the conclusion of this thread is that "everything is good. nothing to see here. move along." Is that right?

For #2, in the case of QUIC and HTTP/2, we could mark the connection as "going away" which prevents it from being used for new requests. Any new requests go out a new connection. We were recently talking about doing this in QUIC for unrelated reasons. Would that be desirable here?

No, actually. We discussed this offline with sleevi@ and mkwst@ and considered the possible solutions.

As a short-term fix, we considered #1 (do nothing) and #3 (don't delete the affected data). We decided that for the time being, an incomplete deletion was a better outcome than failing to load resources, and especially failing to navigate. Solution #3 is landed in the comment 8 above.

The proper solution is more difficult. If we can mark the current connection as "going away" so that it can gracefully finish, that's certainly going to be helpful. Note that we still have to kill the remaining connections to the site that use the now-deleted channel ID, and in ideal world maybe also restart them. What I also understood from Ryan is that this might be difficult in token binding world, where we'll have multiple eTLD+1s over one connection.

HTTP auth cache deletion might be a more low-hanging fruit. No need to close the connection if it wasn't HTTP-auth'd anyways. Which means adding an alternative to HttpNetworkSession::CloseAllConnections() that only closes connections that used HTTP auth data for the current eTLD+1.