Make DocumentMarkerController to detect empty marker set after processing weak member processing

This patch introduces weak member callback in |DocumentMarkerController| to
detect empty weak hash map |markers_| to avoid unregistering mutation observer
during mutation callback.

This patch is follow-up of the patch[1].

[1] http://crrev.com/c/1137988 Blink Editing: Workaround unsafe iteration in
DocumentMarkerController.

TBR=jbroman@chromium.org

Bug: 862960
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Icd6acfed2ca00dabc4ed79959df2fdf0bd6fce4b
Reviewed-on: https://chromium-review.googlesource.com/1143113Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577130}

third_party/blink/renderer/core/dom/synchronous_mutation_notifier.cc

@@ -46,14 +46,7 @@ void SynchronousMutationNotifier::NotifyUpdateCharacterData(
     unsigned offset,
     unsigned old_length,
     unsigned new_length) {
-  // Using ForEachObserverWithoutChecks() instead of ForEachObserver() is
-  // necessary because DocumentMarkerController::DidUpdateCharacterData ends up
-  // calling SynchronousMutationNotifier::RemoveObserver, which is unsafe and
-  // can result in memory corruption.
-  //
-  // TODO(crbug.com/862900): Fix DocumentMarkerController and switch to
-  //                         ForEachObsever() here.
-  ForEachObserverWithoutChecks([&](SynchronousMutationObserver* observer) {
+  ForEachObserver([&](SynchronousMutationObserver* observer) {
     observer->DidUpdateCharacterData(character_data, offset, old_length,
                                      new_length);
   });

third_party/blink/renderer/core/editing/markers/document_marker_controller.cc

@@ -129,27 +129,14 @@ Member<DocumentMarkerList>& DocumentMarkerController::ListForType(
 }
 
 bool DocumentMarkerController::PossiblyHasMarkers(
-    DocumentMarker::MarkerType type) {
+    DocumentMarker::MarkerType type) const {
   return PossiblyHasMarkers(DocumentMarker::MarkerTypes(type));
 }
 
 inline bool DocumentMarkerController::PossiblyHasMarkers(
-    DocumentMarker::MarkerTypes types) {
-  if (markers_.IsEmpty()) {
-    // It's possible for markers_ to become empty through garbage collection if
-    // all its Nodes are GC'ed since we only hold weak references, in which case
-    // possibly_existing_marker_types_ isn't reset to 0 as it is in the other
-    // codepaths that remove from markers_. Therefore, we check for this case
-    // here.
-
-    // Alternatively, we could handle this case at the time the Node is GC'ed,
-    // but that operation is more performance-sensitive than anywhere
-    // PossiblyHasMarkers() is used.
-    possibly_existing_marker_types_ = DocumentMarker::MarkerTypes();
-    SetContext(nullptr);
-    return false;
-  }
-
+    DocumentMarker::MarkerTypes types) const {
+  DCHECK(!markers_.IsEmpty() ||
+         possibly_existing_marker_types_ == DocumentMarker::MarkerTypes(0));
   return possibly_existing_marker_types_.Intersects(types);
 }
 
@@ -662,7 +649,17 @@ void DocumentMarkerController::InvalidateRectsForAllTextMatchMarkers() {
   }
 }
 
+void DocumentMarkerController::DidProcessMarkerMap(Visitor* visitor) {
+  if (markers_.IsEmpty())
+    Clear();
+}
+
 void DocumentMarkerController::Trace(blink::Visitor* visitor) {
+  // Note: To make |DidProcessMarkerMap()| called after weak members callback
+  // of |markers_|, we should register it before tracing |markers_|.
+  visitor->template RegisterWeakMembers<
+      DocumentMarkerController, &DocumentMarkerController::DidProcessMarkerMap>(
+      this);
   visitor->Trace(markers_);
   visitor->Trace(document_);
   SynchronousMutationObserver::Trace(visitor);

third_party/blink/renderer/core/editing/markers/document_marker_controller.h

@@ -152,8 +152,8 @@ class CORE_EXPORT DocumentMarkerController final
   using MarkerMap = HeapHashMap<WeakMember<const Node>, Member<MarkerLists>>;
   static Member<DocumentMarkerList>& ListForType(MarkerLists*,
                                                  DocumentMarker::MarkerType);
-  bool PossiblyHasMarkers(DocumentMarker::MarkerTypes);
-  bool PossiblyHasMarkers(DocumentMarker::MarkerType);
+  bool PossiblyHasMarkers(DocumentMarker::MarkerTypes) const;
+  bool PossiblyHasMarkers(DocumentMarker::MarkerType) const;
   void RemoveMarkersFromList(MarkerMap::iterator, DocumentMarker::MarkerTypes);
   void RemoveMarkers(TextIterator&, DocumentMarker::MarkerTypes);
   void RemoveMarkersInternal(const Node&,
@@ -161,6 +161,9 @@ class CORE_EXPORT DocumentMarkerController final
                              int length,
                              DocumentMarker::MarkerTypes);
 
+  // Called after weak processing of |markers_| is done.
+  void DidProcessMarkerMap(Visitor* visitor);
+
   MarkerMap markers_;
   // Provide a quick way to determine whether a particular marker type is absent
   // without going through the map.

third_party/blink/renderer/core/editing/markers/document_marker_controller_test.cc

@@ -189,10 +189,8 @@ TEST_F(DocumentMarkerControllerTest, NodeWillBeRemovedBySetInnerHTML) {
   EXPECT_EQ(0u, MarkerController().Markers().size());
 }
 
-// TODO(crbug.com/862900): Fix DocumentMarkerController::DidUpdateCharacterData
-//                         and enable this test.
-TEST_F(DocumentMarkerControllerTest,
-       DISABLED_SynchronousMutationNotificationAfterGC) {
+// For http://crbug.com/862900
+TEST_F(DocumentMarkerControllerTest, SynchronousMutationNotificationAfterGC) {
   SetBodyContent("<b><i>foo</i></b>");
   Persistent<Text> sibling_text = CreateTextNode("bar");
   {

third_party/blink/renderer/platform/lifecycle_notifier.h

@@ -81,20 +81,6 @@ class LifecycleNotifier : public GarbageCollectedMixin {
     }
   }
 
-  // ForEachObserver() variant that does not protect against memory corruption.
-  //
-  // Only used by SynchronousMutationNotifier::NotifyUpdateCharacterData. See
-  // implementation comment for why it is necessary.
-  //
-  // TODO(crbug.com/862900): Fix SynchronousMutationNotifier and remove this.
-  template <typename ForEachCallable>
-  void ForEachObserverWithoutChecks(const ForEachCallable& callable) const {
-    for (LifecycleObserverBase* observer_base : observers_) {
-      Observer* observer = static_cast<Observer*>(observer_base);
-      callable(observer);
-    }
-  }
-
  private:
   using ObserverSet = HeapHashSet<WeakMember<LifecycleObserverBase>>;