Revert "Add support for extension message and back/forward cache."

This reverts commit 119091e. Reason for revert: */ExtendedFilesAppBrowserTest.Test/* is flaky Original change's description: > Add support for extension message and back/forward cache. > > Previously we use to disallow back/forward cache on any page that > did extension messaging. Narrow that broad disablement to only if > there is an active message port open. > > BUG=1222803 > > Change-Id: I51e456de5d774c997473924c7cd3e04e7df84a0a > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2980333 > Reviewed-by: Avi Drissman <avi@chromium.org> > Reviewed-by: Kouhei Ueno <kouhei@chromium.org> > Reviewed-by: Karan Bhatia <karandeepb@chromium.org> > Reviewed-by: Kentaro Hara <haraken@chromium.org> > Commit-Queue: Dave Tapuska <dtapuska@chromium.org> > Cr-Commit-Position: refs/heads/master@{#897103} Bug: 1222803, 1225246 Change-Id: I4a29e8d23146de75835ade5821a635251817d069 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2997442 Auto-Submit: Etienne Pierre-Doray <etiennep@chromium.org> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Owners-Override: Dave Tapuska <dtapuska@chromium.org> Cr-Commit-Position: refs/heads/master@{#897385}
qweri0p · Jun 30, 2021 · 1a45e05 · 1a45e05
1 parent 3c57991
commit 1a45e05
Show file tree

Hide file tree

Showing 14 changed files with 54 additions and 406 deletions.
diff --git a/chrome/browser/extensions/back_forward_cache_browsertest.cc b/chrome/browser/extensions/back_forward_cache_browsertest.cc
diff --git a/chrome/test/data/extensions/back_forward_cache/content_script/background.js b/chrome/test/data/extensions/back_forward_cache/content_script/background.js
diff --git a/...rd_cache/content_script/content_script.js → ...cache/content_script/change_page_title.js b/...rd_cache/content_script/content_script.js → ...cache/content_script/change_page_title.js
@@ -3,12 +3,3 @@
 // found in the LICENSE file.
 
 document.title = "modified";
-
-chrome.runtime.onConnect.addListener((p) => {
-  p.postMessage('connected');
-  p.onMessage.addListener((m) => {
-    if (m == 'disconnect') {
-      p.disconnect();
-    }
-  });
-});
diff --git a/chrome/test/data/extensions/back_forward_cache/content_script/manifest.json b/chrome/test/data/extensions/back_forward_cache/content_script/manifest.json
@@ -10,12 +10,8 @@
   "content_scripts": [
     {
       "matches": ["http://*/*", "https://*/*"],
-      "js": ["content_script.js"],
+      "js": ["change_page_title.js"],
       "run_at": "document_end"
     }
-  ],
-  "background": {
-    "scripts": ["background.js"],
-    "persistent": false
-  }
+  ]
 }
diff --git a/components/back_forward_cache/back_forward_cache_disable.cc b/components/back_forward_cache/back_forward_cache_disable.cc
@@ -35,8 +35,6 @@ std::string ReasonIdToString(DisabledReasonId reason_id) {
       return "Extensions";
     case DisabledReasonId::kExtensionMessaging:
       return "ExtensionMessaging";
-    case DisabledReasonId::kExtensionMessagingForOpenPort:
-      return "ExtensionMessagingForOpenPort";
     case DisabledReasonId::kOomInterventionTabHelper:
       return "OomInterventionTabHelper";
   }

diff --git a/components/back_forward_cache/back_forward_cache_disable.h b/components/back_forward_cache/back_forward_cache_disable.h
@@ -31,7 +31,6 @@ enum class DisabledReasonId : content::BackForwardCache::DisabledReasonType {
   kModalDialog = 11,
   kExtensions = 12,
   kExtensionMessaging = 13,
-  kExtensionMessagingForOpenPort = 14,
   // New reasons should be accompanied by a comment as to why BackForwardCache
   // cannot be used in this case and a link to a bug to fix that if it is
   // fixable.

diff --git a/content/browser/renderer_host/back_forward_cache_impl.cc b/content/browser/renderer_host/back_forward_cache_impl.cc
@@ -906,15 +906,6 @@ void BackForwardCache::DisableForRenderFrameHost(
   DisableForRenderFrameHost(render_frame_host->GetGlobalId(), reason);
 }
 
-// static
-void BackForwardCache::ClearDisableReasonForRenderFrameHost(
-    GlobalRenderFrameHostId id,
-    BackForwardCache::DisabledReason reason) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (auto* rfh = RenderFrameHostImpl::FromID(id))
-    rfh->ClearDisableBackForwardCache(reason);
-}
-
 // static
 void BackForwardCache::DisableForRenderFrameHost(
     GlobalRenderFrameHostId id,

diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc
@@ -1728,11 +1728,6 @@ void RenderFrameHostImpl::DisableBackForwardCache(
   MaybeEvictFromBackForwardCache();
 }
 
-void RenderFrameHostImpl::ClearDisableBackForwardCache(
-    BackForwardCache::DisabledReason reason) {
-  back_forward_cache_disabled_reasons_.erase(reason);
-}
-
 void RenderFrameHostImpl::DisableProactiveBrowsingInstanceSwapForTesting() {
   // This should only be called on main frames.
   DCHECK(!GetParent());

diff --git a/content/browser/renderer_host/render_frame_host_impl.h b/content/browser/renderer_host/render_frame_host_impl.h
@@ -1299,7 +1299,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
   void DisableBackForwardCache(BackForwardCache::DisabledSource source,
                                BackForwardCache::DisabledReasonType reason);
   void DisableBackForwardCache(BackForwardCache::DisabledReason reason);
-  void ClearDisableBackForwardCache(BackForwardCache::DisabledReason reason);
 
   bool is_evicted_from_back_forward_cache() {
     return is_evicted_from_back_forward_cache_;

diff --git a/content/public/browser/back_forward_cache.h b/content/public/browser/back_forward_cache.h
@@ -87,9 +87,6 @@ class CONTENT_EXPORT BackForwardCache {
   // its |id| can be used.
   static void DisableForRenderFrameHost(GlobalRenderFrameHostId id,
                                         DisabledReason reason);
-  // Clear a previously set `reason` for a `render_frame_host`.
-  static void ClearDisableReasonForRenderFrameHost(GlobalRenderFrameHostId id,
-                                                   DisabledReason reason);
 
   // List of reasons the BackForwardCache was disabled for a specific test. If a
   // test needs to be disabled for a reason not covered below, please add to

diff --git a/extensions/browser/api/messaging/extension_message_port.cc b/extensions/browser/api/messaging/extension_message_port.cc
@@ -8,13 +8,11 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/containers/contains.h"
 #include "base/scoped_observation.h"
 #include "base/strings/strcat.h"
-#include "components/back_forward_cache/back_forward_cache_disable.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/navigation_handle.h"
-#include "content/public/browser/render_document_host_user_data.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -34,60 +32,6 @@ std::string PortIdToString(const extensions::PortId& port_id) {
                        base::NumberToString(port_id.GetChannelId().second)});
 }
 
-// This class is created per RenderDocument. It stores the number of
-// ExtensionMessagePort objects that have the associated `RenderFrameHost` in
-// the `ExtensionMessagePort::frames_` set.
-class MessagePortStatePerRenderDocument
-    : public content::RenderDocumentHostUserData<
-          MessagePortStatePerRenderDocument> {
- public:
-  ~MessagePortStatePerRenderDocument() override {
-    if (ports_opened_ > 0) {
-      // Clear the disable reason as the new document may reuse the
-      // RenderFrameHost.
-      content::BackForwardCache::ClearDisableReasonForRenderFrameHost(
-          rfh_id_, back_forward_cache::DisabledReason(
-                       back_forward_cache::DisabledReasonId::
-                           kExtensionMessagingForOpenPort));
-    }
-  }
-
-  void PortOpened() {
-    ports_opened_++;
-    if (ports_opened_ != 1)
-      return;
-    content::BackForwardCache::DisableForRenderFrameHost(
-        rfh_id_, back_forward_cache::DisabledReason(
-                     back_forward_cache::DisabledReasonId::
-                         kExtensionMessagingForOpenPort));
-  }
-
-  void PortClosed() {
-    DCHECK_LT(0u, ports_opened_);
-    ports_opened_--;
-    if (ports_opened_ > 0)
-      return;
-
-    content::BackForwardCache::ClearDisableReasonForRenderFrameHost(
-        rfh_id_, back_forward_cache::DisabledReason(
-                     back_forward_cache::DisabledReasonId::
-                         kExtensionMessagingForOpenPort));
-  }
-
- private:
-  explicit MessagePortStatePerRenderDocument(content::RenderFrameHost* rfh)
-      : rfh_id_(rfh->GetGlobalId()) {}
-  friend class content::RenderDocumentHostUserData<
-      MessagePortStatePerRenderDocument>;
-
-  size_t ports_opened_ = 0;
-  const content::GlobalRenderFrameHostId rfh_id_;
-
-  RENDER_DOCUMENT_HOST_USER_DATA_KEY_DECL();
-};
-
-RENDER_DOCUMENT_HOST_USER_DATA_KEY_IMPL(MessagePortStatePerRenderDocument)
-
 }  // namespace
 
 namespace extensions {
@@ -237,29 +181,13 @@ std::unique_ptr<ExtensionMessagePort> ExtensionMessagePort::CreateForEndpoint(
   return port;
 }
 
-ExtensionMessagePort::~ExtensionMessagePort() {
-  ClearFrames();
-}
-
-void ExtensionMessagePort::ClearFrames() {
-  for (auto* rfh : frames_) {
-    auto* message_port_state =
-        MessagePortStatePerRenderDocument::GetForCurrentDocument(rfh);
-    if (message_port_state)
-      message_port_state->PortClosed();
-  }
-  frames_.clear();
-}
+ExtensionMessagePort::~ExtensionMessagePort() {}
 
 void ExtensionMessagePort::RemoveCommonFrames(const MessagePort& port) {
   // Avoid overlap in the set of frames to make sure that it does not matter
   // when UnregisterFrame is called.
   for (auto it = frames_.begin(); it != frames_.end();) {
     if (port.HasFrame(*it)) {
-      auto* message_port_state =
-          MessagePortStatePerRenderDocument::GetForCurrentDocument(*it);
-      if (message_port_state)
-        message_port_state->PortClosed();
       frames_.erase(it++);
     } else {
       ++it;
@@ -398,7 +326,7 @@ void ExtensionMessagePort::ClosePort(int process_id,
     // The only non-frame-specific message is the response to an unhandled
     // onConnect event in the extension process.
     DCHECK(extension_process_);
-    ClearFrames();
+    frames_.clear();
     if (!HasReceivers())
       CloseChannel();
     return;
@@ -425,23 +353,12 @@ void ExtensionMessagePort::RegisterFrame(content::RenderFrameHost* rfh) {
   // ensure that we are notified of frame destruction. Without this check,
   // |frames_| can eventually contain a stale pointer because RenderFrameDeleted
   // is not triggered for |rfh|.
-  if (rfh->IsRenderFrameLive()) {
-    auto* message_port_state =
-        MessagePortStatePerRenderDocument::GetOrCreateForCurrentDocument(rfh);
-    message_port_state->PortOpened();
+  if (rfh->IsRenderFrameLive())
     frames_.insert(rfh);
-  }
 }
 
 void ExtensionMessagePort::UnregisterFrame(content::RenderFrameHost* rfh) {
-  if (frames_.erase(rfh) == 0)
-    return;
-  auto* message_port_state =
-      MessagePortStatePerRenderDocument::GetForCurrentDocument(rfh);
-  if (message_port_state)
-    message_port_state->PortClosed();
-
-  if (!HasReceivers())
+  if (frames_.erase(rfh) != 0 && !HasReceivers())
     CloseChannel();
 }
 

diff --git a/extensions/browser/api/messaging/extension_message_port.h b/extensions/browser/api/messaging/extension_message_port.h
@@ -96,9 +96,6 @@ class ExtensionMessagePort : public MessagePort {
                        const ExtensionId& extension_id,
                        content::BrowserContext* browser_context);
 
-  // Clears the `frames_` set.
-  void ClearFrames();
-
   // Registers a frame as a receiver / sender.
   void RegisterFrame(content::RenderFrameHost* rfh);
 

diff --git a/extensions/browser/api/messaging/message_service.cc b/extensions/browser/api/messaging/message_service.cc
@@ -19,6 +19,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/values.h"
 #include "build/build_config.h"
+#include "components/back_forward_cache/back_forward_cache_disable.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
@@ -110,6 +111,14 @@ const Extension* GetExtensionForNativeAppChannel(
                                                                 true);
 }
 
+void DisableBackForwardCacheForMessaging(content::RenderFrameHost* host) {
+  if (!host)
+    return;
+  content::BackForwardCache::DisableForRenderFrameHost(
+      host, back_forward_cache::DisabledReason(
+                back_forward_cache::DisabledReasonId::kExtensionMessaging));
+}
+
 }  // namespace
 
 struct MessageService::MessageChannel {
@@ -423,6 +432,9 @@ void MessageService::OpenChannelToNativeApp(
   content::RenderFrameHost* source_rfh =
       source.is_for_render_frame() ? source.GetRenderFrameHost() : nullptr;
 
+  // Disable back forward cache.
+  DisableBackForwardCacheForMessaging(source_rfh);
+
   std::string error = kReceivingEndDoesntExistError;
   const PortId receiver_port_id = source_port_id.GetOppositePortId();
   // NOTE: We're creating |receiver| with nullptr |source_rfh|, which seems to
@@ -488,6 +500,9 @@ void MessageService::OpenChannelToTab(const ChannelEndpoint& source,
     return;
   }
 
+  // Disable back forward cache.
+  DisableBackForwardCacheForMessaging(receiver_contents->GetMainFrame());
+
   const PortId receiver_port_id = source_port_id.GetOppositePortId();
   std::unique_ptr<MessagePort> receiver =
       messaging_delegate_->CreateReceiverForTab(weak_factory_.GetWeakPtr(),
@@ -883,6 +898,10 @@ void MessageService::OnOpenChannelAllowed(
     return;
   }
 
+  content::RenderFrameHost* source_rfh =
+      source.is_for_render_frame() ? source.GetRenderFrameHost() : nullptr;
+  DisableBackForwardCacheForMessaging(source_rfh);
+
   // The target might be a lazy background page or a Service Worker. In that
   // case, we have to check if it is loaded and ready, and if not, queue up the
   // task and load the page.

diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
@@ -6836,7 +6836,6 @@ others/histograms.xml -->
   <int value="196619" label="kEmbedder-kModalDialog"/>
   <int value="196620" label="kEmbedder-kExtensions"/>
   <int value="196621" label="kEmbedder-kExtensionMessaging"/>
-  <int value="196622" label="kEmbedder-kExtensionMessagingForOpenPort"/>
 </enum>
 
 <enum name="BackForwardCacheDisabledForRenderFrameHostReasonShort">