bindings, heap: Avoid reading in-construction on ASW::Trace

The in-construction bit is considered an implementation detail of the
garbage collector and should not be relied on by user objects.

ActiveScriptWrappable objects are queried by the garbage collector for
pending activities and rooted as long as there are activities pending.

Since the GC may interrupt construction of ASW objects the virtual
methods for HasPendingActivity() and potentially GetExecutionContext()
may not be called.

While ASW(Base) HasPendingActivity() may provide a default body for
HasPendingActivity() and thus work around the construction problem,
the call to GetExecutionContext() is dispatched through CRTP and there
are three different use cases:
a) GetExecutionContext() may still be pure virtual at ASW
registration (EventTarget).
b) GetExecutionContext() may already be marked as final at ASW
registration (Node)
c) GetExecutionContext() may be a regular method (users of
ExecutionContextClient)

Use case a) would require a default body for GetExecutionContext in
ASW while use case b) prohibits that. The other alternative would have
been adding
  virtual ScriptWrappable::GetExecutionContext() { return nullptr; }

Since ExecutionContext is not a concept of platform layering would be
violated. (Even though just a forward declaration would suffice.)

Bug: chromium:1056170
Change-Id: Ibf3cfb600eaa7f3e11e76579268b52689883440c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2167433
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#762847}

third_party/blink/renderer/bindings/core/v8/active_script_wrappable.h

@@ -18,11 +18,10 @@ class ScriptWrappable;
 // asynchronous activity, even if they are not referenced in the JavaScript or
 // Blink heap.
 //
-// A ScriptWrappable ordinarily is held alive only if it has some such
-// reference, usually via a wrapper object held by script. However, some
-// objects, such as XMLHttpRequest, have pending activity that may be visible
-// (e.g. firing event listeners or resolving promises), and so should not be
-// collected, even if no references remain.
+// This is useful for ScriptWrappable objects that are not held alive by regular
+// references from the object graph. E.g., XMLHttpRequest may have a pending
+// activity that may be visible (e.g. firing event listeners or resolving
+// promises) and should thus not be collected.
 //
 // Such objects should derive from ActiveScriptWrappable<T>, and override
 // ScriptWrappable::HasPendingActivity:
@@ -30,15 +29,10 @@ class ScriptWrappable;
 // which returns true if there may be pending activity which requires the
 // wrappable remain alive.
 //
-// During wrapper tracing, ActiveScriptWrappables which belong to a
-// non-destroyed execution context and have pending activity are treated as
-// roots for the purposes of marking and so will keep themselves and objects
-// they reference alive.
-//
-// Since this pending activity will not keep the wrappable alive after the
-// context is destroyed, it is common for ActiveScriptWrappable objects to also
-// derive from ExecutionContextLifecycleObserver to abort the activity at that
-// time.
+// To avoid leaking objects after the context is destroyed, users of
+// ActiveScriptWrappable<T> also have to provide a GetExecutionContext() method
+// that returns the ExecutionContext or nullptr. A nullptr or already destroyed
+// context results in ignoring HasPendingActivity().
 template <typename T>
 class ActiveScriptWrappable : public ActiveScriptWrappableBase {
  public:

third_party/blink/renderer/platform/bindings/active_script_wrappable_base.cc

@@ -21,14 +21,6 @@ void ActiveScriptWrappableBase::TraceActiveScriptWrappables(
     return;
 
   for (const auto& active_wrappable : *active_script_wrappables) {
-    // Ignore objects that are currently under construction. They are kept alive
-    // via conservative stack scan.
-    HeapObjectHeader const* const header =
-        active_wrappable->GetHeapObjectHeader();
-    if ((header == BlinkGC::kNotFullyConstructedObject) ||
-        header->IsInConstruction())
-      continue;
-
     // A wrapper isn't kept alive after its ExecutionContext becomes detached,
     // even if |HasPendingActivity()| returns |true|. This measure avoids memory
     // leaks and has proven not to be too eager wrt garbage collection of
@@ -51,7 +43,7 @@ void ActiveScriptWrappableBase::TraceActiveScriptWrappables(
   }
 }
 
-ActiveScriptWrappableBase::ActiveScriptWrappableBase() {
+void ActiveScriptWrappableBase::ActiveScriptWrappableBaseConstructed() {
   DCHECK(ThreadState::Current());
   v8::Isolate* isolate = ThreadState::Current()->GetIsolate();
   V8PerIsolateData* isolate_data = V8PerIsolateData::From(isolate);

third_party/blink/renderer/platform/bindings/active_script_wrappable_base.h

@@ -26,8 +26,10 @@ class PLATFORM_EXPORT ActiveScriptWrappableBase : public GarbageCollectedMixin {
 
   virtual ~ActiveScriptWrappableBase() = default;
 
+  void ActiveScriptWrappableBaseConstructed();
+
  protected:
-  ActiveScriptWrappableBase();
+  ActiveScriptWrappableBase() = default;
 
   virtual bool IsContextDestroyed() const = 0;
   virtual bool DispatchHasPendingActivity() const = 0;
@@ -37,6 +39,25 @@ class PLATFORM_EXPORT ActiveScriptWrappableBase : public GarbageCollectedMixin {
   DISALLOW_COPY_AND_ASSIGN(ActiveScriptWrappableBase);
 };
 
+template <typename T>
+struct PostConstructionHookTrait<
+    T,
+    base::void_t<decltype(
+        std::declval<T>().ActiveScriptWrappableBaseConstructed())>> {
+  static void Call(T* object) {
+    static_assert(std::is_base_of<ActiveScriptWrappableBase, T>::value,
+                  "Only ActiveScriptWrappableBase should use the "
+                  "post-construction hook.");
+    // Registering the ActiveScriptWrappableBase after construction means that
+    // the garbage collector does not need to deal with objects that are
+    // currently under construction. This is imnportant as checking whether ASW
+    // should be treated as active involves calling virtual functions which may
+    // not work during construction. The objects in construction are kept alive
+    // via conservative stack scanning.
+    object->ActiveScriptWrappableBaseConstructed();
+  }
+};
+
 }  // namespace blink
 
 #endif  // THIRD_PARTY_BLINK_RENDERER_PLATFORM_BINDINGS_ACTIVE_SCRIPT_WRAPPABLE_BASE_H_

third_party/blink/renderer/platform/heap/heap.h

@@ -559,19 +559,28 @@ struct MakeGarbageCollectedTrait {
   }
 };
 
+template <typename T, typename = void>
+struct PostConstructionHookTrait {
+  static void Call(T*) {}
+};
+
 // Default MakeGarbageCollected: Constructs an instance of T, which is a garbage
 // collected type.
 template <typename T, typename... Args>
 T* MakeGarbageCollected(Args&&... args) {
-  return MakeGarbageCollectedTrait<T>::Call(std::forward<Args>(args)...);
+  T* object = MakeGarbageCollectedTrait<T>::Call(std::forward<Args>(args)...);
+  PostConstructionHookTrait<T>::Call(object);
+  return object;
 }
 
 // Constructs an instance of T, which is a garbage collected type. This special
 // version takes size which enables constructing inline objects.
 template <typename T, typename... Args>
 T* MakeGarbageCollected(AdditionalBytes additional_bytes, Args&&... args) {
-  return MakeGarbageCollectedTrait<T>::Call(additional_bytes,
-                                            std::forward<Args>(args)...);
+  T* object = MakeGarbageCollectedTrait<T>::Call(additional_bytes,
+                                                 std::forward<Args>(args)...);
+  PostConstructionHookTrait<T>::Call(object);
+  return object;
 }
 
 // Assigning class types to their arenas.