Cleanup around LocalFrame initialization

third_party/WebKit/Source/web/WebLocalFrameImpl.cpp

Index: third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
diff --git a/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp b/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
index 32d805f9a5ef089148ec60050f926c431ad18fe0..3fd731b94b9831b1332ab32cd3b5ed368aa794df 100644
--- a/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
+++ b/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
@@ -1559,26 +1559,16 @@ WebLocalFrameImpl* WebLocalFrameImpl::CreateProvisional(
   // When a core Frame is created with no owner, it attempts to set itself as
   // the main frame of the Page. However, this is a provisional frame, and may
   // disappear, so Page::m_mainFrame can't be updated just yet.
-  FrameOwner* temp_owner = DummyFrameOwner::Create();
-  // TODO(dcheng): This block is very similar to initializeCoreFrame. Try to

[Nate Chapin, 2017/05/10 16:33:20]

InitializeCoreFrame() does a couple of things CreateProvisional didn't, but it
all looks benign to me.

[dcheng, 2017/05/11 21:36:25]

I think my main concern with structuring it this way is that we're calling
Init() with an incorrect owner: won't this affect things like sandbox flags on
the initial empty document?

[dcheng, 2017/05/12 18:06:38]

Btw, I talked with Alex about this, and I think it'd probably be worth a comment
noting that this is an issue (we won't inherit the right set of things with the
DummyOwner), but that it's not an issue because the initial empty Document on a
provisional local frame is not reachable via script, and is essentially inert.
Since we do set the right FrameOwner just a few lines later, any subsequent
loads will "just do the right thing"

[Nate Chapin, 2017/05/12 19:52:28]

Done.

-  // reuse it here.
-  LocalFrame* frame = LocalFrame::Create(
-      web_frame->local_frame_client_impl_.Get(), *old_frame->GetPage(),
-      temp_owner, interface_provider, interface_registry);
-  frame->Tree().SetName(
-      ToWebRemoteFrameImpl(old_web_frame)->GetFrame()->Tree().GetName());
-  web_frame->SetCoreFrame(frame);
-
-  frame->SetOwner(old_frame->Owner());
-
-  if (frame->Owner() && frame->Owner()->IsRemote())
-    ToRemoteFrameOwner(frame->Owner())
+  web_frame->InitializeCoreFrame(*old_frame->GetPage(),
+                                 DummyFrameOwner::Create(),
+                                 old_frame->Tree().GetName());
+
+  LocalFrame* new_frame = web_frame->GetFrame();
+  new_frame->SetOwner(old_frame->Owner());
+  if (new_frame->Owner() && new_frame->Owner()->IsRemote()) {
+    ToRemoteFrameOwner(new_frame->Owner())
         ->SetSandboxFlags(static_cast<SandboxFlags>(flags));
-
-  // We must call init() after m_frame is assigned because it is referenced
-  // during init(). Note that this may dispatch JS events; the frame may be
-  // detached after init() returns.
-  frame->Init();
+  }
   return web_frame;
 }
 
@@ -1642,26 +1632,21 @@ void WebLocalFrameImpl::InitializeCoreFrame(Page& page,
   SetCoreFrame(LocalFrame::Create(local_frame_client_impl_.Get(), page, owner,
                                   interface_provider_, interface_registry_));
   GetFrame()->Tree().SetName(name);

[dcheng, 2017/05/11 21:36:25]

frame_ here as well for consistency?

[Nate Chapin, 2017/05/12 19:52:28]

Done.

-  // We must call init() after m_frame is assigned because it is referenced
-  // during init(). Note that this may dispatch JS events; the frame may be
-  // detached after init() returns.
-  GetFrame()->Init();
-  if (GetFrame()) {
-    if (GetFrame()
-            ->Loader()
-            .StateMachine()
-            ->IsDisplayingInitialEmptyDocument() &&
-        !Parent() && !Opener() &&
-        GetFrame()->GetSettings()->GetShouldReuseGlobalForUnownedMainFrame()) {
-      GetFrame()->GetDocument()->GetSecurityOrigin()->GrantUniversalAccess();
-    }
+  // We must call init() after frame_ is assigned because it is referenced
+  // during init().
+  frame_->Init();
+  CHECK(frame_);
+  CHECK(frame_->Loader().StateMachine()->IsDisplayingInitialEmptyDocument());

[Nate Chapin, 2017/05/10 16:33:20]

CHECK might be overkill here, but given that risk of giving a universal access
SecurityOrigin to a a non-empty document if a navigation does somehow happen
inside Init(), it seems worth it to be cautious.

+  if (!Parent() && !Opener() &&
+      frame_->GetSettings()->GetShouldReuseGlobalForUnownedMainFrame()) {
+    frame_->GetDocument()->GetSecurityOrigin()->GrantUniversalAccess();
+  }
 
-    if (!owner) {
-      // This trace event is needed to detect the main frame of the
-      // renderer in telemetry metrics. See crbug.com/692112#c11.
-      TRACE_EVENT_INSTANT1("loading", "markAsMainFrame",
-                           TRACE_EVENT_SCOPE_THREAD, "frame", GetFrame());
-    }
+  if (!owner) {
+    // This trace event is needed to detect the main frame of the
+    // renderer in telemetry metrics. See crbug.com/692112#c11.
+    TRACE_EVENT_INSTANT1("loading", "markAsMainFrame", TRACE_EVENT_SCOPE_THREAD,
+                         "frame", frame_);
   }
 }
 
@@ -1697,10 +1682,7 @@ LocalFrame* WebLocalFrameImpl::CreateChildFrame(
 
   webframe_child->InitializeCoreFrame(*GetFrame()->GetPage(), owner_element,
                                       name);
-  // Initializing the core frame may cause the new child to be detached, since
-  // it may dispatch a load event in the parent.
-  if (!webframe_child->Parent())
-    return nullptr;
+  DCHECK(webframe_child->Parent());
 
   // If we're moving in the back/forward list, we might want to replace the
   // content of this child frame with whatever was there at that point.