From f9cea35d3e88ecc0b2edd526bd67ed2d56457bf0 Mon Sep 17 00:00:00 2001
From: David Baron <[email protected]>
Date: Fri, 25 Sep 2026 16:47:07 -0700
Subject: [PATCH] Ensure sufficient lifetime of WebViewImpl::page_popup_.

This is a straightforward fix that I think should be suitable for
backporting to branches.  In the longer term we might want to make
WebPagePopupImpl garbage collected, but that's a bit more involved.

I verified that the added unittest (built by Gemini from the original
testcase) does exercise the need for both the scoped_refptr in
WebViewImpl::UpdatePagePopup and the null-check in
WebPagePopupImpl::Update, both of which are also needed to fix the
original testcase.

Fixed: 565774991
Change-Id: I828fe321e04a74bf9f6ba25222c507197082cbc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8464185
Commit-Queue: David Baron <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1705319}
---
diff --git a/third_party/blink/renderer/core/exported/web_page_popup_impl.cc b/third_party/blink/renderer/core/exported/web_page_popup_impl.cc
index 372119a..4c2bc01 100644
--- a/third_party/blink/renderer/core/exported/web_page_popup_impl.cc
+++ b/third_party/blink/renderer/core/exported/web_page_popup_impl.cc
@@ -660,6 +660,10 @@
   }
 
   popup_client_->Update(forced_update);
+  if (!widget_base_) {
+    // Something inside Update() closed this popup.
+    return;
+  }
   if (forced_update) {
     SetWindowRect(WindowRectInScreen());
   }
@@ -1066,6 +1070,10 @@
   // Careful, this is now destroyed.
 }
 
+void WebPagePopupImpl::WidgetHostDisconnectedForTesting() {
+  WidgetHostDisconnected();
+}
+
 void WebPagePopupImpl::Close() {
   // If the popup is closed from the renderer via Cancel(), then ClosePopup()
   // has already run on another stack, and destroyed |page_|. If the popup is
diff --git a/third_party/blink/renderer/core/exported/web_page_popup_impl.h b/third_party/blink/renderer/core/exported/web_page_popup_impl.h
index d453230..c57d9eb 100644
--- a/third_party/blink/renderer/core/exported/web_page_popup_impl.h
+++ b/third_party/blink/renderer/core/exported/web_page_popup_impl.h
@@ -121,6 +121,7 @@
 
   // Return the LayerTreeHost backing this popup widget.
   cc::LayerTreeHost* LayerTreeHostForTesting();
+  void WidgetHostDisconnectedForTesting();
 
   // Called when the browser has shown the popup.
   void DidShowPopup();
diff --git a/third_party/blink/renderer/core/exported/web_view_impl.cc b/third_party/blink/renderer/core/exported/web_view_impl.cc
index 5aef3bbc..cff1b407 100644
--- a/third_party/blink/renderer/core/exported/web_view_impl.cc
+++ b/third_party/blink/renderer/core/exported/web_view_impl.cc
@@ -1128,6 +1128,10 @@
 
 void WebViewImpl::CancelPagePopup() {
   if (page_popup_) {
+    // Ensure page_popup_ lifetime lasts through the call even if the member
+    // variable is cleared by a callback during it.
+    scoped_refptr<WebPagePopupImpl> owning_reference(page_popup_);
+
     page_popup_->Cancel();
   }
 }
@@ -1139,6 +1143,11 @@
   if (page_popup_.get() != popup_impl) {
     return;
   }
+
+  // Ensure page_popup_ lifetime lasts through the call even if the member
+  // variable is cleared by a callback during it.
+  scoped_refptr<WebPagePopupImpl> owning_reference(page_popup_);
+
   page_popup_->ClosePopup();
 }
 
@@ -1149,6 +1158,10 @@
 
 void WebViewImpl::UpdatePagePopup() {
   if (page_popup_) {
+    // Ensure page_popup_ lifetime lasts through the call even if the member
+    // variable is cleared by a callback during it.
+    scoped_refptr<WebPagePopupImpl> owning_reference(page_popup_);
+
     page_popup_->Update();
   }
 }
diff --git a/third_party/blink/renderer/core/html/forms/internal_popup_menu_test.cc b/third_party/blink/renderer/core/html/forms/internal_popup_menu_test.cc
index b1499f96..defc7d5 100644
--- a/third_party/blink/renderer/core/html/forms/internal_popup_menu_test.cc
+++ b/third_party/blink/renderer/core/html/forms/internal_popup_menu_test.cc
@@ -11,6 +11,8 @@
 #include "third_party/blink/renderer/core/css/media_feature_overrides.h"
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/dom/element.h"
+#include "third_party/blink/renderer/core/dom/events/native_event_listener.h"
+#include "third_party/blink/renderer/core/event_type_names.h"
 #include "third_party/blink/renderer/core/exported/web_page_popup_impl.h"
 #include "third_party/blink/renderer/core/exported/web_view_impl.h"
 #include "third_party/blink/renderer/core/frame/frame_test_helpers.h"
@@ -192,6 +194,84 @@
   EXPECT_TRUE(select->PopupIsVisible());
 }
 
+namespace {
+class DisconnectOnChangeListener final : public NativeEventListener {
+ public:
+  explicit DisconnectOnChangeListener(WebPagePopupImpl* popup)
+      : popup_(popup) {}
+  void Invoke(ExecutionContext*, Event*) override {
+    if (popup_) {
+      popup_->WidgetHostDisconnectedForTesting();
+      popup_ = nullptr;
+    }
+  }
+
+ private:
+  raw_ptr<WebPagePopupImpl> popup_;
+};
+}  // namespace
+
+// See crbug.com/565774991.
+TEST_F(InternalPopupMenuTest, DisconnectDuringPopupUpdate) {
+  if (!RuntimeEnabledFeatures::PagePopupEnabled()) {
+    return;
+  }
+
+  frame_test_helpers::WebViewHelper web_view_helper;
+  WebViewImpl* web_view = web_view_helper.Initialize();
+  if (web_view->GetChromeClient().UseExternalPopupMenus()) {
+    // <select> doesn't use a WebPagePopupImpl on this platform.
+    return;
+  }
+  web_view->MainFrameViewWidget()->Resize(gfx::Size(800, 600));
+
+  WebURL base_url = url_test_helpers::ToKURL("http://example.com/");
+  frame_test_helpers::LoadHTMLString(web_view->MainFrameImpl(), R"HTML(
+    <!DOCTYPE html>
+    <style>
+      body { margin: 0; }
+      select {
+        position: absolute;
+        left: 0;
+        top: 0;
+        width: 100px;
+        height: 50px;
+      }
+    </style>
+    <select id="select">
+      <option>1</option>
+      <option>2</option>
+    </select>
+  )HTML",
+                                     base_url);
+
+  Document& document =
+      *web_view->MainFrameImpl()->GetDocument().Unwrap<Document>();
+  document.View()->UpdateAllLifecyclePhasesForTest();
+
+  auto* select =
+      To<HTMLSelectElement>(document.getElementById(AtomicString("select")));
+  ASSERT_TRUE(select);
+
+  select->ShowPopup();
+  EXPECT_TRUE(select->PopupIsVisible());
+
+  WebPagePopupImpl* popup = web_view->GetPagePopup();
+  ASSERT_TRUE(popup);
+  popup->DidShowPopup();
+
+  select->ProvisionalSelectionChanged(1);
+  select->addEventListener(
+      event_type_names::kChange,
+      MakeGarbageCollected<DisconnectOnChangeListener>(popup));
+
+  select->SetInlineStyleProperty(CSSPropertyID::kPosition, "absolute");
+  select->SetInlineStyleProperty(CSSPropertyID::kTop, "5000px");
+  document.View()->UpdateAllLifecyclePhasesForTest();
+
+  web_view->UpdatePagePopup();
+}
+
 #endif  // !BUILDFLAG(IS_ANDROID)
 
 }  // namespace blink

diff --git a/third_party/blink/renderer/core/html/forms/internal_popup_menu_test.cc b/third_party/blink/renderer/core/html/forms/internal_popup_menu_test.cc
index b1499f96..defc7d5 100644
--- a/third_party/blink/renderer/core/html/forms/internal_popup_menu_test.cc
+++ b/third_party/blink/renderer/core/html/forms/internal_popup_menu_test.cc
@@ -11,6 +11,8 @@
 #include "third_party/blink/renderer/core/css/media_feature_overrides.h"
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/dom/element.h"
+#include "third_party/blink/renderer/core/dom/events/native_event_listener.h"
+#include "third_party/blink/renderer/core/event_type_names.h"
 #include "third_party/blink/renderer/core/exported/web_page_popup_impl.h"
 #include "third_party/blink/renderer/core/exported/web_view_impl.h"
 #include "third_party/blink/renderer/core/frame/frame_test_helpers.h"
@@ -192,6 +194,84 @@
   EXPECT_TRUE(select->PopupIsVisible());
 }
 
+namespace {
+class DisconnectOnChangeListener final : public NativeEventListener {
+ public:
+  explicit DisconnectOnChangeListener(WebPagePopupImpl* popup)
+      : popup_(popup) {}
+  void Invoke(ExecutionContext*, Event*) override {
+    if (popup_) {
+      popup_->WidgetHostDisconnectedForTesting();
+      popup_ = nullptr;
+    }
+  }
+
+ private:
+  raw_ptr<WebPagePopupImpl> popup_;
+};
+}  // namespace
+
+// See crbug.com/565774991.
+TEST_F(InternalPopupMenuTest, DisconnectDuringPopupUpdate) {
+  if (!RuntimeEnabledFeatures::PagePopupEnabled()) {
+    return;
+  }
+
+  frame_test_helpers::WebViewHelper web_view_helper;
+  WebViewImpl* web_view = web_view_helper.Initialize();
+  if (web_view->GetChromeClient().UseExternalPopupMenus()) {
+    // <select> doesn't use a WebPagePopupImpl on this platform.
+    return;
+  }
+  web_view->MainFrameViewWidget()->Resize(gfx::Size(800, 600));
+
+  WebURL base_url = url_test_helpers::ToKURL("http://example.com/");
+  frame_test_helpers::LoadHTMLString(web_view->MainFrameImpl(), R"HTML(
+    <!DOCTYPE html>
+    <style>
+      body { margin: 0; }
+      select {
+        position: absolute;
+        left: 0;
+        top: 0;
+        width: 100px;
+        height: 50px;
+      }
+    </style>
+    <select id="select">
+      <option>1</option>
+      <option>2</option>
+    </select>
+  )HTML",
+                                     base_url);
+
+  Document& document =
+      *web_view->MainFrameImpl()->GetDocument().Unwrap<Document>();
+  document.View()->UpdateAllLifecyclePhasesForTest();
+
+  auto* select =
+      To<HTMLSelectElement>(document.getElementById(AtomicString("select")));
+  ASSERT_TRUE(select);
+
+  select->ShowPopup();
+  EXPECT_TRUE(select->PopupIsVisible());
+
+  WebPagePopupImpl* popup = web_view->GetPagePopup();
+  ASSERT_TRUE(popup);
+  popup->DidShowPopup();
+
+  select->ProvisionalSelectionChanged(1);
+  select->addEventListener(
+      event_type_names::kChange,
+      MakeGarbageCollected<DisconnectOnChangeListener>(popup));
+
+  select->SetInlineStyleProperty(CSSPropertyID::kPosition, "absolute");
+  select->SetInlineStyleProperty(CSSPropertyID::kTop, "5000px");
+  document.View()->UpdateAllLifecyclePhasesForTest();
+
+  web_view->UpdatePagePopup();
+}
+
 #endif  // !BUILDFLAG(IS_ANDROID)
 
 }  // namespace blink