Avoid mutating frame unique name after first real commit.

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 5742191df101fb60dd771901f535fb76bb6c1d27..5e60c1cff2a7b2a8ba3c595be631b473ad4cd364 100644
--- a/third_party/WebKit/Source/core/page/FrameTree.cpp
+++ b/third_party/WebKit/Source/core/page/FrameTree.cpp
@@ -62,7 +62,7 @@ void FrameTree::setName(const AtomicString& name)
     // This assert helps ensure that early return (a few lines below) won't
     // result in an uninitialized m_uniqueName.
     DCHECK(!m_uniqueName.isNull()
-        || (m_uniqueName.isNull() && m_name.isNull() && !parent()));
+        || (m_uniqueName.isNull() && !parent()));

[Łukasz Anforowicz, 2016/09/09 16:21:38]

m_name and m_uniqueName are no longer in-sync now (because m_uniqueName is
immutable, but m_name can keep changing).  Therefore we need to relax the DCHECK
above.

 
     // Do not recalculate m_uniqueName if there is no real change of m_name.
     // This is not just a performance optimization - other code relies on the
@@ -73,33 +73,31 @@ void FrameTree::setName(const AtomicString& name)
 
     m_name = name;
 
-    // Remove our old frame name so it's not considered in calculateUniqueNameForChildFrame
-    // and appendUniqueSuffix calls below.
+    // https://crbug.com/607205: Make sure m_uniqueName doesn't change after
+    // initial navigation - session history depends on this.
+    if (toLocalFrame(m_thisFrame)->loader().stateMachine()->committedFirstRealDocumentLoad())
+        return;
+
+    // Leave main frame's unique name set to a null string.
+    if (!parent())
+        return;

[Łukasz Anforowicz, 2016/09/09 16:21:38]

I was wondering whether it might be a good idea to (in DCHECK_IS_ON builds?)
assign random garbage to m_uniqueName in case of a main frame (to double-check
that nobody looks at the unique name of a main frame).

[dcheng, 2016/09/09 22:00:13]

I think this is fine (and if they're using it, that's too bad)

+
+    // 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)
 {
-    if (!parent()) {
-        DCHECK(uniqueName == name);

[Łukasz Anforowicz, 2016/09/09 16:21:38]

We can no longer have the DCHECK above, because after this CL name and
unique-name are no longer kept in-sync (because m_uniqueName is immutable, but
m_name can keep changing).

-    } else {
-        DCHECK(!uniqueName.isEmpty());
-    }
-
     m_name = name;
 
+    // Non-main frames should have a non-empty unique name.
+    DCHECK(!parent() || !uniqueName.isEmpty());
+
     // TODO(lukasza): We would like to assert uniqueness below (i.e. by calling
     // setUniqueName), but
     // 1) uniqueness is currently violated by provisional/old frame pairs.