Oilpan: have LifetimeNotifier<T> track its observers weakly.

Source/platform/LifecycleNotifier.h

 /*
  * Copyright (C) 2008 Apple Inc. All Rights Reserved.
  * Copyright (C) 2013 Google Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 16 matching lines @@
 #ifndef LifecycleNotifier_h
 #define LifecycleNotifier_h
 
 #include "platform/heap/Handle.h"
 #include "wtf/HashSet.h"
 #include "wtf/TemporaryChange.h"
 
 namespace blink {
 
 template<typename T, typename Observer>
 class LifecycleNotifier {

[haraken, 2015/03/16 23:44:22]

Just help me understand: Why is it safe to use WillBeHeapHashSet without making
LifecycleNotifier a GCMixin?

[sof, 2015/03/17 06:24:06]

It's kept safe by not handling LifecycleNotifier<>* references anywhere; it's
unlikely that that will start to be done later also. Objects that implement this
interface must call its trace method, but this isn't checked by the plugin (I
think..haven't actually tested.)

Not ideal, but that's my safety argument.

[haraken, 2015/03/17 08:29:36]

Thanks for the clarification; makes sense.

[haraken, 2015/03/17 09:15:36]

Just to confirm: The reason it's safe not to make LifecycleNotifier GCMixin is
that no one keeps a reference to the LifecycleNotifier and thus there is no case
where we have to adjustAndMark a pointer to the LifecycleNotifier, right?

[sof, 2015/03/17 09:28:06]

Yes, coupled with conservative GCs being locked out during construction of the
objects that implement this notifier interface, an on-stack reference to this
object will not be encountered. Not that it would be harmful, as the heap hash
set would be empty then & without any allocations attached.

Clearly this is not a satisfactory state to leave things at, longer-term.

 public:
     virtual ~LifecycleNotifier();
 
     void addObserver(Observer*);
     void removeObserver(Observer*);
 
     // notifyContextDestroyed() should be explicitly dispatched from an
     // observed context to notify observers that contextDestroyed().
     //
     // When contextDestroyed() is called, the observer's lifecycleContext()
     // is still valid and safe to use during the notification.
     virtual void notifyContextDestroyed();
 
-    DEFINE_INLINE_VIRTUAL_TRACE() { }
+    DEFINE_INLINE_VIRTUAL_TRACE()
+    {
+#if ENABLE(OILPAN)
+        visitor->trace(m_observers);
+#endif
+    }
 
     bool isIteratingOverObservers() const { return m_iterating != IteratingNone; }
 
 protected:
     LifecycleNotifier()
         : m_iterating(IteratingNone)
         , m_didCallContextDestroyed(false)
     {
     }
 
     enum IterationType {
         IteratingNone,
         IteratingOverAll,
         IteratingOverActiveDOMObjects,
     };
 
     IterationType m_iterating;
 
 protected:
-    using ObserverSet = HashSet<Observer*>;
+    using ObserverSet = WillBeHeapHashSet<RawPtrWillBeWeakMember<Observer>>;
 
+    // FIXME: Oilpan: make LifecycleNotifier<> a GC mixin, somehow. ExecutionContext
+    // is the problematic case, as it would then be a class with two GC mixin
+    // bases, but cannot itself derive from a GC base class also.

[haraken, 2015/03/16 23:44:22]

Regarding the diamond inheritance problem, can we use the approach in
https://codereview.chromium.org/916033002/?

[sof, 2015/03/17 06:24:06]

A big CL; what are you referring to?

[haraken, 2015/03/17 08:29:36]

The public virtual inheritance in Supplementable.h and LifecycleNotifier.h.

https://codereview.chromium.org/916033002/diff/1/Source/platform/Supplementab...

[sof, 2015/03/17 08:51:46]

Thanks; virtual inheritance brings overhead and complexity that needs to be
considered with some care. crbug.com/467502 seems like a natural place for that.

+    GC_PLUGIN_IGNORE("467502")
     ObserverSet m_observers;
 
 #if ENABLE(ASSERT)
     T* context() { return static_cast<T*>(this); }
 #endif
 
 private:
     bool m_didCallContextDestroyed;
 };
 
@@ skipping 45 matching lines @@
 
 template<typename T, typename Observer>
 inline void LifecycleNotifier<T, Observer>::removeObserver(Observer* observer)
 {
     m_observers.remove(observer);
 }
 
 } // namespace blink
 
 #endif // LifecycleNotifier_h