Unconditionally detach in Page::WillDetachFrame()
ElementVisibilityObserverTest was using PageTestBase and would
have been tricky to fix properly; instead just migrate it over
to use FrameTestHelpers::WebViewHelper.
PresentationAvailabilityTest is left as a V8TestingScope for
now but should ultimately be converted as well.
Bug: 700783, 734748
Change-Id: Ic73f187a2a1082891168143e37675a763f80db2c
Reviewed-on: https://chromium-review.googlesource.com/1038142
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556134}diff --git a/third_party/blink/renderer/core/dom/element_visibility_observer_test.cc b/third_party/blink/renderer/core/dom/element_visibility_observer_test.cc
index 3cf8471..5cc0d1a 100644
--- a/third_party/blink/renderer/core/dom/element_visibility_observer_test.cc
+++ b/third_party/blink/renderer/core/dom/element_visibility_observer_test.cc
@@ -7,56 +7,27 @@
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/renderer/core/dom/document.h"
#include "third_party/blink/renderer/core/dom/dom_implementation.h"
-#include "third_party/blink/renderer/core/frame/remote_frame.h"
+#include "third_party/blink/renderer/core/exported/web_remote_frame_impl.h"
+#include "third_party/blink/renderer/core/frame/frame_test_helpers.h"
+#include "third_party/blink/renderer/core/frame/web_local_frame_impl.h"
#include "third_party/blink/renderer/core/html/html_div_element.h"
#include "third_party/blink/renderer/core/html/html_document.h"
#include "third_party/blink/renderer/core/loader/empty_clients.h"
-#include "third_party/blink/renderer/core/testing/page_test_base.h"
namespace blink {
namespace {
-// Stub implementation of LocalFrameClient for the purpose of testing. It will
-// alow callers to set the parent/top frames by calling |setParent|. It is used
-// in ElementVisibilityObserverTest in order to mock a RemoteFrame parent of a
-// LocalFrame.
-class DOMStubLocalFrameClient final : public EmptyLocalFrameClient {
- public:
- Frame* Parent() const override { return parent_; }
- Frame* Top() const override { return parent_; }
-
- void SetParent(Frame* frame) { parent_ = frame; }
-
- void Trace(blink::Visitor* visitor) override {
- visitor->Trace(parent_);
- EmptyLocalFrameClient::Trace(visitor);
- }
-
- private:
- WeakMember<Frame> parent_ = nullptr;
-};
-
-class ElementVisibilityObserverTest : public PageTestBase {
+class ElementVisibilityObserverTest : public ::testing::Test {
protected:
- void SetUp() override {
- local_frame_client_ = new DOMStubLocalFrameClient();
- SetupPageWithClients(nullptr, local_frame_client_, nullptr);
- }
-
- void TearDown() override { GetFrame().Detach(FrameDetachType::kRemove); }
-
- DOMStubLocalFrameClient* LocalFrameClient() const {
- return local_frame_client_;
- }
-
- private:
- Persistent<DOMStubLocalFrameClient> local_frame_client_;
+ FrameTestHelpers::WebViewHelper helper_;
};
TEST_F(ElementVisibilityObserverTest, ObserveElementWithoutDocumentFrame) {
+ helper_.Initialize();
+ Document& document = *helper_.LocalMainFrame()->GetFrame()->GetDocument();
HTMLElement* element = HTMLDivElement::Create(
- *DOMImplementation::Create(GetDocument())->createHTMLDocument("test"));
+ *DOMImplementation::Create(document)->createHTMLDocument("test"));
ElementVisibilityObserver* observer = new ElementVisibilityObserver(
element, ElementVisibilityObserver::VisibilityCallback());
observer->Start();
@@ -64,12 +35,14 @@
// It should not crash.
}
-TEST_F(ElementVisibilityObserverTest, ObserveElementInRemoteFrame) {
- Persistent<RemoteFrame> remote_frame =
- RemoteFrame::Create(new EmptyRemoteFrameClient(), GetPage(), nullptr);
- LocalFrameClient()->SetParent(remote_frame);
+TEST_F(ElementVisibilityObserverTest, ObserveElementWithRemoteFrameParent) {
+ helper_.InitializeRemote();
- Persistent<HTMLElement> element = HTMLDivElement::Create(GetDocument());
+ WebLocalFrameImpl* child_frame =
+ FrameTestHelpers::CreateLocalChild(*helper_.RemoteMainFrame());
+ Document& document = *child_frame->GetFrame()->GetDocument();
+
+ Persistent<HTMLElement> element = HTMLDivElement::Create(document);
ElementVisibilityObserver* observer =
new ElementVisibilityObserver(element, WTF::BindRepeating([](bool) {}));
observer->Start();
diff --git a/third_party/blink/renderer/core/frame/local_frame.cc b/third_party/blink/renderer/core/frame/local_frame.cc
index cb6f7e392..71d1aaa 100644
--- a/third_party/blink/renderer/core/frame/local_frame.cc
+++ b/third_party/blink/renderer/core/frame/local_frame.cc
@@ -330,8 +330,14 @@
}
void LocalFrame::Detach(FrameDetachType type) {
- // Note that detach() can be re-entered, so it's not possible to
- // DCHECK(isAttached()) here.
+ // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+ // BEGIN RE-ENTRANCY SAFE BLOCK
+ // Starting here, the code must be safe against re-entrancy. Dispatching
+ // events, et cetera can run Javascript, which can reenter Detach().
+ // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+
+ // Detach() can be re-entered, this can't simply DCHECK(IsAttached()).
+ DCHECK_NE(lifecycle_.GetState(), FrameLifecycle::kDetached);
lifecycle_.AdvanceTo(FrameLifecycle::kDetaching);
if (IsLocalRoot()) {
@@ -381,6 +387,14 @@
if (!Client())
return;
+ // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+ // END RE-ENTRANCY SAFE BLOCK
+ // Past this point, no script should be executed. If this method was
+ // re-entered, then check for a non-null Client() above should have already
+ // returned.
+ // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+ DCHECK_NE(lifecycle_.GetState(), FrameLifecycle::kDetached);
+
// TODO(crbug.com/729196): Trace why LocalFrameView::DetachFromLayout crashes.
CHECK(!view_->IsAttached());
Client()->WillBeDetached();
diff --git a/third_party/blink/renderer/core/page/page.cc b/third_party/blink/renderer/core/page/page.cc
index e87c1403..dc89b93 100644
--- a/third_party/blink/renderer/core/page/page.cc
+++ b/third_party/blink/renderer/core/page/page.cc
@@ -743,10 +743,7 @@
void Page::WillBeDestroyed() {
Frame* main_frame = main_frame_;
- // TODO(sashab): Remove this check, the call to detach() here should always
- // work.
- if (main_frame->IsAttached())
- main_frame->Detach(FrameDetachType::kRemove);
+ main_frame->Detach(FrameDetachType::kRemove);
DCHECK(AllPages().Contains(this));
AllPages().erase(this);
diff --git a/third_party/blink/renderer/modules/presentation/presentation_availability_test.cc b/third_party/blink/renderer/modules/presentation/presentation_availability_test.cc
index cc4a1c9..74c0702 100644
--- a/third_party/blink/renderer/modules/presentation/presentation_availability_test.cc
+++ b/third_party/blink/renderer/modules/presentation/presentation_availability_test.cc
@@ -22,22 +22,27 @@
namespace {
TEST(PresentationAvailabilityTest, NoPageVisibilityChangeAfterDetach) {
- V8TestingScope scope;
- WTF::Vector<KURL> urls;
- urls.push_back(URLTestHelpers::ToKURL("https://example.com"));
- urls.push_back(URLTestHelpers::ToKURL("https://another.com"));
+ Page* page = nullptr;
+ {
+ V8TestingScope scope;
+ WTF::Vector<KURL> urls;
+ urls.push_back(URLTestHelpers::ToKURL("https://example.com"));
+ urls.push_back(URLTestHelpers::ToKURL("https://another.com"));
- Persistent<PresentationAvailabilityProperty> resolver =
- new PresentationAvailabilityProperty(
- scope.GetExecutionContext(), nullptr,
- PresentationAvailabilityProperty::kReady);
- Persistent<PresentationAvailability> availability =
- PresentationAvailability::Take(resolver, urls, false);
+ Persistent<PresentationAvailabilityProperty> resolver =
+ new PresentationAvailabilityProperty(
+ scope.GetExecutionContext(), nullptr,
+ PresentationAvailabilityProperty::kReady);
+ Persistent<PresentationAvailability> availability =
+ PresentationAvailability::Take(resolver, urls, false);
- // These two calls should not crash.
- scope.GetFrame().Detach(FrameDetachType::kRemove);
- scope.GetPage().SetVisibilityState(mojom::PageVisibilityState::kHidden,
- false);
+ page = &scope.GetPage();
+ }
+ // This should not crash.
+ // TODO(dcheng): Why are we calling functions on Page after it's been closed?
+ // This case doesn't seem like it should be reachable as we should be shutting
+ // down communication from the embedder on context detach.
+ page->SetVisibilityState(mojom::PageVisibilityState::kHidden, false);
}
} // anonymous namespace
