v8binding: Fixes wrapper-tracing at ResizeObserver.

The current implementation is running ResizeObserver's callback functions even when they're NOT wrapper-traced. This patch makes it sure that the observed Element is performing wrapper-tracing otherwise ResizeObserver does not invoke an untraced callback function. The attached test case causes crash with the current implementation, but not with this patch. Bug: 809784 Change-Id: If2e66d388ab4f4410ca8c472176fbdee5a957f08 Reviewed-on: https://chromium-review.googlesource.com/924104Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/master@{#537714}

v8binding: Fixes wrapper-tracing at ResizeObserver.
The current implementation is running ResizeObserver's callback functions even when they're NOT wrapper-traced. This patch makes it sure that the observed Element is performing wrapper-tracing otherwise ResizeObserver does not invoke an untraced callback function. The attached test case causes crash with the current implementation, but not with this patch. Bug: 809784 Change-Id: If2e66d388ab4f4410ca8c472176fbdee5a957f08 Reviewed-on: https://chromium-review.googlesource.com/924104Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/master@{#537714}
b1a61076 · Yuki Shiino · d164dfff · b1a61076 · b1a61076 · b1a61076
Commit b1a61076 authored Feb 20, 2018 by Yuki Shiino
3 changed files
--- a/third_party/WebKit/LayoutTests/resize-observer/wrapper-tracing.html
+++ b/third_party/WebKit/LayoutTests/resize-observer/wrapper-tracing.html
+<!DOCTYPE html>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+<body>
+<script>
+async_test(t => {
+  // Test that ResizeObserver and its callback function do not cause any crash.
+  // This test is considered as successful as long as the test doesn't crash.
+  // See also https://crbug.com/792604 and https://crbug.com/809784 .
+
+  let target_element = document.createElement('div');
+  document.body.appendChild(target_element);
+
+  // Run a bunch of ResizeObservers to expect high probability to cause a crash.
+  const num_of_observers = 100;
+  resize_observers = new Array(num_of_observers);
+  for (let i = 0; i < num_of_observers; ++i) {
+    resize_observers[i] = new ResizeObserver(entries => {
+      for (let i = 0; i < num_of_observers; ++i) {
+        if (resize_observers[i] === null) continue;
+        // An Element retains the callback functions that are observing the
+        // change on size of the Element. Make the Element not retain the
+        // callback functions.
+        resize_observers[i].unobserve(target_element);
+        // Make the test itself not retain the callback functions.
+        resize_observers[i] = null;
+      }
+
+      // Try to collect the callback functions.
+      gc();
+
+      on_observed();
+    });
+
+    resize_observers[i].observe(target_element);
+  }
+
+  let observation_count = 0;
+  // Wait for the test to finish resize observations for a while.
+  let test_done_timer = null;
+  const test_done_timer_delay = 100;
+  const on_observed = () => {
+    ++observation_count;
+
+    if (test_done_timer !== null)
+      return;
+
+    test_done_timer = setTimeout(() => {
+      // https://wicg.github.io/ResizeObserver/#broadcast-resize-notifications-h
+      // 3.3.4. Broadcast active observations is a bit ambiguous about whether
+      // |unobserve| in the callback functions takes effect immediately or
+      // delayed. Assuming that it must take effect immediately, the expected
+      // number of observations is 1.
+      assert_equals(observation_count, 1);
+
+      target_element.remove();
+      t.done();
+    }, test_done_timer_delay);
+  };
+
+  target_element.style.width = "42px";
+}, "Tries to break wrapper-tracing and to make it crash.");
+</script>
+</body>
--- a/third_party/WebKit/Source/core/resize_observer/ResizeObserver.cpp
+++ b/third_party/WebKit/Source/core/resize_observer/ResizeObserver.cpp
@@ -63,6 +63,10 @@ void ResizeObserver::unobserve(Element* target) {
  auto observation = observer_map->find(this);
  if (observation != observer_map->end()) {
    observations_.erase((*observation).value);
+    auto index = active_observations_.Find((*observation).value);
+    if (index != kNotFound) {
+      active_observations_.EraseAt(index);
+    }
    observer_map->erase(observation);
  }
 }
@@ -103,7 +107,7 @@ void ResizeObserver::DeliverObservations() {
  // We can only clear this flag after all observations have been
  // broadcast.
  element_size_changed_ = skipped_observations_;
-  if (active_observations_.size() == 0)
+  if (active_observations_.IsEmpty())
    return;

  HeapVector<Member<ResizeObserverEntry>> entries;

--- a/third_party/WebKit/Source/core/resize_observer/ResizeObserver.h
+++ b/third_party/WebKit/Source/core/resize_observer/ResizeObserver.h
@@ -65,7 +65,9 @@ class CORE_EXPORT ResizeObserver final : public ScriptWrappable {
  const TraceWrapperMember<V8ResizeObserverCallback> callback_;
  const Member<Delegate> delegate_;

-  // List of elements we are observing
+  // List of Elements we are observing. These Elements make the ResizeObserver
+  // and most-importantly |callback_| alive. If |observations_| is empty, no one
+  // is performing wrapper-tracing and |callback_| might already be gone.
  ObservationList observations_;
  // List of elements that have changes
  HeapVector<Member<ResizeObservation>> active_observations_;