[Oilpan] Remove a raw pointer to PlatformEventController class on Oilpan build

Source/core/frame/PlatformEventDispatcher.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "config.h"
 #include "core/frame/PlatformEventDispatcher.h"
 
 #include "core/frame/PlatformEventController.h"
 #include "wtf/TemporaryChange.h"
 
 namespace blink {
 
 PlatformEventDispatcher::PlatformEventDispatcher()
-    : m_needsPurge(false)
-    , m_isDispatching(false)
+    : m_isDispatching(false)
 {
 }
 
 void PlatformEventDispatcher::addController(PlatformEventController* controller)
 {
+    ASSERT(controller);
+
     bool wasEmpty = m_controllers.isEmpty();
-    if (!m_controllers.contains(controller))
-        m_controllers.append(controller);
+    m_controllers.add(controller);
     if (wasEmpty)
         startListening();
 }
 
 void PlatformEventDispatcher::removeController(PlatformEventController* controller)
 {
-    // Do not actually remove the controller from the vector, instead zero them out.
-    // The zeros are removed in these two cases:
-    // 1. either immediately if we are not dispatching any events,
-    // 2. or after events to all controllers have dispatched (see notifyControllers()).
-    // This is to correctly handle the re-entrancy case when a controller is destroyed
-    // while the events are still being dispatched.
-    size_t index = m_controllers.find(controller);
-    if (index == kNotFound)
+    if (!m_controllers.contains(controller))
         return;
 
-    m_controllers[index] = nullptr;
-    m_needsPurge = true;
-
-    if (!m_isDispatching)
-        purgeControllers();
-}
-
-void PlatformEventDispatcher::purgeControllers()
-{
-    ASSERT(m_needsPurge);
-
-    size_t i = 0;
-    while (i < m_controllers.size()) {
-        if (!m_controllers[i]) {
-            m_controllers[i] = m_controllers.last();
-            m_controllers.removeLast();
-        } else {
-            ++i;
-        }
-    }
-
-    m_needsPurge = false;
-
-    if (m_controllers.isEmpty())
+    m_controllers.remove(controller);
+    if (!m_isDispatching && m_controllers.isEmpty())
         stopListening();
 }
 
 void PlatformEventDispatcher::notifyControllers()
 {
     {
         TemporaryChange<bool> changeIsDispatching(m_isDispatching, true);
-        // Don't notify controllers removed or added during event dispatch.
-        size_t size = m_controllers.size();
-        for (size_t i = 0; i < size; ++i) {
-            if (m_controllers[i])
-                m_controllers[i]->didUpdateData();
+        // m_controllers can be restructured in addController/removeController run on other threads.

[timvolodine, 2015/08/20 11:16:17]

what other threads? I thought blink was single-threaded?

[peria, 2015/08/21 03:33:45]

Oops, it comes from my misunderstanding.
Dropped this comment.

+        // Thus we take a snapshot and restore into a Vector to access all elements.
+        WillBeHeapHashSet<RawPtrWillBeWeakMember<PlatformEventController>> snapshotSet(m_controllers);
+        HeapVector<PlatformEventController*> snapshotVector;
+        for (PlatformEventController* controller : snapshotSet) {
+            snapshotVector.append(controller);

[timvolodine, 2015/08/20 11:16:17]

why is it necessary to create both snapshotVector and snapshotSet?

[peria, 2015/08/21 03:33:45]

snapshotSet was introduced to avoid accessing newly added controller,
and snapshotVector was introduced to access all elements in snapshotSet.

For the details of latter one, didUpdateData() may delete the
controller itself, and it means an element of snapshotSet is removed instead of
turning to a nullptr.
Removing an element can cause a reconstruction of HeapHashSet, and
then we cannot continue the iteration in the set.

[peria, 2015/08/21 03:33:45]

We may remove using snapshotSet.

[timvolodine, 2015/08/21 15:03:32]

yeah looks like it's not really needed

+        }
+        for (PlatformEventController* controller : snapshotVector) {
+            // Skip a controller which is removed during dispatching.
+            if (m_controllers.contains(controller))

[timvolodine, 2015/08/20 11:16:17]

also, how expensive is 'contains' here? the notifyControllers code can be high
frequency as in the case of gamepad and device motion, so it's important to keep
this code efficient.

[timvolodine, 2015/08/20 11:16:17]

is it possible to remove and then re-add the same controller during dispatch? in
which case it would still be called, while not in the previous implementation.
Not sure if this is an issue though.

[peria, 2015/08/21 03:33:45]

I think we can, but am not sure.

[peria, 2015/08/21 03:33:45]

Hmm, unfortunately I don't know the performance regression/improvement of this
method.
I think we can simply remove this line, if "controller->didUpdateData()" does
not delete other controllers.

[peria, 2015/08/21 05:34:19]

It must be more expensive than checking a nullptr, but I believe it runs fast
enough to be used here.

[timvolodine, 2015/08/21 15:03:32]

we cannot get rid of 'contains' because of re-entrancy of 'didUpdateData' (via
JavaScript).
looks like it would be O(1) because you are using a HashSet for m_controllers
now.

+                controller->didUpdateData();
         }
     }
 
-    if (m_needsPurge)
-        purgeControllers();
+    if (m_controllers.isEmpty())
+        stopListening();
 }
 
 DEFINE_TRACE(PlatformEventDispatcher)
 {
 #if ENABLE(OILPAN)
-    // Trace the backing store, the weak(&bare) element references won't be.
     visitor->trace(m_controllers);
-    visitor->template registerWeakMembers<PlatformEventDispatcher, &PlatformEventDispatcher::clearWeakMembers>(this);
 #endif
 }
 
-#if ENABLE(OILPAN)
-void PlatformEventDispatcher::clearWeakMembers(Visitor* visitor)
-{
-    for (size_t i = 0; i < m_controllers.size(); ++i) {
-        if (!Heap::isHeapObjectAlive(m_controllers[i])) {
-            m_controllers[i] = nullptr;
-            m_needsPurge = true;
-        }
-    }
-    // Next notification will purge the empty slots.
-}
-#endif
-
 } // namespace blink