Try harder to make sure that blink::FrameTree::m_uniqueName is truly unique.

Try harder to make sure that blink::FrameTree::m_uniqueName is truly unique.

After this CL, blink::FrameTree::m_uniqueName is guaranteed to be truly
unique (within a given frame tree) except when:
- The frame tree temporarily contains both an old and a new provisional frame.
- The frame tree spans multiple renderer processes, which race to assign
  the same name to different frames (https://crbug.com/558680#c14).

BUG=588800
Committed: https://crrev.com/201638819dd4aa0c3b18bf3fe64c3a8a6256c44c
Cr-Commit-Position: refs/heads/master@{#397443}

[Łukasz Anforowicz, 2016-05-11 23:11:25]

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

[Łukasz Anforowicz, 2016-05-11 23:11:26]

Daniel, could you please take a look?

I am not very happy with the generate-checkForCollisions-retryIfNeeded approach
(especially given O(N) cost of uniqueness verification), but I think this is
necessary given the constraints we have (especially around avoiding changing
uniqueName generated after window.name assignment).

Note that there are also relevant recent notes in https://crbug.com/588800 and
in 
https://crbug.com/558680#c14.

[dcheng, 2016-05-12 05:31:18]

https://codereview.chromium.org/1968213002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/1968213002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FrameTree.cpp:220: } while
(uniqueNameExists(uniqueNameBuilder.toAtomicString()));
So according to the spec, a valid name cannot start with a '_'. Obviously that's
not enforced in Blink today, but is it possible to set a browsing context name
to a name that starts with an underscore in other browsers?

I ask this because it seems like it'd be nicer to just put unique names like
this in a separate namespace.

Alternatively, is there any way to retroactively add namespaces to the unique
names so they can't collide?

e.g.

name:foobar (if specified by the page, and not autogenerated)
gen:<!--frame1//frame2-->--> (if generated)

[Łukasz Anforowicz, 2016-05-12 15:39:39]

On 2016/05/12 05:31:18, dcheng wrote:
>
https://codereview.chromium.org/1968213002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/page/FrameTree.cpp (right):
> 
>
https://codereview.chromium.org/1968213002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/page/FrameTree.cpp:220: } while
> (uniqueNameExists(uniqueNameBuilder.toAtomicString()));
> So according to the spec, a valid name cannot start with a '_'. Obviously
that's
> not enforced in Blink today, but is it possible to set a browsing context name
> to a name that starts with an underscore in other browsers?
> 
> I ask this because it seems like it'd be nicer to just put unique names like
> this in a separate namespace.
> 
> Alternatively, is there any way to retroactively add namespaces to the unique
> names so they can't collide?
> 
> e.g.
> 
> name:foobar (if specified by the page, and not autogenerated)
> gen:<!--frame1//frame2-->--> (if generated)

We could enforce separate namespaces for assigned-vs-generated names by treating
"<!--frame" as a reserved substring (currently this substring is used in all
generated names).  When this substring would appear in an assigned name we would
either 1) fall back to generating a name (my preference) or 2) substitute this
string into something else (being careful that the reserved string cannot
reappear at the boundaries of the replacement).  I claim that "<!--frame"
substring is rare and therefore changing unique names in this rare case would
cause only minimal backcompatibility risk.

We could also avoid collisions within generated names (but only for newly added
frames), by introducing and using
FrameHost::m_subframeMonotonicallyIncreasingSequenceNumber (instead of using
FrameTree::childCount() as today).

But - even when doing the 2 things above, we would still have collisions in some
cases:
1. main frame's assigned name colliding with another assigned name.
2. subframe's assigned name colliding with another assigned name.
Currently the second case falls back to using a generated name (and I think
keeping this fallback behavior is unavoidable because of backcompatibility
concerns).  Unfortunately, falling back to a generated name in this case means
that m_subframeMonotonicallyIncreasingSequenceNumber doesn't guarantee
uniqueness anymore (and because of backcompatibility concerns we cannot change
m_subframeMonotonicallyIncreasingSequenceNumber when not adding a new frame,
because it would make it diverge in too many cases from the currently used
childCount).

So - 1) separate namespaces + 2) m_subframeMonotonicallyIncreasingSequenceNumber
don't necessarily make it easier to avoid collisions (if what I wrote above is
correct).  Therefore, I think that we shouldn't be introducing any of these 2
things and can get by with generate-checkUniqueness-retryIfNeeded approach
(which I think is unavoidable given backcompatibility requirements).

[Łukasz Anforowicz, 2016-05-13 20:34:41]

On 2016/05/12 15:39:39, Łukasz Anforowicz wrote:
> On 2016/05/12 05:31:18, dcheng wrote:
> >
>
https://codereview.chromium.org/1968213002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/page/FrameTree.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1968213002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/page/FrameTree.cpp:220: } while
> > (uniqueNameExists(uniqueNameBuilder.toAtomicString()));
> > So according to the spec, a valid name cannot start with a '_'. Obviously
> that's
> > not enforced in Blink today, but is it possible to set a browsing context
name
> > to a name that starts with an underscore in other browsers?
> > 
> > I ask this because it seems like it'd be nicer to just put unique names like
> > this in a separate namespace.
> > 
> > Alternatively, is there any way to retroactively add namespaces to the
unique
> > names so they can't collide?
> > 
> > e.g.
> > 
> > name:foobar (if specified by the page, and not autogenerated)
> > gen:<!--frame1//frame2-->--> (if generated)
> 
> We could enforce separate namespaces for assigned-vs-generated names by
treating
> "<!--frame" as a reserved substring (currently this substring is used in all
> generated names).  When this substring would appear in an assigned name we
would
> either 1) fall back to generating a name (my preference) or 2) substitute this
> string into something else (being careful that the reserved string cannot
> reappear at the boundaries of the replacement).  I claim that "<!--frame"
> substring is rare and therefore changing unique names in this rare case would
> cause only minimal backcompatibility risk.
> 
> We could also avoid collisions within generated names (but only for newly
added
> frames), by introducing and using
> FrameHost::m_subframeMonotonicallyIncreasingSequenceNumber (instead of using
> FrameTree::childCount() as today).
> 
> But - even when doing the 2 things above, we would still have collisions in
some
> cases:
> 1. main frame's assigned name colliding with another assigned name.
> 2. subframe's assigned name colliding with another assigned name.
> Currently the second case falls back to using a generated name (and I think
> keeping this fallback behavior is unavoidable because of backcompatibility
> concerns).  Unfortunately, falling back to a generated name in this case means
> that m_subframeMonotonicallyIncreasingSequenceNumber doesn't guarantee
> uniqueness anymore (and because of backcompatibility concerns we cannot change
> m_subframeMonotonicallyIncreasingSequenceNumber when not adding a new frame,
> because it would make it diverge in too many cases from the currently used
> childCount).
> 
> So - 1) separate namespaces + 2)
m_subframeMonotonicallyIncreasingSequenceNumber
> don't necessarily make it easier to avoid collisions (if what I wrote above is
> correct).  Therefore, I think that we shouldn't be introducing any of these 2
> things and can get by with generate-checkUniqueness-retryIfNeeded approach
> (which I think is unavoidable given backcompatibility requirements).

I've talked with Daniel and AFAIU the concern is not really with
generate-and-retryIfNeeded approach, but more with using random numbers in the
"retry" part.  I see how avoiding random numbers might be desirable (to try to
salvage session restorability in case of colliding names).

The only reason I chose to retry with a random number was to pick a new suffix
with a minimal chance of a repeated collision (and doing this locally, without
the need to coordinate picking of this unique suffix across multiple frames). 
If there was a per-frame-tree m_collisionsCounter, then we could append it
(instead of the random number), but m_collisionsCounter would be challenging to
replicate across OOPIFs.  

Thinking about this more, I think that a frame's absolute position in the tree
can be used to generate a string with low chance of collisions.  Something like:
  StringBuilder hopefullyUniqueSuffix;
  while (parent = frame->parent()) {
    hopefullyUniqueSuffix.append('-');
    hopefullyUniqueSuffix.appendNumber(frame-position-in-the-parent);  // not
yet sure if this is O(1)...
  } // but overall this has be to be at most O(N), where N is the number of
frames in the tree.

Does that seem reasonable compared to the random numbers approach?  Pragmatic or
maybe too magical? :-)

FWIW, performance-wise this seems ok (collision detection is
O(number-of-frames-in-frame-tree) and calculation above is
O(depth-of-frame-tree) [I think] and we always have depth-of-frame-tree <=
number-of-frame-in-frame-tree).

[Łukasz Anforowicz, 2016-05-16 21:32:55]

Daniel - can you take another look please?  In the last patchset I've removed
random numbers from the unique name generation code.

[dcheng, 2016-05-26 06:43:39]

Sorry for not replying. I've been thinking about this because I (perhaps hoping
for the impossible) still finding myself wishing that we could make it so that
our generated names never collide with page-specified names: that way we don't
need to have things like "likelyUniqueSuffix".

Here's my strawman proposal for how to do this:
1. Unique names derived from user-specified names can never contain <!--
sequences. We can do this by escaping it or stripping it from the originally
specified name.
2. Once it's impossible to collide with our special prefixes, we can stick our
disambiguation suffix in a special <!-- --> block as well.

* If removing all <!-- sequences sounds too radical, then we can just do
<!--framePath, <!--frame, and whatever the disambiguation one is called. But
reserving this prefix generically in unique name generation makes sense to me,
and isn't likely to break a lot of content.

Finally, we should add a comment block somewhere that describes the format of
the generated unique name.

Also, one other thought: is it important to use the number of children before
the current node? When inserting to the child frames, we only ever append, so
isn't that just going to be the same as childCount()?

[Łukasz Anforowicz, 2016-05-26 17:32:32]

On 2016/05/26 06:43:39, dcheng wrote:
> Sorry for not replying. I've been thinking about this because I (perhaps
hoping
> for the impossible) still finding myself wishing that we could make it so that
> our generated names never collide with page-specified names
> that way we don't

The problem is generated names colliding with other generated names.
For an example, see 2 scenarios at the end of my reply.

I agree with you that we can come up with a generated names scheme that will
guarantee uniqueness on the first try.  I am skeptical that we can do this while
preserving backcompatibility.  The
generateUsingOldApproach-verifyUniqueness-retryWithNewHighlyUniqueSuffixes
approach of this CL should minimize cases when we end up with a different
uniqueNames than in the old algorithm.

> need to have things like "likelyUniqueSuffix".

Is the concern the extra computation time spent when generating a unique name?
Or aesthetics / maintenance considerations?

> 
> Here's my strawman proposal for how to do this:
> 1. Unique names derived from user-specified names can never contain <!--
> sequences. We can do this by escaping it or stripping it from the originally
> specified name.
> 2. Once it's impossible to collide with our special prefixes, we can stick our
> disambiguation suffix in a special <!-- --> block as well.
> 
> * If removing all <!-- sequences sounds too radical, then we can just do
> <!--framePath, <!--frame, and whatever the disambiguation one is called. But
> reserving this prefix generically in unique name generation makes sense to me,
> and isn't likely to break a lot of content.
> 
> Finally, we should add a comment block somewhere that describes the format of
> the generated unique name.

Fair feedback.  Let's agree on the format of unique names and then I can add a
comment.
> 
> Also, one other thought: is it important to use the number of children before
> the current node? When inserting to the child frames, we only ever append, so
> isn't that just going to be the same as childCount()?

We don't append in 2 cases:

1) We don't append when we need to generate unique name if there is a collission
when a assigning a name to an already existing frame.  i.e. frame A has a
sibling with generatedName-childCount2 and frame A is assigned a name (via
window.name = ...) that collides with some other frame elsewhere in the tree. 
In this case we need to generate a unique name for frame A - we cannot use
childCount, because the sibling already has
uniqueName=generatedName-childCount2.

2) Scenario from the bug - 5 siblings, last sibling has a generated name -
generatedName-childCount5.  Any of the previous siblings is deleted and a new
sibling is inserted - we cannot use childCount when generating the name for the
new sibling, because generatedName-childCount5 is already taken.

[Łukasz Anforowicz, 2016-05-26 18:27:58]

I've tried to gather scenarios that I hope will be useful to keep in mind when
discussing the unique name generation algorithm:
https://docs.google.com/document/d/1orcNi0HEcIb7HRlanS3q-qdbnwu-L23rdAzNP-kxg4U

[Łukasz Anforowicz, 2016-05-27 16:55:27]

Daniel, I have:

1) Added a new test that with old code would repro a conflict ("foo/bar"
ancestor chain VS single name) with just a static html.

2) Tweaked format of the new unique suffix, so it is easier to find/delete if
needed (the format of the suffix looks now like this: "<!--framePosition...-->"
and is symmetrical to the old format: "<!--framePath...-->").

3) Added some comments (including the big comment trying to describe the overall
format - see also my notes next to this big comment).

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.cpp:287: // Description of the
current unique name format
I am not sure if I am happy with the comment here:

1. This is based on reverse-engineering and it is quite likely that I've made
   some mistakes.

2. For ultimate format answer, one has to inspect the code anyway.

So - we might want to discuss:

A) whether to keep the documentation at all

B) whether to have the documentation here or elsewhere (e.g. in the doc linked
from the bug)

C) whether to tweak the code to make the documentation/format prettier, at the
cost of making the code slightly more complex (i.e. oldGeneratedName doesn't
apply to main frame - for non-unique main frame's name the code concatenates
assignedName and newUniqueSuffix).

My opinion:

A) no opinion

B) slight preference for keeping the doc next to the code

C) strong preference toward keeping the code simple
I can also move the description to the doc and link from here.

[dcheng, 2016-05-31 19:57:48]

Just a few nits. Also, I'm wondering... is it possible to just make
ensureUniquenessOfUniqueName handle the generateLikelyUniqueSuffix stuff
internally?

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.cpp:213: if (!child) {
When will child be null?

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.cpp:287: // Description of the
current unique name format
On 2016/05/27 at 16:55:27, Łukasz Anforowicz wrote:
> I am not sure if I am happy with the comment here:
> 
> 1. This is based on reverse-engineering and it is quite likely that I've made
>    some mistakes.
> 
> 2. For ultimate format answer, one has to inspect the code anyway.
> 
> So - we might want to discuss:
> 
> A) whether to keep the documentation at all
> 
> B) whether to have the documentation here or elsewhere (e.g. in the doc linked
from the bug)
> 
> C) whether to tweak the code to make the documentation/format prettier, at the
cost of making the code slightly more complex (i.e. oldGeneratedName doesn't
apply to main frame - for non-unique main frame's name the code concatenates
assignedName and newUniqueSuffix).
> 
> My opinion:
> 
> A) no opinion
> 
> B) slight preference for keeping the doc next to the code
> 
> C) strong preference toward keeping the code simple
> I can also move the description to the doc and link from here.

This is fine. It's far better than not having anything at all.

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FrameTree.h (right):

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.h:46: // The value should be
treated (at least outside of FrameTree.cpp) as an
I would just condense this down to "The value should be treated as an
unstructured, opaque string."

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.h:94: AtomicString
generateOldStyleMaybeUniqueName(bool existingChildFrame) const;
Nit: generateUniqueNameCandidate?

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.h:99: String
generateLikelyUniqueSuffix(Frame* child) const;
Nit: generateFramePosition?

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.h:104: AtomicString
ensureUniquenessOfUniqueName(
Nit: generateUniqueNameFromCandidate

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.h:105: const AtomicString&
notYetUniqueName,
Nit: candidate

[Łukasz Anforowicz, 2016-05-31 23:50:20]

Daniel, can you take another look please?

On 2016/05/31 19:57:48, dcheng wrote:
> ...
> I'm wondering... is it possible to just make
> ensureUniquenessOfUniqueName handle the generateLikelyUniqueSuffix stuff
> internally?

This is a little bit difficult.

Currently ensureUniquenessOfUniqueName (aka appendUniqueSuffix) always appends a
likelyUniqueSuffix.  This is undesirable, unless the original name was not
unique.  This can obviously be solved by moving the call to
generateLikelyUniqueSuffix (aka generateFramePosition) to happen *inside*
ensureUniquenessOfUniqueName + only happen when the initial candidate was not
unique.  But ensureUniquenessOfUniqueName needs to work for all frames and so
the problem is that generateLikelyUniqueSuffix doesn't work for the main frame
(current code passes hardcoded likelyUniqueSuffix for main frame from
FrameTree::setName).

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.cpp:213: if (!child) {
On 2016/05/31 19:57:47, dcheng wrote:
> When will child be null?

|child| will be null in cases when we need to calculate unique name *before* a
child frame is created (i.e. before we call m_client->createChildFrame).  Such
cases have been introduced by https://codereview.chromium.org/1635873003/
(Replicating WebFrame::uniqueName across renderers).  Right now the only call to
FrameTree::calculateUniqueNameForNewChildFrame is in
WebLocalFrameImpl::createChildFrame.

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.cpp:287: // Description of the
current unique name format
On 2016/05/31 19:57:47, dcheng wrote:
> On 2016/05/27 at 16:55:27, Łukasz Anforowicz wrote:
> > I am not sure if I am happy with the comment here:
> > 
> > 1. This is based on reverse-engineering and it is quite likely that I've
made
> >    some mistakes.
> > 
> > 2. For ultimate format answer, one has to inspect the code anyway.
> > 
> > So - we might want to discuss:
> > 
> > A) whether to keep the documentation at all
> > 
> > B) whether to have the documentation here or elsewhere (e.g. in the doc
linked
> from the bug)
> > 
> > C) whether to tweak the code to make the documentation/format prettier, at
the
> cost of making the code slightly more complex (i.e. oldGeneratedName doesn't
> apply to main frame - for non-unique main frame's name the code concatenates
> assignedName and newUniqueSuffix).
> > 
> > My opinion:
> > 
> > A) no opinion
> > 
> > B) slight preference for keeping the doc next to the code
> > 
> > C) strong preference toward keeping the code simple
> > I can also move the description to the doc and link from here.
> 
> This is fine. It's far better than not having anything at all.

Acknowledged.

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FrameTree.h (right):

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.h:46: // The value should be
treated (at least outside of FrameTree.cpp) as an
On 2016/05/31 19:57:47, dcheng wrote:
> I would just condense this down to "The value should be treated as an
> unstructured, opaque string."

Done.  Let me know if you think I should also remove pointer to the comment
describing internal implementation details (i.e. remove the last 2 lines of the
comment here).

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.h:94: AtomicString
generateOldStyleMaybeUniqueName(bool existingChildFrame) const;
On 2016/05/31 19:57:47, dcheng wrote:
> Nit: generateUniqueNameCandidate?

Done.

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.h:99: String
generateLikelyUniqueSuffix(Frame* child) const;
On 2016/05/31 19:57:47, dcheng wrote:
> Nit: generateFramePosition?

Done.

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.h:104: AtomicString
ensureUniquenessOfUniqueName(
On 2016/05/31 19:57:47, dcheng wrote:
> Nit: generateUniqueNameFromCandidate

How about
    // Concatenates |prefix|, |likelyUniqueSuffix| (and additional, internally
    // generated suffix) until the result is a unique name, that doesn't exist
    // elsewhere in the frame tree.  Returns the unique name built in this way.
    AtomicString appendUniqueSuffix(
        const AtomicString& prefix,
        const String& likelyUniqueSuffix) const;
?

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.h:105: const AtomicString&
notYetUniqueName,
On 2016/05/31 19:57:47, dcheng wrote:
> Nit: candidate

How about |prefix| (we use |uniqueNameCandidate| as a variable name inside the
method's implementation, so |candidate| is a bit confusing).

[dcheng, 2016-06-01 20:58:11]

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FrameTree.h (right):

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.h:46: // The value should be
treated (at least outside of FrameTree.cpp) as an
On 2016/05/31 at 23:50:20, Łukasz Anforowicz wrote:
> On 2016/05/31 19:57:47, dcheng wrote:
> > I would just condense this down to "The value should be treated as an
> > unstructured, opaque string."
> 
> Done.  Let me know if you think I should also remove pointer to the comment
describing internal implementation details (i.e. remove the last 2 lines of the
comment here).

I think it makes sense to remove that part of the comment as well (it's
generally implied)

https://codereview.chromium.org/1968213002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/1968213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:202: return
uniqueName.toAtomicString();
Let's have this return a String rather than an AtomicString.

https://codereview.chromium.org/1968213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:257: AtomicString
uniqueNameCandidate;
Nit: Omit |uniqueName| here and just call it |candidate|.

https://codereview.chromium.org/1968213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:266: uniqueNameCandidate =
uniqueNameBuilder.toAtomicString();
Let's not convert to an atomic string until we return.

[Łukasz Anforowicz, 2016-06-01 22:31:29]

Daniel, can you take another look please?

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FrameTree.h (right):

https://codereview.chromium.org/1968213002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.h:46: // The value should be
treated (at least outside of FrameTree.cpp) as an
On 2016/06/01 20:58:11, dcheng wrote:
> On 2016/05/31 at 23:50:20, Łukasz Anforowicz wrote:
> > On 2016/05/31 19:57:47, dcheng wrote:
> > > I would just condense this down to "The value should be treated as an
> > > unstructured, opaque string."
> > 
> > Done.  Let me know if you think I should also remove pointer to the comment
> describing internal implementation details (i.e. remove the last 2 lines of
the
> comment here).
> 
> I think it makes sense to remove that part of the comment as well (it's
> generally implied)

Done.

https://codereview.chromium.org/1968213002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/1968213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:202: return
uniqueName.toAtomicString();
On 2016/06/01 20:58:11, dcheng wrote:
> Let's have this return a String rather than an AtomicString.

Done.

https://codereview.chromium.org/1968213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:257: AtomicString
uniqueNameCandidate;
On 2016/06/01 20:58:11, dcheng wrote:
> Nit: Omit |uniqueName| here and just call it |candidate|.

Done.

https://codereview.chromium.org/1968213002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:266: uniqueNameCandidate =
uniqueNameBuilder.toAtomicString();
On 2016/06/01 20:58:11, dcheng wrote:
> Let's not convert to an atomic string until we return.

Done.

[dcheng, 2016-06-01 23:06:03]

LGTM

[Łukasz Anforowicz, 2016-06-02 16:01:29]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-06-02 16:01:58]

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

[commit-bot: I haz the power, 2016-06-02 16:49:25]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-02 16:50:46]

Description was changed from

==========
Try harder to make sure that blink::FrameTree::m_uniqueName is truly unique.

After this CL, blink::FrameTree::m_uniqueName is guaranteed to be truly
unique (within a given frame tree) except when:
- The frame tree temporarily contains both an old and a new provisional frame.
- The frame tree spans multiple renderer processes, which race to assign
  the same name to different frames (https://crbug.com/558680#c14).

BUG=588800
==========

to

==========
Try harder to make sure that blink::FrameTree::m_uniqueName is truly unique.

After this CL, blink::FrameTree::m_uniqueName is guaranteed to be truly
unique (within a given frame tree) except when:
- The frame tree temporarily contains both an old and a new provisional frame.
- The frame tree spans multiple renderer processes, which race to assign
  the same name to different frames (https://crbug.com/558680#c14).

BUG=588800

Committed: https://crrev.com/201638819dd4aa0c3b18bf3fe64c3a8a6256c44c
Cr-Commit-Position: refs/heads/master@{#397443}
==========

[commit-bot: I haz the power, 2016-06-02 16:50:47]

Patchset 7 (id:??) landed as
https://crrev.com/201638819dd4aa0c3b18bf3fe64c3a8a6256c44c
Cr-Commit-Position: refs/heads/master@{#397443}