Disable SDCH by Default

Disable SDCH by Default

Disable SDCH in Chrome. It remains accessible via either explicit
initialization of an SdchManager/SdchOwner or through the use of
URLRequestContextBuilder's set_sdch_enabled(true)

BUG=696815

[Ryan Sleevi, 2017-02-28 00:40:05]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 00:40:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Sleevi, 2017-02-28 00:41:45]

rsleevi@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Ryan Sleevi, 2017-02-28 00:41:46]

Randy: As far as I can tell, this is _all_ it takes.

I'm curious what tests you think should be appropriate here. AFAICT, the //net
tests should cover most permutations. My only concern was the potential risk of
cache issues with SDCH-encoded content, but that 'should' be addressed by the
server sending Vary: accept-encoding, so we should be fine.

Any suggestions you might have of additional tests, I'm game to make a stab at
(with the caveat I turn into a pumpkin on Wednesday afternoon for the move, and
ideally we have this go with 58)

[commit-bot: I haz the power, 2017-02-28 01:43:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 01:43:57]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Randy Smith (Not in Mondays), 2017-02-28 19:17:16]

[CCing Helen FHI.]

I agree with you that this is all it takes.  I'm concerned about caches being
poisoned with SDCH encoded content when the server *hasn't* sent the Vary:
Accept-Encoding header, and while servers should send it, trusting servers seems
like a bad idea.  

Let's do this: Can you do a manual test to fill the cache with an SDCH encoded
response, disable SDCH, and see how the browser behaves?  Ideally for both a
main frame and subframe resource.  I want to make sure that's a recoverable
failure; if it's not, we might want to think about nuking the cache on the
revision change.  

The other request I have is to change the description to say "It remains
accessible *for non-Chrome embedders* ..." just to make clearer what that
accessibility means.  

With the above two, assuming it's not hard for users to recover from the cache
poisoning scenario, LGTM.

[Ryan Sleevi, 2017-02-28 19:22:16]

On 2017/02/28 19:17:16, Randy Smith (Not in Mondays) wrote:
> Let's do this: Can you do a manual test to fill the cache with an SDCH encoded
> response, disable SDCH, and see how the browser behaves?  Ideally for both a
> main frame and subframe resource.  I want to make sure that's a recoverable
> failure; if it's not, we might want to think about nuking the cache on the
> revision change.  

I'm a bit uncertain about how to do that, given the SDCH experiments. Can you
provide any further pointers? I would have thought this would have been a basic
test already covered by the SDCH and URLRequest unit tests.

Would you agree that, if a test for this doesn't already exist, that's where it
belongs?

[Ryan Sleevi, 2017-02-28 19:25:07]

On 2017/02/28 19:17:16, Randy Smith (Not in Mondays) wrote:
> The other request I have is to change the description to say "It remains
> accessible *for non-Chrome embedders* ..." just to make clearer what that
> accessibility means.  

Well, that definition isn't what it means :) So I think we should try to figure
that out.

I'm a bit confused on your proposed wording, as there's no such thing as a
Chrome embedder. I suspect the confusion may be that you read accessibility as
"within the product" and I wrote accessibility as "within the code" - would that
be a fair summary?

[Randy Smith (Not in Mondays), 2017-02-28 19:32:06]

On 2017/02/28 19:22:16, Ryan Sleevi (overloaded) wrote:
> On 2017/02/28 19:17:16, Randy Smith (Not in Mondays) wrote:
> > Let's do this: Can you do a manual test to fill the cache with an SDCH
encoded
> > response, disable SDCH, and see how the browser behaves?  Ideally for both a
> > main frame and subframe resource.  I want to make sure that's a recoverable
> > failure; if it's not, we might want to think about nuking the cache on the
> > revision change.  
> 
> I'm a bit uncertain about how to do that, given the SDCH experiments. Can you
> provide any further pointers? I would have thought this would have been a
basic
> test already covered by the SDCH and URLRequest unit tests.
> 
> Would you agree that, if a test for this doesn't already exist, that's where
it
> belongs?

I certainly have no objection to putting it there, but I'm not aware of any such
test currently.  I don't think there are any tests that explore transitioning
within the test from SDCH enabled to SDCH disabled.

I'm also not certain that this is really a question about SDCH.  After the
disable, what this will look like is a unknown encoding type, which will result
in the response being passed through unchanged.  So (presuming a failure on the
part of the servers WRT Vary: Accept-Encoding) this will cause a bad user
experience--either a garbled web page or a garbled subresource.  So such a test
will fail, under reasonable definitions of success.  What I'm more curious about
is how painful this is for the user--does a reload fix it, or not?  I think of
that as a question a manual test can answer better than an auto test.  

I suspect a reload won't fix it (except for a main frame resource in the case
that the server fails up-to-date validation because it notices the
accept-encoding change), which makes me a bit concerned about the change.  Then
it comes down to "How frequently do servers fail to send Vary: Accept-Encoding
with SDCH encoded responses?" to which the answer is that I have no freaking
idea and I don't think we have UMA to evaluate it currently in the product.

If you think I'm being over-paranoid, please tell me.  And I'm sorry not to have
thought this through before this code review.  But as I talk this through more,
I get more concerned about it.

What do you think the chances are that we can do a cache reset as part of the
CL/release transition?

[xunjieli, 2017-02-28 19:38:41]

xunjieli@chromium.org changed reviewers:
+ xunjieli@chromium.org

[xunjieli, 2017-02-28 19:38:42]

https://codereview.chromium.org/2720613006/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (left):

https://codereview.chromium.org/2720613006/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:447:
profile_path_.Append(chrome::kNetworkPersistentStateFilename));
chrome::kNetworkPersistentStateFilename is only used for SDCH purpose. We can
remove the constant in this CL.

Do we have any guideline on how to remove a persisted file that is no longer
needed? Do we leave the file there and rely on the user to clear their cache in
this case?