Cleanup around LocalFrame initialization

third_party/WebKit/Source/web/WebLocalFrameImpl.cpp

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 1541 matching lines @@
   DCHECK(client);
   WebLocalFrameImpl* web_frame = new WebLocalFrameImpl(
       old_web_frame, client, interface_provider, interface_registry);
   Frame* old_frame = ToWebRemoteFrameImpl(old_web_frame)->GetFrame();
   web_frame->SetParent(old_web_frame->Parent());
   web_frame->SetOpener(old_web_frame->Opener());
   // Note: this *always* temporarily sets a frame owner, even for main frames!
   // 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);
+  web_frame->InitializeCoreFrame(*old_frame->GetPage(),
+                                 DummyFrameOwner::Create(),
+                                 old_frame->Tree().GetName());
 
-  frame->SetOwner(old_frame->Owner());
-
-  if (frame->Owner() && frame->Owner()->IsRemote())
-    ToRemoteFrameOwner(frame->Owner())
+  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;
 }
 
 WebLocalFrameImpl::WebLocalFrameImpl(
     WebTreeScopeType scope,
     WebFrameClient* client,
     blink::InterfaceProvider* interface_provider,
     blink::InterfaceRegistry* interface_registry)
     : WebLocalFrameBase(scope),
       local_frame_client_impl_(LocalFrameClientImpl::Create(this)),
@@ skipping 42 matching lines @@
 
 void WebLocalFrameImpl::SetCoreFrame(LocalFrame* frame) {
   frame_ = frame;
 }
 
 void WebLocalFrameImpl::InitializeCoreFrame(Page& page,
                                             FrameOwner* owner,
                                             const AtomicString& name) {
   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_);
   }
 }
 
 LocalFrame* WebLocalFrameImpl::CreateChildFrame(
     const FrameLoadRequest& request,
     const AtomicString& name,
     HTMLFrameOwnerElement* owner_element) {
   DCHECK(client_);
   TRACE_EVENT0("blink", "WebLocalFrameImpl::createChildframe");
   WebTreeScopeType scope =
@@ skipping 15 matching lines @@
           this, scope, name,
           owner_element->getAttribute(
               owner_element->SubResourceAttributeName()),
           static_cast<WebSandboxFlags>(owner_element->GetSandboxFlags()),
           owner_element->ContainerPolicy(), owner_properties));
   if (!webframe_child)
     return nullptr;
 
   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.
   HistoryItem* child_item = nullptr;
   if (IsBackForwardLoadType(
           GetFrame()->Loader().GetDocumentLoader()->LoadType()) &&
       !GetFrame()->GetDocument()->LoadEventFinished())
     child_item = webframe_child->Client()->HistoryItemForNewChildFrame();
 
   FrameLoadRequest new_request = request;
@@ skipping 862 matching lines @@
 TextCheckerClient& WebLocalFrameImpl::GetTextCheckerClient() const {
   return *text_checker_client_;
 }
 
 void WebLocalFrameImpl::SetTextCheckClient(
     WebTextCheckClient* text_check_client) {
   text_check_client_ = text_check_client;
 }
 
 }  // namespace blink