Fix parsing of max-age in SdchManager.

Fix parsing of max-age in SdchManager.

BUG=596541, 596523
Committed: https://crrev.com/38ab932422f88d294a7869ed512c41d6bfec3236
Cr-Commit-Position: refs/heads/master@{#386766}

[eroman, 2016-04-11 19:36:36]

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

[eroman, 2016-04-11 19:36:37]

https://codereview.chromium.org/1871383005/diff/1/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/1871383005/diff/1/net/base/sdch_manager.cc#ne...
net/base/sdch_manager.cc:379: // max-age must be a non-negative number. If it is
very large saturate
Randy, what is the expected behavior here?

I wrote some code as a proof of concept, but don't expect to actually leave it
as-written.

  * What is the normative reference for the SDCH spec? I found some stuff
online, but it didn't clarify parsing strictness, nor the full production for
max-age (other than it is delta-seconds, which it leaves undefined. The HTTP
spec defines delta-seconds as 1*DIGIT though, so it does not include negative
numbers).

  * At least one test relies on max-age allowing negative numbers. This seems
like a violation. The existing behavior has it successfully parse it and infer
an expired time.

  * What is the rationale for which things in this function hard fail, vs those
that do optimistic parsing? For instance the parsing of "port" below is
best-effort and if given a bad number simply pretends it doesn't exist. (I
previously ported this to use net::ParseUint32, but preserved its existing
semantics).

  * The default expiration if max-age is unspecified is Now() + 30days. The code
I wrote here as a proof of concept uses the default in the case too where
max-age was specified but was invalid. This seems wrong, and does cause a test
to fail. A better approach would be to either default to an expired date, or to
just fail the parsing outright.

What do you think?

[eroman, 2016-04-11 19:36:48]

Description was changed from

==========
Fix parsing of max-mage in SdchManager.

BUG=596541
==========

to

==========
Fix parsing of max-age in SdchManager.

BUG=596541
==========

[Randy Smith (Not in Mondays), 2016-04-12 17:17:15]

On 2016/04/11 19:36:37, eroman wrote:
> https://codereview.chromium.org/1871383005/diff/1/net/base/sdch_manager.cc
> File net/base/sdch_manager.cc (right):
> 
>
https://codereview.chromium.org/1871383005/diff/1/net/base/sdch_manager.cc#ne...
> net/base/sdch_manager.cc:379: // max-age must be a non-negative number. If it
is
> very large saturate
> Randy, what is the expected behavior here?
> 
> I wrote some code as a proof of concept, but don't expect to actually leave it
> as-written.
> 
>   * What is the normative reference for the SDCH spec? I found some stuff
> online, but it didn't clarify parsing strictness, nor the full production for
> max-age (other than it is delta-seconds, which it leaves undefined. The HTTP
> spec defines delta-seconds as 1*DIGIT though, so it does not include negative
> numbers).

I believe the normative spec is
https://docs.google.com/document/d/1REMkwjXY5yFOkJwtJPjCMwZ4Shx3D9vfdAytV_KQC....
 It has the same problems you describe, though.  I'm happy to enforce the
delta-seconds as per the HTTP spec.

>   * At least one test relies on max-age allowing negative numbers. This seems
> like a violation. The existing behavior has it successfully parse it and infer
> an expired time.

Yeah, mea culpa (I think--it was a long time ago); I took a shortcut to get an
expired dictionary.  I certainly have no objection to rewriting the test
properly.

>   * What is the rationale for which things in this function hard fail, vs
those
> that do optimistic parsing? For instance the parsing of "port" below is
> best-effort and if given a bad number simply pretends it doesn't exist. (I
> previously ported this to use net::ParseUint32, but preserved its existing
> semantics).

I'm not aware of any carefully thought out reason for those decisions; I believe
this code pre-dates my ownership of SDCH.  I certainly have no objection to
tightening the parsing up.  

>   * The default expiration if max-age is unspecified is Now() + 30days. The
code
> I wrote here as a proof of concept uses the default in the case too where
> max-age was specified but was invalid. This seems wrong, and does cause a test
> to fail. A better approach would be to either default to an expired date, or
to
> just fail the parsing outright.
> 
> What do you think?

I'd be inclined to fail the parsing in that case, though it might be worthwhile
putting in UMA first just to make sure we won't break half the users of SDCH if
we do that.

[eroman, 2016-04-12 17:44:12]

Thanks Randy!

I see now that the spec was mentioned in net/sdch/README.md, but didn't think to
check for such a file since few code in //net has READMEs.

I updated the CL to be more compatible with current approach. Increasing
strictness can be done as a follow-up
(https://bugs.chromium.org/p/chromium/issues/detail?id=602691) once equipped
with data from the field.

[Randy Smith (Not in Mondays), 2016-04-12 17:56:02]

On 2016/04/12 17:44:12, eroman wrote:
> Thanks Randy!
> 
> I see now that the spec was mentioned in net/sdch/README.md, but didn't think
to
> check for such a file since few code in //net has READMEs.
> 
> I updated the CL to be more compatible with current approach. Increasing
> strictness can be done as a follow-up
> (https://bugs.chromium.org/p/chromium/issues/detail?id=602691) once equipped
> with data from the field.

The so-far-green trybot runs tell me that there's something basic I'm missing,
but I don't understand either:
* Where ParseUint32 is defined on non-Windows bots (I didn't find it currently
used in a scan of Chromium, and the net just has references to it in msdn), and
* Why the test that uses max-age: -1 is still working now that the code no
longer parses negative numbers

Can you fix my confusion?

[eroman, 2016-04-12 18:00:42]

(1)
https://chromium.googlesource.com/chromium/src/+/master/net/base/parse_number...

(2) From the comment in sdch_manger.cc:

// max-age must be a non-negative number. If it is very large saturate
// to 2^32 - 1. If it is invalid then treat it as expired.
// TODO(eroman): crbug.com/602691 be stricter on failure.

In the case of -1 that is invalid and it fails parsing. It therefore treats the
expiration implicitly as being expired (using base::Time()).

The test is just relying on the entry to be expired, and not having any specific
timestamp, hence it passes as before.

[eroman, 2016-04-12 18:01:11]

Description was changed from

==========
Fix parsing of max-age in SdchManager.

BUG=596541
==========

to

==========
Fix parsing of max-age in SdchManager.

BUG=596541, 596523
==========

[Randy Smith (Not in Mondays), 2016-04-12 18:13:34]

Thanks for your patience.  LGTM.

https://codereview.chromium.org/1871383005/diff/20001/net/base/sdch_manager_u...
File net/base/sdch_manager_unittest.cc (right):

https://codereview.chromium.org/1871383005/diff/20001/net/base/sdch_manager_u...
net/base/sdch_manager_unittest.cc:561: // as a consequence of crbug.com/602691.
Would you be willing to expand the second sentence?  I.e. "This test works
because currently an invalid max-age results in the dictionary being considered
expired on loading (crbug.com/602691)."

[eroman, 2016-04-12 18:26:21]

https://codereview.chromium.org/1871383005/diff/20001/net/base/sdch_manager_u...
File net/base/sdch_manager_unittest.cc (right):

https://codereview.chromium.org/1871383005/diff/20001/net/base/sdch_manager_u...
net/base/sdch_manager_unittest.cc:561: // as a consequence of crbug.com/602691.
On 2016/04/12 18:13:33, Randy Smith - Not in Fridays wrote:
> Would you be willing to expand the second sentence?  I.e. "This test works
> because currently an invalid max-age results in the dictionary being
considered
> expired on loading (crbug.com/602691)."

Done.

[eroman, 2016-04-12 18:27:27]

The CQ bit was checked by eroman@chromium.org

[eroman, 2016-04-12 18:27:27]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1871383005/#ps40001
(title: "improve comment with rdsmith's suggestion")

[commit-bot: I haz the power, 2016-04-12 18:27:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1871383005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1871383005/40001

[commit-bot: I haz the power, 2016-04-12 19:39:04]

Description was changed from

==========
Fix parsing of max-age in SdchManager.

BUG=596541, 596523
==========

to

==========
Fix parsing of max-age in SdchManager.

BUG=596541, 596523
==========

[commit-bot: I haz the power, 2016-04-12 19:39:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-12 19:40:59]

Description was changed from

==========
Fix parsing of max-age in SdchManager.

BUG=596541, 596523
==========

to

==========
Fix parsing of max-age in SdchManager.

BUG=596541, 596523

Committed: https://crrev.com/38ab932422f88d294a7869ed512c41d6bfec3236
Cr-Commit-Position: refs/heads/master@{#386766}
==========

[commit-bot: I haz the power, 2016-04-12 19:41:00]

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