[Sync] Ensure referrer is recorded regardless of policy conversion

[Sync] Ensure referrer is recorded regardless of policy conversion

Previously recording the referrer was gated on logic checking if the referrer
policy could be mapped to an old value. Whether the policy maps or not should
not affect whether the referrer is set or not, just whether the obsolete
value is set or not.

BUG=729159
Review-Url: https://codereview.chromium.org/2921883003
Cr-Commit-Position: refs/heads/master@{#479088}
Committed: https://chromium.googlesource.com/chromium/src/+/59a1748c0349df8961742469156763529134c634

[Nicolas Zea, 2017-06-02 21:06:39]

zea@chromium.org changed reviewers:
+ jochen@chromium.org

[Nicolas Zea, 2017-06-02 21:06:40]

Jochen, I'm assuming that the change previously (in
https://codereview.chromium.org/869613006) was an oversight, and referrer was
not meant to be conditionally written. Let me know if that doesn't seem correct.

[Nicolas Zea, 2017-06-02 21:06:49]

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

[commit-bot: I haz the power, 2017-06-02 21:07:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-06-02 21:29:05]

no, that's not an oversight :/

if we can't map the referrer policy to the old values, we decided to strip the
referrer (so old clients don't end up with an referrer that doesn't match the
policy).

I think the correct solution is to introduce yet another field
"correct_referrer" and use that to unconditionally send the referrer url for new
clients

does that make sense?

[Nicolas Zea, 2017-06-02 22:15:16]

Ah, it makes sense why my choice to conditionally write the
obsolete_referrer_policy is wrong then.

But why would it be wrong to write both unconditionally, and simply have the
obsolete policy fall back to kObsoleteReferrerPolicyNever if we can't convert
(like MapReferrerPolicyToOldValues already does)? Do old clients not ensure the
referrer isn't treated properly when the policy is Never?

[commit-bot: I haz the power, 2017-06-02 23:00:27]

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

[commit-bot: I haz the power, 2017-06-02 23:00:28]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jochen (gone - plz use gerrit), 2017-06-05 06:05:52]

On 2017/06/02 at 22:15:16, zea wrote:
> Ah, it makes sense why my choice to conditionally write the
obsolete_referrer_policy is wrong then.
> 
> But why would it be wrong to write both unconditionally, and simply have the
obsolete policy fall back to kObsoleteReferrerPolicyNever if we can't convert
(like MapReferrerPolicyToOldValues already does)? Do old clients not ensure the
referrer isn't treated properly when the policy is Never?

yeah, so what happened was this:

when I first introduced referrer policies, I would only check on redirect, so it
was possible to set a non-empty referrer together with the policy "never".

Then at some point, I reviewed a CL that added new policies, but it sorted the
enum alphabetically. This meant that old and new clients that synced to the same
account now had different policies for the same navigation entry.

Even later, I added verification code the network requests, and found that a
large number of requests had incorrect referrers (i.e. contradicting the
referrer policy).

To fix this, I renamed the existing referrer policy to obsolete referrer policy,
and added a new field. For cases where the referrer policy before and after
alphabetically sorting agree, I'd translate the obsolete referrer policy to the
new value, and for other cases, I'd use "never". For newer clients, I introduced
a separate field where the real referrer policy is always stored.

Now it looks this fix was incomplete, and I should have also always stored the
real referrer.

does that make sense?

I guess we could also argue that we no longer support so old clients, and just
always store the real referrer and real policy and rip out the legacy logic?

[Nicolas Zea, 2017-06-05 20:20:13]

On 2017/06/05 06:05:52, jochen wrote:
> On 2017/06/02 at 22:15:16, zea wrote:
> > Ah, it makes sense why my choice to conditionally write the
> obsolete_referrer_policy is wrong then.
> > 
> > But why would it be wrong to write both unconditionally, and simply have the
> obsolete policy fall back to kObsoleteReferrerPolicyNever if we can't convert
> (like MapReferrerPolicyToOldValues already does)? Do old clients not ensure
the
> referrer isn't treated properly when the policy is Never?
> 
> yeah, so what happened was this:
> 
> when I first introduced referrer policies, I would only check on redirect, so
it
> was possible to set a non-empty referrer together with the policy "never".
> 
> Then at some point, I reviewed a CL that added new policies, but it sorted the
> enum alphabetically. This meant that old and new clients that synced to the
same
> account now had different policies for the same navigation entry.
> 
> Even later, I added verification code the network requests, and found that a
> large number of requests had incorrect referrers (i.e. contradicting the
> referrer policy).
> 
> To fix this, I renamed the existing referrer policy to obsolete referrer
policy,
> and added a new field. For cases where the referrer policy before and after
> alphabetically sorting agree, I'd translate the obsolete referrer policy to
the
> new value, and for other cases, I'd use "never". For newer clients, I
introduced
> a separate field where the real referrer policy is always stored.
> 
> Now it looks this fix was incomplete, and I should have also always stored the
> real referrer.
> 
> does that make sense?
> 
> I guess we could also argue that we no longer support so old clients, and just
> always store the real referrer and real policy and rip out the legacy logic?

Yeah, I think given how long this logic has been in place, it's safe to say
we're willing to break backwards compatibility. For reference, in situations
where we migrate a synced field, such as with a preference, we typically only
ensure it remains backwards compatible for 2-3 milestones. Will upload a CL that
removes all the legacy logic.

[Nicolas Zea, 2017-06-10 00:26:46]

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

[Nicolas Zea, 2017-06-10 00:26:53]

PTAL

[commit-bot: I haz the power, 2017-06-10 00:27:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-10 01:31:59]

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

[commit-bot: I haz the power, 2017-06-10 01:31:59]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-06-12 08:38:07]

lgtm

[Nicolas Zea, 2017-06-13 17:20:34]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2017-06-13 17:20:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 19:04:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497374434159660,
"parent_rev": "e635c3f42d2634a3335fea3671efeb95b3b898da", "commit_rev":
"59a1748c0349df8961742469156763529134c634"}

[commit-bot: I haz the power, 2017-06-13 19:04:43]

Description was changed from

==========
[Sync] Ensure referrer is recorded regardless of policy conversion

Previously recording the referrer was gated on logic checking if the referrer
policy could be mapped to an old value. Whether the policy maps or not should
not affect whether the referrer is set or not, just whether the obsolete
value is set or not.

BUG=729159
==========

to

==========
[Sync] Ensure referrer is recorded regardless of policy conversion

Previously recording the referrer was gated on logic checking if the referrer
policy could be mapped to an old value. Whether the policy maps or not should
not affect whether the referrer is set or not, just whether the obsolete
value is set or not.

BUG=729159

Review-Url: https://codereview.chromium.org/2921883003
Cr-Commit-Position: refs/heads/master@{#479088}
Committed:
https://chromium.googlesource.com/chromium/src/+/59a1748c0349df89617424691567...
==========

[commit-bot: I haz the power, 2017-06-13 19:04:45]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/59a1748c0349df89617424691567...