Bug 1403027 - Do not throw from PerformanceObserver.observe when none of the entryTypes are known (log a JS console warning instead); r=bz

MozReview-Commit-ID: Lx2cjWDX8sh --HG-- extra : rebase_source : d7e8b3dfbf395de0c0d7b5b7ce716a37337735f5
2024-11-24 13:21:05 +00:00 · 2017-10-22 22:49:44 -04:00 · 2017-10-22 22:49:44 -04:00 · 6bf7c6882a
commit 6bf7c6882a
parent 4235fc5881
22 changed files with 210 additions and 53 deletions
--- a/dom/locales/en-US/chrome/dom/dom.properties
+++ b/dom/locales/en-US/chrome/dom/dom.properties
@ -341,3 +341,4 @@ ScriptSourceNotAllowed=<script> source URI is not allowed in this document: “%
 InvalidKeyframePropertyValue=Keyframe property value “%1$S” is invalid according to the syntax for “%2$S”.
 # LOCALIZATION NOTE: Do not translate "ReadableStream".
 ReadableStreamReadingFailed=Failed to read data from the ReadableStream: “%S”.
+UnsupportedEntryTypesIgnored=Ignoring unsupported entryTypes: %S.
--- a/dom/performance/PerformanceObserver.cpp
+++ b/dom/performance/PerformanceObserver.cpp
@ -10,6 +10,7 @@
 #include "mozilla/dom/PerformanceBinding.h"
 #include "mozilla/dom/PerformanceEntryBinding.h"
 #include "mozilla/dom/PerformanceObserverBinding.h"
+#include "nsIScriptError.h"
 #include "nsPIDOMWindow.h"
 #include "nsQueryObject.h"
 #include "nsString.h"
@ -144,11 +145,9 @@ static const char16_t *const sValidTypeNames[4] = {
 };

 void
-PerformanceObserver::Observe(const PerformanceObserverInit& aOptions,
-                             ErrorResult& aRv)
+PerformanceObserver::Observe(const PerformanceObserverInit& aOptions)
 {
  if (aOptions.mEntryTypes.IsEmpty()) {
-    aRv.Throw(NS_ERROR_DOM_TYPE_ERR);
    return;
  }

@ -162,8 +161,37 @@ PerformanceObserver::Observe(const PerformanceObserverInit& aOptions,
    }
  }

+  nsAutoString invalidTypesJoined;
+  bool addComma = false;
+  for (const auto& type : aOptions.mEntryTypes) {
+    if (!validEntryTypes.Contains<nsString>(type)) {
+      if (addComma) {
+        invalidTypesJoined.AppendLiteral(", ");
+      }
+      addComma = true;
+      invalidTypesJoined.Append(type);
+    }
+  }
+
+  if (!invalidTypesJoined.IsEmpty()) {
+    if (!NS_IsMainThread()) {
+      nsTArray<nsString> params;
+      params.AppendElement(invalidTypesJoined);
+      WorkerPrivate::ReportErrorToConsole("UnsupportedEntryTypesIgnored",
+                                          params);
+    } else {
+      nsCOMPtr<nsPIDOMWindowInner> ownerWindow =
+        do_QueryInterface(mOwner);
+      nsIDocument* document = ownerWindow->GetExtantDoc();
+      const char16_t* params[] = { invalidTypesJoined.get() };
+      nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
+                                      NS_LITERAL_CSTRING("DOM"), document,
+                                      nsContentUtils::eDOM_PROPERTIES,
+                                      "UnsupportedEntryTypesIgnored", params, 1);
+    }
+  }
+
  if (validEntryTypes.IsEmpty()) {
-    aRv.Throw(NS_ERROR_DOM_TYPE_ERR);
    return;
  }

--- a/dom/performance/PerformanceObserver.h
+++ b/dom/performance/PerformanceObserver.h
@ -54,8 +54,7 @@ public:

  nsISupports* GetParentObject() const { return mOwner; }

-  void Observe(const PerformanceObserverInit& aOptions,
-               mozilla::ErrorResult& aRv);
+  void Observe(const PerformanceObserverInit& aOptions);

  void Disconnect();

--- a/dom/performance/tests/test_performance_observer.html
+++ b/dom/performance/tests/test_performance_observer.html
@ -7,11 +7,84 @@
 <head>
 <meta charset=utf-8>
 <title>Test for performance observer</title>
-<script src="/resources/testharness.js"></script>
-<script src="/resources/testharnessreport.js"></script>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
 </head>
 <body>
 <div id="log"></div>
+<script>
+SimpleTest.requestFlakyTimeout("For testing when observer callbacks should not be called.");
+SimpleTest.waitForExplicitFinish();
+
+let _tests = [];
+
+let test = promise_test = fn => {
+  let cleanups = [];
+  _tests.push(async () => {
+    try {
+      await fn({
+        add_cleanup: f => { cleanups.push(f); },
+        step_timeout: function(f, timeout) {
+          var test_this = this;
+          var args = Array.prototype.slice.call(arguments, 2);
+          return setTimeout(() => {
+            return f.apply(test_this, args);
+          }, timeout);
+        }
+      });
+    } catch(e) {
+      ok(false, `got unexpected exception ${e}`);
+    }
+    try {
+      cleanups.forEach(fn, fn);
+    } catch(e) {
+      ok(false, `got unexpected exception during cleanup ${e}`);
+    }
+    runNextTest();
+  });
+}
+
+function runNextTest() {
+  if (_tests.length == 0) {
+    SimpleTest.finish()
+    return;
+  }
+  _tests.shift()();
+}
+
+function assert_equals(actual, expected, description) {
+  ok(typeof actual == typeof expected,
+     `${description} expected (${typeof expected}) ${expected} but got (${typeof actual}) ${actual}`);
+  ok(Object.is(actual, expected),
+     `${description} expected ${expected} but got ${actual}`);
+}
+
+function assert_array_equals(actual, expected, description) {
+  ok(actual.length === expected.length,
+     `${description} lengths differ, expected ${expected.length} but got ${actual.length}`);
+  for (var i = 0; i < actual.length; i++) {
+    ok(actual.hasOwnProperty(i) === expected.hasOwnProperty(i),
+       `${description} property expected to be ${expected[i]} but got ${actual[i]}`);
+  }
+}
+
+function assert_throws(expected_exc, func, desc) {
+  try {
+    func.call(this);
+  } catch(e) {
+    var actual = e.name || e.type;
+    var expected = expected_exc.name || expected_exc.type;
+    ok(actual == expected,
+       `Expected '${expected}', got '${actual}'.`);
+    return;
+  }
+  ok(false, "Expected exception, but none was thrown");
+}
+
+function assert_unreached(description) {
+  ok(false, `${description} reached unreachable code`);
+}
+</script>
 <script src="test_performance_observer.js"></script>
 <script>
 function makeXHR(aUrl) {
@ -20,6 +93,12 @@ function makeXHR(aUrl) {
  xmlhttp.send();
 }

+let waitForConsole = new Promise(resolve => {
+  SimpleTest.monitorConsole(resolve, [{
+    message: /JavaScript Warning: "Ignoring unsupported entryTypes: invalid."/,
+  }]);
+});
+
 promise_test(t => {
  var promise = new Promise(resolve => {
    performance.clearResourceTimings();
@ -31,7 +110,7 @@ promise_test(t => {

  makeXHR("test-data.json");

-  return promise.then(list => {
+  return promise.then(async list => {
    assert_equals(list.getEntries().length, 1);
    assert_array_equals(list.getEntries(),
                        performance.getEntriesByType("resource"),
@ -50,8 +129,12 @@ promise_test(t => {
    assert_array_equals(list.getEntries({initiatorType: "link"}),
                        [],
                        "getEntries with non-existent initiatorType filter should return an empty array.");
+
+    SimpleTest.endMonitorConsole();
+    await waitForConsole;
  });
 }, "resource-timing test");

+runNextTest();
 </script>
 </body>
--- a/dom/performance/tests/test_performance_observer.js
+++ b/dom/performance/tests/test_performance_observer.js
@ -20,17 +20,15 @@ test(t => {
    observer.observe({ unsupportedAttribute: "unsupported" });
  }, "obsrve() should throw TypeError exception if the option has no 'entryTypes' attribute.");

-  assert_throws({name: "TypeError"}, function() {
-    observer.observe({ entryTypes: [] });
-  }, "obsrve() should throw TypeError exception if 'entryTypes' attribute is an empty sequence.");
+  assert_equals(undefined, observer.observe({ entryTypes: [] }),
+     "observe() should silently ignore empty 'entryTypes' sequence.");

  assert_throws({name: "TypeError"}, function() {
    observer.observe({ entryTypes: null });
  }, "obsrve() should throw TypeError exception if 'entryTypes' attribute is null.");

-  assert_throws({name: "TypeError"}, function() {
-    observer.observe({ entryTypes: ["invalid"]});
-  }, "obsrve() should throw TypeError exception if 'entryTypes' attribute value is invalid.");
+  assert_equals(undefined, observer.observe({ entryTypes: ["invalid"] }),
+     "observe() should silently ignore invalid 'entryTypes' values.");
 }, "Test that PerformanceObserver.observe throws exception");

 function promiseObserve(test, options) {
--- a/dom/performance/tests/test_worker_observer.html
+++ b/dom/performance/tests/test_worker_observer.html
@ -9,10 +9,24 @@
 <title>Test for performance observer in worker</title>
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
 </head>
 <body>
 <div id="log"></div>
 <script>
+SimpleTest.waitForExplicitFinish();
+
+let waitForConsole = new Promise(resolve => {
+  SimpleTest.monitorConsole(resolve, [{
+    message: /JavaScript Warning: "Ignoring unsupported entryTypes: invalid."/,
+  }]);
+});
+
+add_completion_callback(async () => {
+    SimpleTest.endMonitorConsole();
+    await waitForConsole;
+});
+
 fetch_tests_from_worker(new Worker("worker_performance_observer.js"));
 </script>
 </body>
--- a/dom/webidl/PerformanceObserver.webidl
+++ b/dom/webidl/PerformanceObserver.webidl
@ -18,7 +18,6 @@ callback PerformanceObserverCallback = void (PerformanceObserverEntryList entrie
 Constructor(PerformanceObserverCallback callback),
 Exposed=(Window,Worker)]
 interface PerformanceObserver {
-  [Throws]
  void observe(PerformanceObserverInit options);
  void disconnect();
 };
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@ -958,11 +958,13 @@ private:
 class ReportErrorToConsoleRunnable final : public WorkerRunnable
 {
  const char* mMessage;
+  const nsTArray<nsString> mParams;

 public:
  // aWorkerPrivate is the worker thread we're on (or the main thread, if null)
  static void
-  Report(WorkerPrivate* aWorkerPrivate, const char* aMessage)
+  Report(WorkerPrivate* aWorkerPrivate, const char* aMessage,
+         const nsTArray<nsString>& aParams)
  {
    if (aWorkerPrivate) {
      aWorkerPrivate->AssertIsOnWorkerThread();
@ -973,23 +975,30 @@ public:
    // Now fire a runnable to do the same on the parent's thread if we can.
    if (aWorkerPrivate) {
      RefPtr<ReportErrorToConsoleRunnable> runnable =
-        new ReportErrorToConsoleRunnable(aWorkerPrivate, aMessage);
+        new ReportErrorToConsoleRunnable(aWorkerPrivate, aMessage, aParams);
      runnable->Dispatch();
      return;
    }

+    uint16_t paramCount = aParams.Length();
+    const char16_t** params = new const char16_t*[paramCount];
+    for (uint16_t i=0; i<paramCount; ++i) {
+      params[i] = aParams[i].get();
+    }
+
    // Log a warning to the console.
    nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
-                                    NS_LITERAL_CSTRING("DOM"),
-                                    nullptr,
-                                    nsContentUtils::eDOM_PROPERTIES,
-                                    aMessage);
+                                    NS_LITERAL_CSTRING("DOM"), nullptr,
+                                    nsContentUtils::eDOM_PROPERTIES, aMessage,
+                                    paramCount ? params : nullptr, paramCount);
+    delete[] params;
  }

 private:
-  ReportErrorToConsoleRunnable(WorkerPrivate* aWorkerPrivate, const char* aMessage)
+  ReportErrorToConsoleRunnable(WorkerPrivate* aWorkerPrivate, const char* aMessage,
+                               const nsTArray<nsString>& aParams)
  : WorkerRunnable(aWorkerPrivate, ParentThreadUnchangedBusyCount),
-    mMessage(aMessage)
+    mMessage(aMessage), mParams(aParams)
  { }

  virtual void
@ -1006,7 +1015,7 @@ private:
  {
    WorkerPrivate* parent = aWorkerPrivate->GetParent();
    MOZ_ASSERT_IF(!parent, NS_IsMainThread());
-    Report(parent, mMessage);
+    Report(parent, mMessage, mParams);
    return true;
  }
 };
@ -6423,13 +6432,22 @@ WorkerPrivate::ReportError(JSContext* aCx, JS::ConstUTF8CharsZ aToStringResult,
 // static
 void
 WorkerPrivate::ReportErrorToConsole(const char* aMessage)
+{
+  nsTArray<nsString> emptyParams;
+  WorkerPrivate::ReportErrorToConsole(aMessage, emptyParams);
+}
+
+// static
+void
+WorkerPrivate::ReportErrorToConsole(const char* aMessage,
+                                    const nsTArray<nsString>& aParams)
 {
  WorkerPrivate* wp = nullptr;
  if (!NS_IsMainThread()) {
    wp = GetCurrentThreadWorkerPrivate();
  }

-  ReportErrorToConsoleRunnable::Report(wp, aMessage);
+  ReportErrorToConsoleRunnable::Report(wp, aMessage, aParams);
 }

 int32_t
--- a/dom/workers/WorkerPrivate.h
+++ b/dom/workers/WorkerPrivate.h
@ -1229,6 +1229,9 @@ public:
  static void
  ReportErrorToConsole(const char* aMessage);

+  static void
+  ReportErrorToConsole(const char* aMessage, const nsTArray<nsString>& aParams);
+
  int32_t
  SetTimeout(JSContext* aCx, nsIScriptTimeoutHandler* aHandler,
             int32_t aTimeout, bool aIsInterval,
--- a/testing/web-platform/meta/longtask-timing/longtask-attributes.html.ini
+++ b/testing/web-platform/meta/longtask-timing/longtask-attributes.html.ini
@ -1,8 +1,9 @@
 [longtask-attributes.html]
  type: testharness
+  expected: TIMEOUT
  [Performance longtask entries are observable]
-    expected: FAIL
+    expected: TIMEOUT

  [Performance longtask entries are observable.]
-    expected: FAIL
+    expected: TIMEOUT

--- a/testing/web-platform/meta/longtask-timing/longtask-in-childiframe-crossorigin.html.ini
+++ b/testing/web-platform/meta/longtask-timing/longtask-in-childiframe-crossorigin.html.ini
@ -1,8 +1,9 @@
 [longtask-in-childiframe-crossorigin.html]
  type: testharness
+  expected: TIMEOUT
  [Performance longtask entries in child iframe are observable in parent]
-    expected: FAIL
+    expected: TIMEOUT

  [Performance longtask entries in child iframe are observable in parent.]
-    expected: FAIL
+    expected: TIMEOUT

--- a/testing/web-platform/meta/longtask-timing/longtask-in-childiframe.html.ini
+++ b/testing/web-platform/meta/longtask-timing/longtask-in-childiframe.html.ini
@ -1,8 +1,9 @@
 [longtask-in-childiframe.html]
  type: testharness
+  expected: TIMEOUT
  [Performance longtask entries in child iframe are observable in parent]
-    expected: FAIL
+    expected: TIMEOUT

  [Performance longtask entries in child iframe are observable in parent.]
-    expected: FAIL
+    expected: TIMEOUT

--- a/testing/web-platform/meta/longtask-timing/longtask-in-externalscript.html.ini
+++ b/testing/web-platform/meta/longtask-timing/longtask-in-externalscript.html.ini
@ -1,8 +1,9 @@
 [longtask-in-externalscript.html]
  type: testharness
+  expected: TIMEOUT
  [Performance longtask entries are observable]
-    expected: FAIL
+    expected: TIMEOUT

  [Performance longtask entries are observable.]
-    expected: FAIL
+    expected: TIMEOUT

--- a/testing/web-platform/meta/longtask-timing/longtask-in-raf.html.ini
+++ b/testing/web-platform/meta/longtask-timing/longtask-in-raf.html.ini
@ -1,8 +1,9 @@
 [longtask-in-raf.html]
  type: testharness
+  expected: TIMEOUT
  [Performance longtask entries are observable]
-    expected: FAIL
+    expected: TIMEOUT

  [Performance longtask entries are observable.]
-    expected: FAIL
+    expected: TIMEOUT

--- a/testing/web-platform/meta/navigation-timing/nav2_test_attributes_exist.html.ini
+++ b/testing/web-platform/meta/navigation-timing/nav2_test_attributes_exist.html.ini
@ -1,5 +1,6 @@
 [nav2_test_attributes_exist.html]
  type: testharness
+  expected: TIMEOUT
  [Performance navigation timing entries are observable.]
-    expected: FAIL
+    expected: TIMEOUT

--- a/testing/web-platform/meta/navigation-timing/nav2_test_attributes_values.html.ini
+++ b/testing/web-platform/meta/navigation-timing/nav2_test_attributes_values.html.ini
@ -1,5 +1,6 @@
 [nav2_test_attributes_values.html]
  type: testharness
+  expected: TIMEOUT
  [Performance navigation timing instance's value is reasonable.]
-    expected: FAIL
+    expected: TIMEOUT

--- a/testing/web-platform/meta/navigation-timing/nav2_test_instance_accessors.html.ini
+++ b/testing/web-platform/meta/navigation-timing/nav2_test_instance_accessors.html.ini
@ -1,5 +1,6 @@
 [nav2_test_instance_accessors.html]
  type: testharness
+  expected: TIMEOUT
  [Performance navigation timing entries are accessible through three different accessors.]
-    expected: FAIL
+    expected: TIMEOUT

--- a/testing/web-platform/meta/navigation-timing/nav2_test_navigation_type_navigate.html.ini
+++ b/testing/web-platform/meta/navigation-timing/nav2_test_navigation_type_navigate.html.ini
@ -1,5 +1,6 @@
 [nav2_test_navigation_type_navigate.html]
  type: testharness
+  expected: TIMEOUT
  [Navigation type to be navigate.]
-    expected: FAIL
+    expected: TIMEOUT

--- a/testing/web-platform/meta/navigation-timing/nav2_test_redirect_none.html.ini
+++ b/testing/web-platform/meta/navigation-timing/nav2_test_redirect_none.html.ini
@ -1,5 +1,6 @@
 [nav2_test_redirect_none.html]
  type: testharness
+  expected: TIMEOUT
  [Naivation without redirects.]
-    expected: FAIL
+    expected: TIMEOUT

--- a/testing/web-platform/meta/navigation-timing/nav2_test_unique_nav_instances.html.ini
+++ b/testing/web-platform/meta/navigation-timing/nav2_test_unique_nav_instances.html.ini
@ -1,5 +1,6 @@
 [nav2_test_unique_nav_instances.html]
  type: testharness
+  expected: TIMEOUT
  [Each window has a unique nav timing 2 instance.]
-    expected: FAIL
+    expected: TIMEOUT

--- a/testing/web-platform/meta/performance-timeline/po-navigation.html.ini
+++ b/testing/web-platform/meta/performance-timeline/po-navigation.html.ini
@ -1,5 +1,6 @@
 [po-navigation.html]
  type: testharness
+  expected: TIMEOUT
  [navigation entry is observable]
-    expected: FAIL
+    expected: TIMEOUT

--- a/testing/web-platform/tests/performance-timeline/po-observe.any.js
+++ b/testing/web-platform/tests/performance-timeline/po-observe.any.js
@ -14,16 +14,18 @@
    assert_throws(new TypeError(), function () {
      obs.observe({entryTypes: "mark"});
    });
-    assert_throws(new TypeError(), function () {
-      obs.observe({entryTypes: []});
-    });
-    assert_throws(new TypeError(), function () {
-      obs.observe({entryTypes: ["this-cannot-match-an-entryType"]});
-    });
-    assert_throws(new TypeError(), function () {
-      obs.observe({entryTypes: ["marks","navigate", "resources"]});
-    });
-  }, "Empty sequence entryTypes throws a TypeError");
+  }, "entryTypes must be a sequence or throw a TypeError");
+
+  test(function () {
+    var obs = new PerformanceObserver(function () { return true; });
+    obs.observe({entryTypes: []});
+  }, "Empty sequence entryTypes is a no-op");
+
+  test(function () {
+    var obs = new PerformanceObserver(function () { return true; });
+    obs.observe({entryTypes: ["this-cannot-match-an-entryType"]});
+    obs.observe({entryTypes: ["marks","navigate", "resources"]});
+  }, "Unknown entryTypes are no-op");

  test(function () {
    var obs = new PerformanceObserver(function () { return true; });