ContentCapture: Remove the previous session upon the new one starts

- This will give the consumer the ability to discard the old content
ASAP.
- Also change the test to make it pass on BFCache feature.

Bug: 1115234
Change-Id: Ib41f5642018e6e11c919100b29bf7fb10e2f91b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2350126Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798881}

components/content_capture/browser/DEPS

@@ -3,3 +3,9 @@ include_rules = [
   "+content/public/test",
   "+third_party/blink/public/common/associated_interfaces",
 ]
+
+specific_include_rules = {
+  ".*_test.cc": [
+    "+content/public/common",
+  ],
+}

components/content_capture/browser/content_capture_receiver.cc

@@ -29,11 +29,7 @@ ContentCaptureReceiver::ContentCaptureReceiver(content::RenderFrameHost* rfh)
     : rfh_(rfh), id_(GetIdFrom(rfh)) {}
 
 ContentCaptureReceiver::~ContentCaptureReceiver() {
-  // TODO(crbug.com/995952): Find a way to notify of session being removed if
-  // rfh isn't available.
-  if (auto* manager = GetContentCaptureReceiverManager(rfh_)) {
-    manager->DidRemoveSession(this);
-  }
+  RemoveSession();
 }
 
 int64_t ContentCaptureReceiver::GetIdFrom(content::RenderFrameHost* rfh) {
@@ -60,13 +56,14 @@ void ContentCaptureReceiver::DidCaptureContent(const ContentCaptureData& data,
     // is changed, otherwise the child frame's session will be removed.
     if (frame_content_capture_data_.id != 0 &&
         frame_content_capture_data_.value != data.value) {
-      manager->DidRemoveSession(this);
+      RemoveSession();
     }
 
     frame_content_capture_data_.id = id_;
     // Copies everything except id and children.
     frame_content_capture_data_.value = data.value;
     frame_content_capture_data_.bounds = data.bounds;
+    has_session_ = true;
   }
   // We can't avoid copy the data here, because id need to be overridden.
   ContentCaptureData content(data);
@@ -118,6 +115,21 @@ void ContentCaptureReceiver::StopCapture() {
   }
 }
 
+void ContentCaptureReceiver::RemoveSession() {
+  if (!has_session_)
+    return;
+
+  // TODO(crbug.com/995952): Find a way to notify of session being removed if
+  // rfh isn't available.
+  if (auto* manager = GetContentCaptureReceiverManager(rfh_)) {
+    manager->DidRemoveSession(this);
+    has_session_ = false;
+    // We can reset the frame_content_capture_data_ here, because it could be
+    // used by GetFrameContentCaptureDataLastSeen(), has_session_ is used to
+    // check if new session shall be created as needed.
+  }
+}
+
 const mojo::AssociatedRemote<mojom::ContentCaptureSender>&
 ContentCaptureReceiver::GetContentCaptureSender() {
   if (!content_capture_sender_) {
@@ -129,20 +141,19 @@ ContentCaptureReceiver::GetContentCaptureSender() {
 
 const ContentCaptureData& ContentCaptureReceiver::GetFrameContentCaptureData() {
   base::string16 url = base::UTF8ToUTF16(rfh_->GetLastCommittedURL().spec());
-  if (url == frame_content_capture_data_.value)
+  if (url == frame_content_capture_data_.value && has_session_)
     return frame_content_capture_data_;
 
-  if (frame_content_capture_data_.id != 0) {
-    auto* manager = GetContentCaptureReceiverManager(rfh_);
-    DCHECK(manager);
-    manager->DidRemoveSession(this);
-  }
+  if (frame_content_capture_data_.id != 0 && has_session_)
+    RemoveSession();
 
   frame_content_capture_data_.id = id_;
   frame_content_capture_data_.value = url;
   const base::Optional<gfx::Size>& size = rfh_->GetFrameSize();
   if (size.has_value())
     frame_content_capture_data_.bounds = gfx::Rect(size.value());
+
+  has_session_ = true;
   return frame_content_capture_data_;
 }
 

components/content_capture/browser/content_capture_receiver.h

@@ -50,6 +50,8 @@ class ContentCaptureReceiver : public mojom::ContentCaptureReceiver {
     return frame_content_capture_data_;
   }
 
+  void RemoveSession();
+
  private:
   FRIEND_TEST_ALL_PREFIXES(ContentCaptureReceiverTest, RenderFrameHostGone);
 
@@ -68,6 +70,12 @@ class ContentCaptureReceiver : public mojom::ContentCaptureReceiver {
   // ContentCaptureReceiverManager can't get parent frame id in both cases.
   int64_t id_;
   bool content_capture_enabled_ = false;
+
+  // Indicates whether this receiver is visible to consumer. It should be set
+  // upon the |frame_content_capture_data_| is created and reset on the session
+  // removed; the former is caused by either the content captured or the
+  // |frame_content_capture_data_| required by child frame.
+  bool has_session_ = false;
   mojo::AssociatedRemote<mojom::ContentCaptureSender> content_capture_sender_;
   DISALLOW_COPY_AND_ASSIGN(ContentCaptureReceiver);
 };

components/content_capture/browser/content_capture_receiver_manager.cc

@@ -87,6 +87,16 @@ void ContentCaptureReceiverManager::RenderFrameDeleted(
 
 void ContentCaptureReceiverManager::ReadyToCommitNavigation(
     content::NavigationHandle* navigation_handle) {
+  // Don't remove the session for the same document navigation.
+  if (!navigation_handle->IsSameDocument()) {
+    if (auto* rfh = content::RenderFrameHost::FromID(
+            navigation_handle->GetPreviousRenderFrameHostId())) {
+      if (auto* receiver = ContentCaptureReceiverForFrame(rfh)) {
+        receiver->RemoveSession();
+      }
+    }
+  }
+
   if (auto* receiver = ContentCaptureReceiverForFrame(
           navigation_handle->GetRenderFrameHost())) {
     if (web_contents()->GetBrowserContext()->IsOffTheRecord() ||

components/content_capture/browser/content_capture_receiver_test.cc

@@ -9,19 +9,23 @@
 
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/post_task.h"
+#include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "components/content_capture/browser/content_capture_receiver_manager.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/content_features.h"
 #include "content/public/test/test_renderer_host.h"
+#include "content/public/test/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content_capture {
 namespace {
 
-static const char kMainFrameUrl[] = "http://foo.com/main.html";
-static const char kMainFrameUrl2[] = "http://foo.com/2.html";
-static const char kChildFrameUrl[] = "http://foo.org/child.html";
+static constexpr char kMainFrameUrl[] = "http://foo.com/main.html";
+static constexpr char kMainFrameUrl2[] = "http://foo.com/2.html";
+static constexpr char kChildFrameUrl[] = "http://foo.org/child.html";
+static constexpr char kMainFrameSameDocument[] = "http://foo.com/main.html#1";
 
 // Fake ContentCaptureSender to call ContentCaptureReceiver mojom interface.
 class FakeContentCaptureSender {
@@ -151,9 +155,16 @@ class ContentCaptureReceiverManagerHelper
 
 }  // namespace
 
-class ContentCaptureReceiverTest : public content::RenderViewHostTestHarness {
+class ContentCaptureReceiverTest : public content::RenderViewHostTestHarness,
+                                   public ::testing::WithParamInterface<bool> {
  public:
   void SetUp() override {
+    // TODO (crbug.com/1115234): Remove the param when BFCache same site feature
+    // launched.
+    if (GetParam()) {
+      scoped_feature_list_.InitAndEnableFeatureWithParameters(
+          features::kBackForwardCache, {{"enable_same_site", "true"}});
+    }
     content::RenderViewHostTestHarness::SetUp();
     ContentCaptureReceiverManagerHelper::Create(web_contents(),
                                                 &session_removed_test_helper_);
@@ -208,6 +219,11 @@ class ContentCaptureReceiverTest : public content::RenderViewHostTestHarness {
     main_frame_ = web_contents()->GetMainFrame();
   }
 
+  void NavigateMainFrameSameDocument() {
+    content_capture_receiver_manager_helper()->Reset();
+    NavigateAndCommit(GURL(kMainFrameSameDocument));
+  }
+
   void SetupChildFrame() {
     child_content_capture_sender_ =
         std::make_unique<FakeContentCaptureSender>();
@@ -347,9 +363,14 @@ class ContentCaptureReceiverTest : public content::RenderViewHostTestHarness {
   // Expected removed Ids.
   std::vector<int64_t> expected_removed_ids_{2};
   SessionRemovedTestHelper session_removed_test_helper_;
+  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
-TEST_F(ContentCaptureReceiverTest, DidCaptureContent) {
+INSTANTIATE_TEST_SUITE_P(,
+                         ContentCaptureReceiverTest,
+                         testing::Values(true, false));
+
+TEST_P(ContentCaptureReceiverTest, DidCaptureContent) {
   DidCaptureContent(test_data(), true /* first_data */);
   EXPECT_TRUE(
       content_capture_receiver_manager_helper()->parent_session().empty());
@@ -366,7 +387,7 @@ TEST_F(ContentCaptureReceiverTest, DidCaptureContent) {
 #else
 #define MAYBE_DidCaptureContentWithUpdate DidCaptureContentWithUpdate
 #endif
-TEST_F(ContentCaptureReceiverTest, MAYBE_DidCaptureContentWithUpdate) {
+TEST_P(ContentCaptureReceiverTest, MAYBE_DidCaptureContentWithUpdate) {
   DidCaptureContent(test_data(), true /* first_data */);
   // Verifies to get test_data() with correct frame content id.
   EXPECT_TRUE(
@@ -394,7 +415,7 @@ TEST_F(ContentCaptureReceiverTest, MAYBE_DidCaptureContentWithUpdate) {
 #else
 #define MAYBE_DidUpdateContent DidUpdateContent
 #endif
-TEST_F(ContentCaptureReceiverTest, MAYBE_DidUpdateContent) {
+TEST_P(ContentCaptureReceiverTest, MAYBE_DidUpdateContent) {
   DidCaptureContent(test_data(), true /* first_data */);
   EXPECT_TRUE(
       content_capture_receiver_manager_helper()->parent_session().empty());
@@ -415,7 +436,7 @@ TEST_F(ContentCaptureReceiverTest, MAYBE_DidUpdateContent) {
             content_capture_receiver_manager_helper()->updated_data());
 }
 
-TEST_F(ContentCaptureReceiverTest, DidRemoveSession) {
+TEST_P(ContentCaptureReceiverTest, DidRemoveSession) {
   DidCaptureContent(test_data(), true /* first_data */);
   // Verifies to get test_data() with correct frame content id.
   EXPECT_TRUE(
@@ -441,7 +462,7 @@ TEST_F(ContentCaptureReceiverTest, DidRemoveSession) {
             content_capture_receiver_manager_helper()->captured_data());
 }
 
-TEST_F(ContentCaptureReceiverTest, DidRemoveContent) {
+TEST_P(ContentCaptureReceiverTest, DidRemoveContent) {
   DidCaptureContent(test_data(), true /* first_data */);
   // Verifies to get test_data() with correct frame content id.
   EXPECT_TRUE(
@@ -464,7 +485,7 @@ TEST_F(ContentCaptureReceiverTest, DidRemoveContent) {
   VerifySession(expected, content_capture_receiver_manager_helper()->session());
 }
 
-TEST_F(ContentCaptureReceiverTest, ChildFrameDidCaptureContent) {
+TEST_P(ContentCaptureReceiverTest, ChildFrameDidCaptureContent) {
   // Simulate add child frame.
   SetupChildFrame();
   // Simulate to capture the content from main frame.
@@ -493,7 +514,7 @@ TEST_F(ContentCaptureReceiverTest, ChildFrameDidCaptureContent) {
 }
 
 // This test is for issue crbug.com/995121 .
-TEST_F(ContentCaptureReceiverTest, RenderFrameHostGone) {
+TEST_P(ContentCaptureReceiverTest, RenderFrameHostGone) {
   auto* receiver =
       content_capture_receiver_manager_helper()->GetContentCaptureReceiver(
           web_contents()->GetMainFrame());
@@ -515,7 +536,7 @@ TEST_F(ContentCaptureReceiverTest, RenderFrameHostGone) {
 #else
 #define MAYBE_ChildFrameCaptureContentFirst ChildFrameCaptureContentFirst
 #endif
-TEST_F(ContentCaptureReceiverTest, MAYBE_ChildFrameCaptureContentFirst) {
+TEST_P(ContentCaptureReceiverTest, MAYBE_ChildFrameCaptureContentFirst) {
   // Simulate add child frame.
   SetupChildFrame();
   // Simulate to capture the content from child frame.
@@ -545,23 +566,26 @@ TEST_F(ContentCaptureReceiverTest, MAYBE_ChildFrameCaptureContentFirst) {
   BuildChildSession(expected,
                     content_capture_receiver_manager_helper()->captured_data(),
                     &removed_child_session);
-
+  ContentCaptureSession removed_main_session = expected;
   // When main frame navigates to same url, the parent session will not change.
   NavigateMainFrame(GURL(kMainFrameUrl));
   SetupChildFrame();
   DidCaptureContentForChildFrame(test_data2(), true /* first_data */);
   VerifySession(expected,
                 content_capture_receiver_manager_helper()->parent_session());
-  // Verify the child frame is removed.
+
   EXPECT_EQ(
-      1u, content_capture_receiver_manager_helper()->removed_sessions().size());
+      2u, content_capture_receiver_manager_helper()->removed_sessions().size());
   VerifySession(
       removed_child_session,
+      content_capture_receiver_manager_helper()->removed_sessions().back());
+  VerifySession(
+      removed_main_session,
       content_capture_receiver_manager_helper()->removed_sessions().front());
 
   // Get main and child session to verify that they are removed in next
   // navigateion.
-  ContentCaptureSession removed_main_session = expected;
+  removed_main_session = expected;
   BuildChildSession(expected,
                     content_capture_receiver_manager_helper()->captured_data(),
                     &removed_child_session);
@@ -574,7 +598,12 @@ TEST_F(ContentCaptureReceiverTest, MAYBE_ChildFrameCaptureContentFirst) {
   // Intentionally reuse the data.id from previous result, so we know navigating
   // to same domain didn't create new ContentCaptureReceiver when call
   // VerifySession(), otherwise, we can't test the code to handle the navigation
-  // in ContentCaptureReceiver.
+  // in ContentCaptureReceiver.  - except when ProactivelySwapBrowsingInstance
+  // or RenderDocument is enabled on same-site main frame navigation, where we
+  // will get new RenderFrameHosts after the navigation to |kMainFrameUrl2|.
+  if (content::CanSameSiteMainFrameNavigationsChangeRenderFrameHosts())
+    data = GetExpectedTestData(/* main_frame =*/true);
+
   data.value = base::ASCIIToUTF16(kMainFrameUrl2);
   // Currently, there is no way to fake frame size, set it to 0.
   data.bounds = gfx::Rect();
@@ -590,10 +619,10 @@ TEST_F(ContentCaptureReceiverTest, MAYBE_ChildFrameCaptureContentFirst) {
 
   VerifySession(
       removed_child_session,
-      content_capture_receiver_manager_helper()->removed_sessions().front());
+      content_capture_receiver_manager_helper()->removed_sessions().back());
   VerifySession(
       removed_main_session,
-      content_capture_receiver_manager_helper()->removed_sessions().back());
+      content_capture_receiver_manager_helper()->removed_sessions().front());
 
   // Keep current sessions to verify removed sessions later.
   removed_main_session = expected;
@@ -618,10 +647,10 @@ TEST_F(ContentCaptureReceiverTest, MAYBE_ChildFrameCaptureContentFirst) {
       2u, content_capture_receiver_manager_helper()->removed_sessions().size());
   VerifySession(
       removed_child_session,
-      content_capture_receiver_manager_helper()->removed_sessions().front());
+      content_capture_receiver_manager_helper()->removed_sessions().back());
   VerifySession(
       removed_main_session,
-      content_capture_receiver_manager_helper()->removed_sessions().back());
+      content_capture_receiver_manager_helper()->removed_sessions().front());
 
   // Keep current sessions to verify removed sessions later.
   removed_main_session = expected;
@@ -638,6 +667,20 @@ TEST_F(ContentCaptureReceiverTest, MAYBE_ChildFrameCaptureContentFirst) {
                 session_removed_test_helper()->removed_sessions().back());
 }
 
+TEST_P(ContentCaptureReceiverTest, SameDocumentSameSession) {
+  DidCaptureContent(test_data(), true /* first_data */);
+  // Verifies to get test_data() with correct frame content id.
+  EXPECT_TRUE(
+      content_capture_receiver_manager_helper()->parent_session().empty());
+  EXPECT_TRUE(
+      content_capture_receiver_manager_helper()->removed_sessions().empty());
+  EXPECT_EQ(GetExpectedTestData(true /* main_frame */),
+            content_capture_receiver_manager_helper()->captured_data());
+  NavigateMainFrameSameDocument();
+  // Verifies the session wasn't removed for the same document navigation.
+  EXPECT_TRUE(
+      content_capture_receiver_manager_helper()->removed_sessions().empty());
+}
 class ContentCaptureReceiverMultipleFrameTest
     : public ContentCaptureReceiverTest {
  public: