Main frame's unique name can always be null.

third_party/WebKit/Source/core/page/FrameTree.cpp

Index: third_party/WebKit/Source/core/page/FrameTree.cpp
diff --git a/third_party/WebKit/Source/core/page/FrameTree.cpp b/third_party/WebKit/Source/core/page/FrameTree.cpp
index e5b70f736e45c19dd7c395c724e3aa29e60fc621..0b6dad8382e00dfca29c4735c85c781c8b3089ad 100644
--- a/third_party/WebKit/Source/core/page/FrameTree.cpp
+++ b/third_party/WebKit/Source/core/page/FrameTree.cpp
@@ -78,29 +78,29 @@ void FrameTree::setName(const AtomicString& name)
     if (toLocalFrame(m_thisFrame)->loader().stateMachine()->committedFirstRealDocumentLoad())
         return;
 
-    // Remove our old frame name so it's not considered in calculateUniqueNameForChildFrame
-    // and appendUniqueSuffix calls below.
+    // Leave main frame's unique name set to a null string.
+    if (!parent())
+        return;
+
+    // Remove our old frame name so it's not considered in
+    // calculateUniqueNameForChildFrame call below.
     m_uniqueName = AtomicString();
 
     // Calculate a new unique name based on inputs.
-    if (parent()) {
-        setUniqueName(
-            parent()->tree().calculateUniqueNameForChildFrame(m_thisFrame, name, nullAtom));
-    } else if (name.isEmpty() || !uniqueNameExists(name)) {
-        // Only main frame can have an empty unique name, so for main frames
-        // emptiness guarantees uniquness.
-        setUniqueName(name);
-    } else {
-        setUniqueName(appendUniqueSuffix(name, "<!--framePosition"));
-    }
+    setUniqueName(
+        parent()->tree().calculateUniqueNameForChildFrame(m_thisFrame, name, nullAtom));
 }
 
 void FrameTree::setPrecalculatedName(const AtomicString& name, const AtomicString& uniqueName)
 {
     m_name = name;
 
+    // Leave main frame's unique name set to a null string.
+    if (!parent())

[dcheng, 2016/09/10 02:54:49]

Is this non-empty sometimes for parent frames? I didn't read around that much,
but it seems new views are always create with no name?

[Łukasz Anforowicz, 2016/09/12 23:58:34]

I am not sure if I understand the question, but I think you are asking if new
views can be named right from the start.  AFAICT new views can be named by:

1. window.open(url, window_name) [+ I see that FrameTree::setName is called on
the main frame from createNewWindow function in CreateWindow.cpp]

2. embedder [i.e. I see that ViewMsg_New_Params contains
replicated_frame_state.name + there is
content::WebContents::CreateParams::main_frame_name]

[dcheng, 2016/09/13 00:44:23]

Right, but I don't think the main frame is ever *created* with a name set. In
the window.open case, the name isn't set until later (when we return back out of
content into Blink). In the IPC case, it's not set until a later explicit call
to FrameTree::setName. So I'm wondering if we ever call setPrecalculatedName for
a main frame and |name| won't be null / empty.

[Łukasz Anforowicz, 2016/09/13 01:14:29]

Agreed.
Ack.
Yes - we would use setPrecalculatedName when replicating unique name to another
renderer process (i.e. via WebRemoteFrameImpl::setReplicatedName and [for
example] RenderFrameProxy::OnDidUpdateName).  In this case name and unique name
might not be null / empty.

[dcheng, 2016/09/13 01:38:00]

Right, but the unique name we calculated in that case ought to be empty anyway.
Basically, what I'm wondering: if we didn't have this if () statement, what
would break?

[Łukasz Anforowicz, 2016/09/13 16:45:14]

It turns out [1] that when setPrecalculatedName is called during swap-out [2],
then the parent is not yet set, but obviously the unique name (coming from a
subframe / non-main-frame) can be not empty.

So:
- The early return is wrong / would mess up unique names during swap-out.  I
probably should spend some time coming up with extra test coverage (because the
first patchset should not be green I think).
- The last patchset contains corrected FrameTree.cpp changes.

[1] https://codereview.chromium.org/2329523003/#ps20001 and
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

[2] #2 0x000003d309aa blink::FrameTree::setPrecalculatedName()@@@
#3 0x00000328fd54 blink::WebRemoteFrameImpl::setReplicatedName()@@@
#4 0x000002f283f5 content::RenderFrameProxy::SetReplicatedState()@@@
#5 0x000002f051ef content::RenderFrameImpl::OnSwapOut()@@@

+        return;
+
     // Non-main frames should have a non-empty unique name.
-    DCHECK(!parent() || !uniqueName.isEmpty());
+    DCHECK(!uniqueName.isEmpty());
 
     // TODO(lukasza): We would like to assert uniqueness below (i.e. by calling
     // setUniqueName), but
@@ -313,10 +313,9 @@ AtomicString FrameTree::calculateUniqueNameForChildFrame(
     // backcompatibility-aware approach has resulted so far in the following
     // rather baroque format... :
     //
-    //   uniqueName ::= <assignedName> | <generatedName>
+    //   uniqueName ::= <nullForMainFrame> | <assignedName> | <generatedName>
     //                 (generatedName is used if assignedName is
-    //                 1) non-unique / conflicts with other frame's unique name or
-    //                 2) assignedName is empty for a non-main frame)
+    //                 non-unique / conflicts with other frame's unique name.
     //
     //   assignedName ::= value of iframe's name attribute
     //                 or value assigned to window.name (*before* the first
@@ -327,9 +326,7 @@ AtomicString FrameTree::calculateUniqueNameForChildFrame(
     //                  not unique after all)
     //
     //   oldGeneratedName ::= "<!--framePath //" ancestorChain "/<!--frame" childCount "-->-->"
-    //                  (main frame is special - oldGeneratedName for main frame
-    //                  is always the frame's assignedName;  oldGeneratedName is
-    //                  generated by generateUniqueNameCandidate method).
+    //                  (oldGeneratedName is generated by generateUniqueNameCandidate method).
     //
     //   childCount ::= current number of siblings
     //
@@ -344,20 +341,14 @@ AtomicString FrameTree::calculateUniqueNameForChildFrame(
     //   newUniqueSuffix ::= "<!--framePosition" framePosition "/" retryNumber "-->"
     //
     //   framePosition ::= "-" numberOfSiblingsBeforeChild [ framePosition-forParent? ]
-    //                   | <empty string for main frame>
     //
     //   retryNumber ::= smallest non-negative integer resulting in unique name
 }
 
 void FrameTree::setUniqueName(const AtomicString& uniqueName)
 {
-    // Main frame is the only frame that can have an empty unique name.
-    if (parent()) {
-        DCHECK(!uniqueName.isEmpty() && !uniqueNameExists(uniqueName));
-    } else {
-        DCHECK(uniqueName.isEmpty() || !uniqueNameExists(uniqueName));
-    }
-
+    DCHECK(parent()); // Only subframes can have a custom-tailored unique name.
+    DCHECK(!uniqueName.isEmpty() && !uniqueNameExists(uniqueName));
     m_uniqueName = uniqueName;
 }