[SessionStorageS13N] Observer support for clearing site data

This uses the (previously unused by SessionStorage) StorageArea's
observer interface to notify the renderer when session storage is
cleared by the browser. This change
* Updates the service to notify observers in this way
* Adds tests in the service (& cleans up old observer usage)
* Modifies WebStorageNamespace to include a WebString namespace getter.
  This lets the IsSameNamespace comparision to not cause undefined
  behavior when comparing different subclasses of WebStorageNamespace.

R=mek@chromium.org

Bug: 856120
Change-Id: I97e2d7a8f7a2b660143bf37bcbd04f5a3985bce2
Reviewed-on: https://chromium-review.googlesource.com/1114300Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570568}

content/browser/dom_storage/session_storage_area_impl.cc

@@ -5,6 +5,8 @@
 #include "content/browser/dom_storage/session_storage_area_impl.h"
 
 #include "base/bind.h"
+#include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "content/browser/dom_storage/session_storage_data_map.h"
 #include "content/common/dom_storage/dom_storage_types.h"
@@ -44,14 +46,20 @@ std::unique_ptr<SessionStorageAreaImpl> SessionStorageAreaImpl::Clone(
       namespace_entry, origin_, shared_data_map_, register_new_map_callback_));
 }
 
+void SessionStorageAreaImpl::NotifyObserversAllDeleted() {
+  observers_.ForAllPtrs([](blink::mojom::StorageAreaObserver* observer) {
+    // Renderer process expects |source| to always be two newline separated
+    // strings.
+    observer->AllDeleted("\n");
+  });
+}
+
 // blink::mojom::StorageArea:
 void SessionStorageAreaImpl::AddObserver(
     blink::mojom::StorageAreaObserverAssociatedPtrInfo observer) {
   blink::mojom::StorageAreaObserverAssociatedPtr observer_ptr;
   observer_ptr.Bind(std::move(observer));
-  mojo::InterfacePtrSetElementId ptr_id =
-      shared_data_map_->storage_area()->AddObserver(std::move(observer_ptr));
-  observer_ptrs_.push_back(ptr_id);
+  observers_.AddPtr(std::move(observer_ptr));
 }
 
 void SessionStorageAreaImpl::Put(
@@ -122,13 +130,6 @@ void SessionStorageAreaImpl::OnConnectionError() {
 void SessionStorageAreaImpl::CreateNewMap(
     NewMapType map_type,
     const base::Optional<std::string>& delete_all_source) {
-  std::vector<blink::mojom::StorageAreaObserverAssociatedPtr> ptrs_to_move;
-  for (const mojo::InterfacePtrSetElementId& ptr_id : observer_ptrs_) {
-    DCHECK(shared_data_map_->storage_area()->HasObserver(ptr_id));
-    ptrs_to_move.push_back(
-        shared_data_map_->storage_area()->RemoveObserver(ptr_id));
-  }
-  observer_ptrs_.clear();
   shared_data_map_->RemoveBindingReference();
   switch (map_type) {
     case NewMapType::FORKED:
@@ -145,18 +146,10 @@ void SessionStorageAreaImpl::CreateNewMap(
           shared_data_map_->listener(),
           register_new_map_callback_.Run(namespace_entry_, origin_),
           shared_data_map_->storage_area()->database());
-      for (auto& ptr : ptrs_to_move) {
-        ptr->AllDeleted(delete_all_source.value_or("\n"));
-      }
       break;
     }
   }
   shared_data_map_->AddBindingReference();
-
-  for (auto& observer_ptr : ptrs_to_move) {
-    observer_ptrs_.push_back(
-        shared_data_map_->storage_area()->AddObserver(std::move(observer_ptr)));
-  }
 }
 
 }  // namespace content

content/browser/dom_storage/session_storage_area_impl.h

@@ -29,6 +29,10 @@ class SessionStorageDataMap;
 // cloning (copy-on-write). This should be done through the |Clone()| method and
 // not manually.
 //
+// Unlike regular observers, adding an observer to this class only notifies when
+// the whole area is deleted manually (not by using DeleteAll on this binding).
+// This can happen when clearing browser data.
+//
 // During forking, this class is responsible for dealing with moving its
 // observers from the SessionStorageDataMap's StorageArea to the new forked
 // SessionStorageDataMap's StorageArea.
@@ -61,6 +65,9 @@ class CONTENT_EXPORT SessionStorageAreaImpl : public blink::mojom::StorageArea {
 
   SessionStorageDataMap* data_map() { return shared_data_map_.get(); }
 
+  // Notifies all observers that this area was deleted.
+  void NotifyObserversAllDeleted();
+
   // blink::mojom::StorageArea:
   void AddObserver(
       blink::mojom::StorageAreaObserverAssociatedPtrInfo observer) override;
@@ -93,7 +100,7 @@ class CONTENT_EXPORT SessionStorageAreaImpl : public blink::mojom::StorageArea {
   scoped_refptr<SessionStorageDataMap> shared_data_map_;
   RegisterNewAreaMap register_new_map_callback_;
 
-  std::vector<mojo::InterfacePtrSetElementId> observer_ptrs_;
+  mojo::AssociatedInterfacePtrSet<blink::mojom::StorageAreaObserver> observers_;
   mojo::AssociatedBinding<blink::mojom::StorageArea> binding_;
 
   DISALLOW_COPY_AND_ASSIGN(SessionStorageAreaImpl);

content/browser/dom_storage/session_storage_area_impl_unittest.cc

@@ -224,7 +224,7 @@ TEST_F(SessionStorageAreaImplTest, Cloning) {
   ss_leveldb_impl2 = nullptr;
 }
 
-TEST_F(SessionStorageAreaImplTest, ObserverTransfer) {
+TEST_F(SessionStorageAreaImplTest, NotifyAllDeleted) {
   EXPECT_CALL(listener_,
               OnDataMapCreation(StdStringToUint8Vector("0"), testing::_))
       .Times(1);
@@ -238,69 +238,26 @@ TEST_F(SessionStorageAreaImplTest, ObserverTransfer) {
           leveldb_database_.get()),
       GetRegisterNewAreaMapCallback());
 
-  // Create the mojo binding.
   blink::mojom::StorageAreaAssociatedPtr ss_leveldb1;
   ss_leveldb_impl1->Bind(
       mojo::MakeRequestAssociatedWithDedicatedPipe(&ss_leveldb1));
 
-  // Create the observer, and attach it to the mojo bound implementation.
   testing::StrictMock<test::MockLevelDBObserver> mock_observer;
   mojo::AssociatedBinding<blink::mojom::StorageAreaObserver> observer_binding(
       &mock_observer);
   blink::mojom::StorageAreaObserverAssociatedPtrInfo observer_ptr_info;
   observer_binding.Bind(mojo::MakeRequest(&observer_ptr_info));
   ss_leveldb1->AddObserver(std::move(observer_ptr_info));
+  ss_leveldb1.FlushForTesting();
 
-  // Perform a shallow clone.
-  std::vector<leveldb::mojom::BatchedOperationPtr> save_operations;
-  metadata_.RegisterShallowClonedNamespace(
-      metadata_.GetOrCreateNamespaceEntry(test_namespace_id1_),
-      metadata_.GetOrCreateNamespaceEntry(test_namespace_id2_),
-      &save_operations);
-  leveldb_database_->Write(std::move(save_operations), base::DoNothing());
-  auto ss_leveldb_impl2 = ss_leveldb_impl1->Clone(
-      metadata_.GetOrCreateNamespaceEntry(test_namespace_id2_));
-  blink::mojom::StorageAreaAssociatedPtr ss_leveldb2;
-  ss_leveldb_impl2->Bind(
-      mojo::MakeRequestAssociatedWithDedicatedPipe(&ss_leveldb2));
-
-  // Wait for our future commits to finish.
-  base::RunLoop commit_waiters;
-  auto barrier = base::BarrierClosure(3, commit_waiters.QuitClosure());
-  EXPECT_CALL(listener_, OnCommitResult(DatabaseError::OK))
-      .Times(3)
-      .WillRepeatedly(base::test::RunClosure(barrier));
-
-  // Do a change on the new interface. There should be no observation.
-  EXPECT_CALL(listener_,
-              OnDataMapCreation(StdStringToUint8Vector("1"), testing::_))
-      .Times(1);
-  EXPECT_TRUE(test::PutSync(ss_leveldb2.get(), StdStringToUint8Vector("key2"),
-                            StdStringToUint8Vector("data2"), base::nullopt,
-                            ""));
-  ss_leveldb_impl2->data_map()->storage_area()->ScheduleImmediateCommit();
-
-  // Do a change on the old interface.
-  EXPECT_CALL(mock_observer,
-              KeyChanged(StdStringToUint8Vector("key1"),
-                         StdStringToUint8Vector("data2"),
-                         StdStringToUint8Vector("data1"), "ss_leveldb1"))
-      .Times(1);
-  EXPECT_TRUE(test::PutSync(ss_leveldb1.get(), StdStringToUint8Vector("key1"),
-                            StdStringToUint8Vector("data2"),
-                            StdStringToUint8Vector("data1"), "ss_leveldb1"));
-  ss_leveldb_impl1->data_map()->storage_area()->ScheduleImmediateCommit();
-
-  // Wait for the commits.
-  commit_waiters.Run();
+  base::RunLoop loop;
+  EXPECT_CALL(mock_observer, AllDeleted("\n"))
+      .WillOnce(base::test::RunClosure(loop.QuitClosure()));
+  ss_leveldb_impl1->NotifyObserversAllDeleted();
+  loop.Run();
 
   EXPECT_CALL(listener_, OnDataMapDestruction(StdStringToUint8Vector("0")))
       .Times(1);
-  EXPECT_CALL(listener_, OnDataMapDestruction(StdStringToUint8Vector("1")))
-      .Times(1);
-
-  ss_leveldb_impl1 = nullptr;
-  ss_leveldb_impl2 = nullptr;
 }
 
 TEST_F(SessionStorageAreaImplTest, DeleteAllOnShared) {
@@ -337,7 +294,9 @@ TEST_F(SessionStorageAreaImplTest, DeleteAllOnShared) {
   // Same maps are used.
   EXPECT_EQ(ss_leveldb_impl1->data_map(), ss_leveldb_impl2->data_map());
 
-  // Create the observer, attach to the first namespace.
+  // Create the observer, attach to the first namespace, and verify we don't see
+  // any changes (see SessionStorageAreaImpl class comment about when observers
+  // are called).
   testing::StrictMock<test::MockLevelDBObserver> mock_observer;
   mojo::AssociatedBinding<blink::mojom::StorageAreaObserver> observer_binding(
       &mock_observer);
@@ -353,7 +312,6 @@ TEST_F(SessionStorageAreaImplTest, DeleteAllOnShared) {
   // There should be no commits, as we don't actually have to change any data.
   // |ss_leveldb_impl1| should just switch to a new, empty map.
   EXPECT_CALL(listener_, OnCommitResult(DatabaseError::OK)).Times(0);
-  EXPECT_CALL(mock_observer, AllDeleted("source")).Times(1);
   EXPECT_TRUE(test::DeleteAllSync(ss_leveldb1.get(), "source"));
 
   // The maps were forked on the above call.

content/browser/dom_storage/session_storage_context_mojo_unittest.cc

@@ -616,13 +616,6 @@ TEST_F(SessionStorageContextMojoTest, RecreateOnCommitFailure) {
     loop.Run();
   }
 
-  // Add observers to the first two connections.
-  testing::StrictMock<test::MockLevelDBObserver> observer1;
-  area1->AddObserver(observer1.Bind());
-  EXPECT_FALSE(area1.encountered_error());
-  testing::StrictMock<test::MockLevelDBObserver> observer3;
-  area3->AddObserver(observer3.Bind());
-
   // Verify one attempt was made to open the database, and connect that request
   // with a database implementation that always fails on write.
   ASSERT_EQ(1u, fake_leveldb_service.open_requests().size());
@@ -645,9 +638,6 @@ TEST_F(SessionStorageContextMojoTest, RecreateOnCommitFailure) {
   // pending commit that will get cancelled when the database connection is
   // closed.
   auto value = leveldb::StringPieceToUint8Vector("avalue");
-  EXPECT_CALL(observer3, KeyAdded(leveldb::StringPieceToUint8Vector("w3key"),
-                                  value, "source"))
-      .Times(1);
   area3->Put(leveldb::StringPieceToUint8Vector("w3key"), value, base::nullopt,
              "source",
              base::BindOnce([](bool success) { EXPECT_TRUE(success); }));
@@ -663,17 +653,6 @@ TEST_F(SessionStorageContextMojoTest, RecreateOnCommitFailure) {
     value[0]++;
     area1.set_connection_error_handler(put_loop.QuitClosure());
 
-    if (i == 1) {
-      EXPECT_CALL(observer1, ShouldSendOldValueOnMutations(false)).Times(1);
-      EXPECT_CALL(observer1, KeyAdded(leveldb::StringPieceToUint8Vector("key"),
-                                      value, "source"))
-          .Times(1);
-    } else {
-      EXPECT_CALL(observer1,
-                  KeyChanged(leveldb::StringPieceToUint8Vector("key"), value,
-                             old_value, "source"))
-          .Times(1);
-    }
     area1->Put(leveldb::StringPieceToUint8Vector("key"), value, base::nullopt,
                "source", base::BindLambdaForTesting([&](bool success) {
                  EXPECT_TRUE(success);

content/browser/dom_storage/session_storage_namespace_impl_mojo.cc

@@ -128,6 +128,7 @@ void SessionStorageNamespaceImplMojo::RemoveOriginData(
   // Renderer process expects |source| to always be two newline separated
   // strings.
   it->second->DeleteAll("\n", base::DoNothing());
+  it->second->NotifyObserversAllDeleted();
   it->second->data_map()->storage_area()->ScheduleImmediateCommit();
 }
 

content/browser/dom_storage/session_storage_namespace_impl_mojo_unittest.cc

@@ -14,6 +14,7 @@
 #include "content/browser/dom_storage/test/storage_area_test_util.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/test/fake_leveldb_database.h"
+#include "content/test/gmock_util.h"
 #include "mojo/edk/embedder/embedder.h"
 #include "mojo/public/cpp/bindings/strong_associated_binding.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -378,6 +379,19 @@ TEST_F(SessionStorageNamespaceImplMojoTest, RemoveOriginData) {
   ss_namespace->OpenArea(test_origin1_, mojo::MakeRequest(&leveldb_1));
   ss_namespace.FlushForTesting();
 
+  // Create an observer to make sure the deletion is observed.
+  testing::StrictMock<test::MockLevelDBObserver> mock_observer;
+  mojo::AssociatedBinding<blink::mojom::StorageAreaObserver> observer_binding(
+      &mock_observer);
+  blink::mojom::StorageAreaObserverAssociatedPtrInfo observer_ptr_info;
+  observer_binding.Bind(mojo::MakeRequest(&observer_ptr_info));
+  leveldb_1->AddObserver(std::move(observer_ptr_info));
+  leveldb_1.FlushForTesting();
+
+  base::RunLoop loop;
+  EXPECT_CALL(mock_observer, AllDeleted("\n"))
+      .WillOnce(base::test::RunClosure(loop.QuitClosure()));
+
   EXPECT_CALL(listener_, OnCommitResult(DatabaseError::OK)).Times(1);
   namespace_impl->RemoveOriginData(test_origin1_);
 
@@ -385,6 +399,9 @@ TEST_F(SessionStorageNamespaceImplMojoTest, RemoveOriginData) {
   EXPECT_TRUE(test::GetAllSync(leveldb_1.get(), &data));
   EXPECT_EQ(0ul, data.size());
 
+  // Check that the observer was notified.
+  loop.Run();
+
   EXPECT_CALL(listener_, OnDataMapDestruction(StdStringToUint8Vector("0")))
       .Times(1);
   namespaces_.clear();

content/renderer/dom_storage/local_storage_cached_area.cc

@@ -93,6 +93,7 @@ LocalStorageCachedArea::LocalStorageCachedArea(
   blink::mojom::StorageAreaObserverAssociatedPtrInfo ptr_info;
   binding_.Bind(mojo::MakeRequest(&ptr_info),
                 main_thread_scheduler->IPCTaskRunner());
+  leveldb_->AddObserver(std::move(ptr_info));
 }
 
 LocalStorageCachedArea::LocalStorageCachedArea(
@@ -436,7 +437,6 @@ void LocalStorageCachedArea::KeyDeleted(const std::vector<uint8_t>& key,
 }
 
 void LocalStorageCachedArea::AllDeleted(const std::string& source) {
-  DCHECK(!IsSessionStorage());
   GURL page_url;
   std::string storage_area_id;
   UnpackSource(source, &page_url, &storage_area_id);
@@ -460,9 +460,18 @@ void LocalStorageCachedArea::AllDeleted(const std::string& source) {
     }
   }
 
-  blink::WebStorageEventDispatcher::DispatchLocalStorageEvent(
-      blink::WebString(), blink::WebString(), blink::WebString(),
-      origin_.GetURL(), page_url, originating_area);
+  if (IsSessionStorage()) {
+    SessionWebStorageNamespaceImpl session_namespace_for_event_dispatch(
+        namespace_id_, nullptr);
+    blink::WebStorageEventDispatcher::DispatchSessionStorageEvent(
+        blink::WebString(), blink::WebString(), blink::WebString(),
+        origin_.GetURL(), page_url, session_namespace_for_event_dispatch,
+        originating_area);
+  } else {
+    blink::WebStorageEventDispatcher::DispatchLocalStorageEvent(
+        blink::WebString(), blink::WebString(), blink::WebString(),
+        origin_.GetURL(), page_url, originating_area);
+  }
 }
 
 void LocalStorageCachedArea::ShouldSendOldValueOnMutations(bool value) {

content/renderer/dom_storage/local_storage_namespace.cc

@@ -24,6 +24,10 @@ LocalStorageNamespace::LocalStorageNamespace(
 LocalStorageNamespace::~LocalStorageNamespace() {
 }
 
+blink::WebString LocalStorageNamespace::GetNamespaceId() const {
+  return blink::WebString();
+}
+
 WebStorageArea* LocalStorageNamespace::CreateStorageArea(
     const blink::WebSecurityOrigin& origin) {
   return new LocalStorageArea(

content/renderer/dom_storage/local_storage_namespace.h

@@ -22,6 +22,9 @@ class LocalStorageNamespace : public blink::WebStorageNamespace {
   // blink::WebStorageNamespace:
   blink::WebStorageArea* CreateStorageArea(
       const blink::WebSecurityOrigin& origin) override;
+
+  blink::WebString GetNamespaceId() const override;
+
   bool IsSameNamespace(const WebStorageNamespace&) const override;
 
  private:

content/renderer/dom_storage/session_web_storage_namespace_impl.cc

@@ -31,11 +31,13 @@ WebStorageArea* SessionWebStorageNamespaceImpl::CreateStorageArea(
                                                          origin));
 }
 
+blink::WebString SessionWebStorageNamespaceImpl::GetNamespaceId() const {
+  return blink::WebString::FromASCII(namespace_id_);
+}
+
 bool SessionWebStorageNamespaceImpl::IsSameNamespace(
     const WebStorageNamespace& other) const {
-  const SessionWebStorageNamespaceImpl* other_impl =
-      static_cast<const SessionWebStorageNamespaceImpl*>(&other);
-  return namespace_id_ == other_impl->namespace_id_;
+  return GetNamespaceId() == other.GetNamespaceId();
 }
 
 }  // namespace content

content/renderer/dom_storage/session_web_storage_namespace_impl.h

@@ -23,6 +23,7 @@ class SessionWebStorageNamespaceImpl : public blink::WebStorageNamespace {
   // blink::WebStorageNamespace:
   blink::WebStorageArea* CreateStorageArea(
       const blink::WebSecurityOrigin& origin) override;
+  blink::WebString GetNamespaceId() const override;
   bool IsSameNamespace(const WebStorageNamespace&) const override;
 
  private:

content/renderer/dom_storage/webstoragenamespace_impl.cc

@@ -39,11 +39,13 @@ WebStorageNamespace* WebStorageNamespaceImpl::copy() {
   return nullptr;
 }
 
+blink::WebString WebStorageNamespaceImpl::GetNamespaceId() const {
+  return blink::WebString::FromASCII(namespace_id_);
+}
+
 bool WebStorageNamespaceImpl::IsSameNamespace(
     const WebStorageNamespace& other) const {
-  const WebStorageNamespaceImpl* other_impl =
-      static_cast<const WebStorageNamespaceImpl*>(&other);
-  return namespace_id_ == other_impl->namespace_id_;
+  return GetNamespaceId() == other.GetNamespaceId();
 }
 
 }  // namespace content

content/renderer/dom_storage/webstoragenamespace_impl.h

@@ -21,6 +21,7 @@ class WebStorageNamespaceImpl : public blink::WebStorageNamespace {
   blink::WebStorageArea* CreateStorageArea(
       const blink::WebSecurityOrigin& origin) override;
   virtual blink::WebStorageNamespace* copy();
+  blink::WebString GetNamespaceId() const override;
   bool IsSameNamespace(const WebStorageNamespace&) const override;
 
  private:

third_party/blink/public/platform/web_storage_namespace.h

@@ -32,6 +32,7 @@
 #define THIRD_PARTY_BLINK_PUBLIC_PLATFORM_WEB_STORAGE_NAMESPACE_H_
 
 #include "third_party/blink/public/platform/web_common.h"
+#include "third_party/blink/public/platform/web_string.h"
 
 namespace blink {
 
@@ -52,6 +53,8 @@ class WebStorageNamespace {
   // finished.
   virtual WebStorageArea* CreateStorageArea(const WebSecurityOrigin&) = 0;
 
+  virtual WebString GetNamespaceId() const = 0;
+
   // Returns true of the two instances represent the same storage namespace.
   virtual bool IsSameNamespace(const WebStorageNamespace&) const {
     return false;