Do no AppCache responses with SSL cert errors.

Do not AppCache responses with SSL cert errors.

BUG=414026
Committed: https://crrev.com/3ec982d993b769efc893d4f3a0fa28eea95b69d0
Cr-Commit-Position: refs/heads/master@{#299999}

[michaeln, 2014-10-10 22:15:29]

michaeln@chromium.org changed reviewers:
+ palmer@chromium.org, rsleevi@chromium.org

[michaeln, 2014-10-10 22:15:52]

ptal

[palmer, 2014-10-10 23:48:11]

Thank you! This is so much better than my attempt to solve this bug. I had no
idea what I was doing. :)

LGTM.

https://codereview.chromium.org/645123003/diff/1/content/browser/appcache/app...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/645123003/diff/1/content/browser/appcache/app...
content/browser/appcache/appcache_update_job.cc:178: result_ = SERVER_ERROR;  //
Not the best match?
Maybe CANCELED_ERROR, or add a new one, like NOT_SUPPORTED_ERROR or something?

[palmer, 2014-10-10 23:50:32]

https://codereview.chromium.org/645123003/diff/1/content/browser/appcache/app...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/645123003/diff/1/content/browser/appcache/app...
content/browser/appcache/appcache_update_job.cc:185: if
(net::IsCertStatusError(request->ssl_info().cert_status)) {
Lines 186 – 190 duplicate lines 176 – 180; maybe just add the condition on line
185 to the condition in lines 174 – 175?

[palmer, 2014-10-10 23:50:59]

Also typo in the commit message: "Do not cache AppCache responses..."

[palmer, 2014-10-10 23:51:00]

Also typo in the commit message: "Do not cache AppCache responses..."

[Ryan Sleevi, 2014-10-11 00:54:44]

LGTM, but style nits

https://codereview.chromium.org/645123003/diff/1/content/browser/appcache/app...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/645123003/diff/1/content/browser/appcache/app...
content/browser/appcache/appcache_update_job.cc:164: 
unnecessary newline?

https://codereview.chromium.org/645123003/diff/1/content/browser/appcache/app...
content/browser/appcache/appcache_update_job.cc:212: OnResponseCompleted();
This reads like error handling under success handling, which our style guide
discourages ( http://www.chromium.org/developers/coding-style#Code_Formatting
used to make this more explicit)

It definitely took me a while to read through and make sure this didn't also
need to change, so I'd encourage you to restructure as

if ((response_code / 100) != 2) {
  if (response_code > 0)
    result_ = SERVER_ERROR;
  else
    result_ = NETWORK_ERROR;
  OnResponseCompleted();
  return;
}

....

[michaeln, 2014-10-15 19:20:22]

https://codereview.chromium.org/645123003/diff/1/content/browser/appcache/app...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/645123003/diff/1/content/browser/appcache/app...
content/browser/appcache/appcache_update_job.cc:164: 
On 2014/10/11 00:54:44, Ryan Sleevi wrote:
> unnecessary newline?

Done.

https://codereview.chromium.org/645123003/diff/1/content/browser/appcache/app...
content/browser/appcache/appcache_update_job.cc:178: result_ = SERVER_ERROR;  //
Not the best match?
On 2014/10/10 23:48:10, Chromium Palmer wrote:
> Maybe CANCELED_ERROR, or add a new one, like NOT_SUPPORTED_ERROR or something?

SECURITY_ERROR?

https://codereview.chromium.org/645123003/diff/1/content/browser/appcache/app...
content/browser/appcache/appcache_update_job.cc:185: if
(net::IsCertStatusError(request->ssl_info().cert_status)) {
On 2014/10/10 23:50:32, Chromium Palmer wrote:
> Lines 186 – 190 duplicate lines 176 – 180; maybe just add the condition on
line
> 185 to the condition in lines 174 – 175?

Done.

https://codereview.chromium.org/645123003/diff/1/content/browser/appcache/app...
content/browser/appcache/appcache_update_job.cc:212: OnResponseCompleted();
On 2014/10/11 00:54:44, Ryan Sleevi wrote:
> This reads like error handling under success handling, which our style guide
> discourages ( http://www.chromium.org/developers/coding-style#Code_Formatting
> used to make this more explicit)
> 
> It definitely took me a while to read through and make sure this didn't also
> need to change, so I'd encourage you to restructure as
> 
> if ((response_code / 100) != 2) {
>   if (response_code > 0)
>     result_ = SERVER_ERROR;
>   else
>     result_ = NETWORK_ERROR;
>   OnResponseCompleted();
>   return;
> }
> 
> ....

Thnx, that helped with readability quite a lot.

[michaeln, 2014-10-16 20:35:03]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-10-16 20:36:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645123003/140001

[michaeln, 2014-10-16 21:25:53]

michaeln@chromium.org changed reviewers:
+ isherman@chromium.org

[michaeln, 2014-10-16 21:25:53]

@isherman for metrics OWNER review

[commit-bot: I haz the power, 2014-10-16 21:37:23]

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

[commit-bot: I haz the power, 2014-10-16 21:37:25]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ilya Sherman, 2014-10-16 21:51:34]

histograms LGTM

[Ilya Sherman, 2014-10-16 21:51:39]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-10-16 21:56:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645123003/140001

[commit-bot: I haz the power, 2014-10-16 22:53:19]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2014-10-16 22:54:43]

Patchset 4 (id:??) landed as
https://crrev.com/3ec982d993b769efc893d4f3a0fa28eea95b69d0
Cr-Commit-Position: refs/heads/master@{#299999}

[cbiesinger, 2014-10-17 00:03:03]

It looks like this broke a blink layout test:
http/tests/appcache/different-https-origin-resource-main.html

https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7/2165...

[cbiesinger, 2014-10-17 00:03:04]

It looks like this broke a blink layout test:
http/tests/appcache/different-https-origin-resource-main.html

https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7/2165...

[michaeln, 2014-10-17 01:36:28]

On 2014/10/17 00:03:04, cbiesinger wrote:
> It looks like this broke a blink layout test:
> http/tests/appcache/different-https-origin-resource-main.html
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7/2165...

Yup, apparently the http server used for SSL tests uses a cert with an error so
the test can't setup it's preconditions of having an appcache installed.

[Ryan Sleevi, 2014-10-17 01:50:08]

On 2014/10/17 01:36:28, michaeln wrote:
> On 2014/10/17 00:03:04, cbiesinger wrote:
> > It looks like this broke a blink layout test:
> > http/tests/appcache/different-https-origin-resource-main.html
> > 
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7/2165...
> 
> Yup, apparently the http server used for SSL tests uses a cert with an error
so
> the test can't setup it's preconditions of having an appcache installed.

Is it some Blink test server? The one in //net properly masks off all errors.

We can also do this in any unit test target (just not full binaries), but more
details about the test infra are needed. I'm surprised we have any tests with
invalid certs - they will trigger all sorts of other behaviors in the net stack.

[michaeln1, 2014-10-17 19:32:09]

> Is it some Blink test server? The one in //net properly masks off all errors.

Apache is used for layout tests. I see broken lock icons and
NET::ERR_CERT_AUTHORITY_INVALID when running it locally.

[michaeln, 2014-10-23 21:10:35]

On 2014/10/17 19:32:09, michaeln1 wrote:
> > Is it some Blink test server? The one in //net properly masks off all
errors.
> 
> Apache is used for layout tests. I see broken lock icons and
> NET::ERR_CERT_AUTHORITY_INVALID when running it locally.

horo found more magic, net::HttpNetworkSession::Params.ignore_certificate_errors

[Ryan Sleevi, 2014-10-23 21:14:30]

On 2014/10/23 21:10:35, michaeln wrote:
> On 2014/10/17 19:32:09, michaeln1 wrote:
> > > Is it some Blink test server? The one in //net properly masks off all
> errors.
> > 
> > Apache is used for layout tests. I see broken lock icons and
> > NET::ERR_CERT_AUTHORITY_INVALID when running it locally.
> 
> horo found more magic,
net::HttpNetworkSession::Params.ignore_certificate_errors

You shouldn't be respecting that. We can follow-up with a meeting if needed.