Add UMA to measure feasibility of making unique names immutable

Add UMA to measure feasibility of making unique names immutable

r418350 changed Chrome so that unique names are fixed once the frame has
committed the first real load. Unfortunately, the bug still occurs if
there are in-page navigations on the initial empty document mixed with
window.name changes, since the first real load has not yet committed.

One possible simplification is to just make unique name completely
immutable. However, it's possible that some sites depend on creating a
browsing context, naming it, and then navigating it. This measures how
many sites depend on that behavior (though it doesn't measure how many
sites might be broken if this behavior changes).

BUG=607205
Committed: https://crrev.com/77df108d260b0f877cfae096bf136397fce6b4f8
Cr-Commit-Position: refs/heads/master@{#423411}

[dcheng, 2016-09-30 20:51:06]

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

[commit-bot: I haz the power, 2016-09-30 20:51:43]

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

[dcheng, 2016-09-30 20:52:18]

dcheng@chromium.org changed reviewers:
+ creis@chromium.org

[dcheng, 2016-09-30 20:56:52]

Btw, it would be nice to try to measure things that would break, but I'm not
sure what a good way would be. I guess we could record an additional metric if
there are additional history commits?

[commit-bot: I haz the power, 2016-09-30 21:44:11]

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

[commit-bot: I haz the power, 2016-09-30 21:44:12]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-10-01 00:05:11]

> Unfortunately, the bug still occurs if
there are in-page navigations mixed with window.name changes.

Can you clarify?  I'm not sure if you're referring to the "before first commit"
case or not, or how the bug would repro.

Also, what would a broken case look like for fully immutable frame names?  The
frame would still have an auto-generated unique name, so I guess it would affect
pages that create their frames in a different order when you come back, such
that they were relying on the assigned name for restore?

https://codereview.chromium.org/2385773003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2385773003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2970: if (!committed_first_load_) {
nit: No braces.

https://codereview.chromium.org/2385773003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:3360: if (!committed_first_load_ &&
!current_history_item_.isNull()) {
Why isn't current_history_item_ enough (here and above)?  Seems like they're
both only set in this method.

[dcheng, 2016-10-01 00:12:13]

Description was changed from

==========
Add UMA to measure feasibility of making unique names immutable

r418350 changed Chrome so that unique names are fixed once the frame has
committed the first real load. Unfortunately, the bug still occurs if
there are in-page navigations mixed with window.name changes.

One possible simplification is to just make unique name completely
immutable. However, it's possible that some sites depend on creating a
browsing context, naming it, and then navigating it. This measures how
many sites depend on that behavior (though it doesn't measure how many
sites might be broken if this behavior changes).

BUG=607205
==========

to

==========
Add UMA to measure feasibility of making unique names immutable

r418350 changed Chrome so that unique names are fixed once the frame has
committed the first real load. Unfortunately, the bug still occurs if
there are in-page navigations on the initial empty document mixed with
window.name changes, since the first real load has not yet committed.

One possible simplification is to just make unique name completely
immutable. However, it's possible that some sites depend on creating a
browsing context, naming it, and then navigating it. This measures how
many sites depend on that behavior (though it doesn't measure how many
sites might be broken if this behavior changes).

BUG=607205
==========

[dcheng, 2016-10-01 20:25:14]

On 2016/10/01 00:05:11, Charlie Reis wrote:
> > Unfortunately, the bug still occurs if
> there are in-page navigations mixed with window.name changes.
> 
> Can you clarify?  I'm not sure if you're referring to the "before first
commit"
> case or not, or how the bug would repro.
> 
> Also, what would a broken case look like for fully immutable frame names?  The
> frame would still have an auto-generated unique name, so I guess it would
affect
> pages that create their frames in a different order when you come back, such
> that they were relying on the assigned name for restore?

Yeah, I've tried to update the CL description and the UMA recording code with
comments to make it clearer. Hopefully it helps?

https://codereview.chromium.org/2385773003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2385773003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2970: if (!committed_first_load_) {
On 2016/10/01 00:05:12, Charlie Reis wrote:
> nit: No braces.

Done.

https://codereview.chromium.org/2385773003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:3360: if (!committed_first_load_ &&
!current_history_item_.isNull()) {
On 2016/10/01 00:05:12, Charlie Reis wrote:
> Why isn't current_history_item_ enough (here and above)?  Seems like they're
> both only set in this method.

I added a comment to try to make the purpose clearer.

[dcheng, 2016-10-04 17:09:10]

ping =)

[Charlie Reis, 2016-10-04 18:15:45]

Sorry for the delay, and thanks for the explanation.  One question below, and
LGTM if you're ok with it.

https://codereview.chromium.org/2385773003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2385773003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3378: if (!committed_first_load_ &&
!current_history_item_.isNull()) {
One possible hiccup: I don't think we get a commit for the initial empty
document if the iframe is created with a slow URL.  (I would love for that to be
fixed, but I don't know what the implications are.)

I think that means you'll miss the case that an iframe is created with no name
and a slow URL, then a name is assigned, then the iframe is navigated to a
different URL.

That's a pretty obscure case, so I'm ok with proceeding if you decide the rest
is worthwhile.

[dcheng, 2016-10-05 20:37:24]

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

[dcheng, 2016-10-05 20:37:24]

I'm going to land this first and work on the followup (always send
didCommitProvisionalLoad) as a separate CL.

+isherman for histogram changes

[Ilya Sherman, 2016-10-05 22:44:35]

histograms.xml lgtm % a nit:

https://codereview.chromium.org/2385773003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2385773003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:55755: +    enum="BooleanSuccess">
Please provide an enum that's more specifically relevant to this histogram, e.g.
with labels "Kept old name" and "Assigned new name", or maybe just
Unchanged/Changed.

[dcheng, 2016-10-05 22:48:44]

https://codereview.chromium.org/2385773003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2385773003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:55755: +    enum="BooleanSuccess">
On 2016/10/05 22:44:35, Ilya Sherman wrote:
> Please provide an enum that's more specifically relevant to this histogram,
e.g.
> with labels "Kept old name" and "Assigned new name", or maybe just
> Unchanged/Changed.

Is there no generic boolean enum type? That's really what I want here, as the
behavior I'm measuring is already captured entirely in the UMA name.

[dcheng, 2016-10-06 01:45:52]

We would like this CL to make the branch cut, so I'm going to go ahead and land.
I'm happy to make any histograms.xml changes in a followup (since I think we
always use the ToT version?)

[dcheng, 2016-10-06 01:45:56]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-10-06 01:46:39]

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, 2016-10-06 02:54:39]

Description was changed from

==========
Add UMA to measure feasibility of making unique names immutable

r418350 changed Chrome so that unique names are fixed once the frame has
committed the first real load. Unfortunately, the bug still occurs if
there are in-page navigations on the initial empty document mixed with
window.name changes, since the first real load has not yet committed.

One possible simplification is to just make unique name completely
immutable. However, it's possible that some sites depend on creating a
browsing context, naming it, and then navigating it. This measures how
many sites depend on that behavior (though it doesn't measure how many
sites might be broken if this behavior changes).

BUG=607205
==========

to

==========
Add UMA to measure feasibility of making unique names immutable

r418350 changed Chrome so that unique names are fixed once the frame has
committed the first real load. Unfortunately, the bug still occurs if
there are in-page navigations on the initial empty document mixed with
window.name changes, since the first real load has not yet committed.

One possible simplification is to just make unique name completely
immutable. However, it's possible that some sites depend on creating a
browsing context, naming it, and then navigating it. This measures how
many sites depend on that behavior (though it doesn't measure how many
sites might be broken if this behavior changes).

BUG=607205
==========

[commit-bot: I haz the power, 2016-10-06 02:54:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-06 02:56:27]

Description was changed from

==========
Add UMA to measure feasibility of making unique names immutable

r418350 changed Chrome so that unique names are fixed once the frame has
committed the first real load. Unfortunately, the bug still occurs if
there are in-page navigations on the initial empty document mixed with
window.name changes, since the first real load has not yet committed.

One possible simplification is to just make unique name completely
immutable. However, it's possible that some sites depend on creating a
browsing context, naming it, and then navigating it. This measures how
many sites depend on that behavior (though it doesn't measure how many
sites might be broken if this behavior changes).

BUG=607205
==========

to

==========
Add UMA to measure feasibility of making unique names immutable

r418350 changed Chrome so that unique names are fixed once the frame has
committed the first real load. Unfortunately, the bug still occurs if
there are in-page navigations on the initial empty document mixed with
window.name changes, since the first real load has not yet committed.

One possible simplification is to just make unique name completely
immutable. However, it's possible that some sites depend on creating a
browsing context, naming it, and then navigating it. This measures how
many sites depend on that behavior (though it doesn't measure how many
sites might be broken if this behavior changes).

BUG=607205

Committed: https://crrev.com/77df108d260b0f877cfae096bf136397fce6b4f8
Cr-Commit-Position: refs/heads/master@{#423411}
==========

[commit-bot: I haz the power, 2016-10-06 02:56:29]

Patchset 2 (id:??) landed as
https://crrev.com/77df108d260b0f877cfae096bf136397fce6b4f8
Cr-Commit-Position: refs/heads/master@{#423411}

[Ilya Sherman, 2016-10-06 19:08:31]

Sorry for missing your previous reply the first time around.

https://codereview.chromium.org/2385773003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2385773003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:55755: +    enum="BooleanSuccess">
On 2016/10/05 22:48:44, dcheng wrote:
> On 2016/10/05 22:44:35, Ilya Sherman wrote:
> > Please provide an enum that's more specifically relevant to this histogram,
> e.g.
> > with labels "Kept old name" and "Assigned new name", or maybe just
> > Unchanged/Changed.
> 
> Is there no generic boolean enum type? That's really what I want here, as the
> behavior I'm measuring is already captured entirely in the UMA name.

I agree that the name says a lot, but it's still easier to read the data if the
buckets have clear labels.  I guess it's not critical, but I'd definitely
encourage you to land a CL with more semantically contentful labels for the
buckets.