Performance observers better, not perfect

components/script/dom/performance.rs

@@ -185,22 +185,12 @@ impl Performance {
 
     /// Add a PerformanceObserver to the list of observers with a set of
     /// observed entry types.
-    pub fn add_observer(
+
+    pub fn add_multiple_type_observer(
         &self,
         observer: &DOMPerformanceObserver,
         entry_types: Vec<DOMString>,
-        buffered: bool,
     ) {
-        if buffered {
-            let buffer = self.buffer.borrow();
-            let mut new_entries = entry_types
-                .iter()
-                .flat_map(|e| buffer.get_entries_by_name_and_type(None, Some(e.clone())))
-                .collect::<DOMPerformanceEntryList>();
-            let mut obs_entries = observer.entries();
-            obs_entries.append(&mut new_entries);
-            observer.set_entries(obs_entries);
-        }
         let mut observers = self.observers.borrow_mut();
         match observers.iter().position(|o| *o.observer == *observer) {
             // If the observer is already in the list, we only update the observed
@@ -214,6 +204,51 @@ impl Performance {
         };
     }
 
+    pub fn add_single_type_observer(
+        &self,
+        observer: &DOMPerformanceObserver,
+        entry_type: &DOMString,
+        buffered: bool,
+    ) {
+        if buffered {
+            let buffer = self.buffer.borrow();
+            let mut new_entries =
+                buffer.get_entries_by_name_and_type(None, Some(entry_type.clone()));
+            if new_entries.len() > 0 {
+                let mut obs_entries = observer.entries();
+                obs_entries.append(&mut new_entries);
+                observer.set_entries(obs_entries);
+
+                // W3C spec as of Jan 24 2020 does not say that we necessarily
+                // queue a notification task here, but WPT tests such as
+                // performance-timeline/multiple-buffered-flag-observers.any.js
+                // assume we do, and we get intermittent race condition
+                // test results if we don't.
+                if !self.pending_notification_observers_task.get() {
+                    self.pending_notification_observers_task.set(true);
+                    let task_source = self.global().performance_timeline_task_source();
+                    task_source.queue_notification(&self.global());
+                }
+            }
+        }
+        let mut observers = self.observers.borrow_mut();
+        match observers.iter().position(|o| *o.observer == *observer) {
+            // If the observer is already in the list, we only update
+            // the observed entry types.
+            Some(p) => {
+                // Append the type if not already present, otherwise do nothing
+                if !observers[p].entry_types.contains(entry_type) {
+                    observers[p].entry_types.push(entry_type.clone())
+                }
+            },
+            // Otherwise, we create and insert the new PerformanceObserver.
+            None => observers.push(PerformanceObserver {
+                observer: DomRoot::from_ref(observer),
+                entry_types: vec![entry_type.clone()],
+            }),
+        };
+    }
+
     /// Remove a PerformanceObserver from the list of observers.
     pub fn remove_observer(&self, observer: &DOMPerformanceObserver) {
         let mut observers = self.observers.borrow_mut();
@@ -287,18 +322,13 @@ impl Performance {
         // Step 7.2.
         // We have to operate over a copy of the performance observers to avoid
         // the risk of an observer's callback modifying the list of registered
-        // observers.
+        // observers. This is a shallow copy, so observers can
+        // disconnect themselves by using the argument of their own callback.
         let observers: Vec<DomRoot<DOMPerformanceObserver>> = self
             .observers
             .borrow()
             .iter()
-            .map(|o| {
+            .map(|o| DomRoot::from_ref(&*o.observer))
-                DOMPerformanceObserver::new(
-                    &self.global(),
-                    o.observer.callback(),
-                    o.observer.entries(),
-                )
-            })
             .collect();
 
         // Step 7.3.

components/script/dom/performanceobserver.rs

@@ -13,11 +13,13 @@ use crate::dom::bindings::error::{Error, Fallible};
 use crate::dom::bindings::reflector::{reflect_dom_object, DomObject, Reflector};
 use crate::dom::bindings::root::DomRoot;
 use crate::dom::bindings::str::DOMString;
+use crate::dom::console::Console;
 use crate::dom::globalscope::GlobalScope;
 use crate::dom::performance::PerformanceEntryList;
 use crate::dom::performanceentry::PerformanceEntry;
 use crate::dom::performanceobserverentrylist::PerformanceObserverEntryList;
 use dom_struct::dom_struct;
+use std::cell::Cell;
 use std::rc::Rc;
 
 /// List of allowed performance entry types.
@@ -31,12 +33,20 @@ const VALID_ENTRY_TYPES: &'static [&'static str] = &[
     "paint", // Paint Timing API
 ];
 
+#[derive(Clone, Copy, JSTraceable, MallocSizeOf, PartialEq)]
+enum ObserverType {
+    Undefined,
+    Single,
+    Multiple,
+}
+
 #[dom_struct]
 pub struct PerformanceObserver {
     reflector_: Reflector,
     #[ignore_malloc_size_of = "can't measure Rc values"]
     callback: Rc<PerformanceObserverCallback>,
     entries: DomRefCell<DOMPerformanceEntryList>,
+    observer_type: Cell<ObserverType>,
 }
 
 impl PerformanceObserver {
@@ -48,6 +58,7 @@ impl PerformanceObserver {
             reflector_: Reflector::new(),
             callback,
             entries,
+            observer_type: Cell::new(ObserverType::Undefined),
         }
     }
 
@@ -77,17 +88,15 @@ impl PerformanceObserver {
     /// Trigger performance observer callback with the list of performance entries
     /// buffered since the last callback call.
     pub fn notify(&self) {
-        let entries = self.entries.borrow();
+        if self.entries.borrow().is_empty() {
-        if entries.is_empty() {
             return;
         }
-        let mut entries = entries.clone();
+        let entry_list = PerformanceEntryList::new(self.entries.borrow_mut().drain(..).collect());
-        let global = self.global();
+        let observer_entry_list = PerformanceObserverEntryList::new(&self.global(), entry_list);
-        let entry_list = PerformanceEntryList::new(entries.drain(..).collect());
+        // using self both as thisArg and as the second formal argument
-        let observer_entry_list = PerformanceObserverEntryList::new(&global, entry_list);
         let _ = self
             .callback
-            .Call__(&observer_entry_list, self, ExceptionHandling::Report);
+            .Call_(self, &observer_entry_list, self, ExceptionHandling::Report);
     }
 
     pub fn callback(&self) -> Rc<PerformanceObserverCallback> {
@@ -106,31 +115,112 @@ impl PerformanceObserver {
 impl PerformanceObserverMethods for PerformanceObserver {
     // https://w3c.github.io/performance-timeline/#dom-performanceobserver-observe()
     fn Observe(&self, options: &PerformanceObserverInit) -> Fallible<()> {
-        // step 1
+        // Step 1 is self
-        // Make sure the client is asking to observe events from allowed entry types.
+
-        let entry_types = options
+        // Step 2 is self.global()
-            .entryTypes
+
-            .iter()
+        // Step 3
-            .filter(|e| VALID_ENTRY_TYPES.contains(&e.as_ref()))
+        if options.entryTypes.is_none() && options.type_.is_none() {
-            .map(|e| e.clone())
+            return Err(Error::Syntax);
-            .collect::<Vec<DOMString>>();
-        // step 2
-        // There must be at least one valid entry type.
-        if entry_types.is_empty() {
-            return Err(Error::Type("entryTypes cannot be empty".to_string()));
         }
 
-        // step 3-4-5
+        // Step 4
-        self.global()
+        if options.entryTypes.is_some() && (options.buffered.is_some() || options.type_.is_some()) {
-            .performance()
+            return Err(Error::Syntax);
-            .add_observer(self, entry_types, options.buffered);
+        }
 
-        Ok(())
+        // If this point is reached, then one of options.entryTypes or options.type_
+        // is_some, but not both.
+
+        // Step 5
+        match self.observer_type.get() {
+            ObserverType::Undefined => {
+                if options.entryTypes.is_some() {
+                    self.observer_type.set(ObserverType::Multiple);
+                } else {
+                    self.observer_type.set(ObserverType::Single);
+                }
+            },
+            ObserverType::Single => {
+                if options.entryTypes.is_some() {
+                    return Err(Error::InvalidModification);
+                }
+            },
+            ObserverType::Multiple => {
+                if options.type_.is_some() {
+                    return Err(Error::InvalidModification);
+                }
+            },
+        }
+
+        // The entryTypes and type paths diverge here
+        if let Some(entry_types) = &options.entryTypes {
+            // Steps 6.1 - 6.2
+            let entry_types = entry_types
+                .iter()
+                .filter(|e| VALID_ENTRY_TYPES.contains(&e.as_ref()))
+                .map(|e| e.clone())
+                .collect::<Vec<DOMString>>();
+
+            // Step 6.3
+            if entry_types.is_empty() {
+                Console::Warn(
+                    &*self.global(),
+                    vec![DOMString::from(
+                        "No valid entry type provided to observe().",
+                    )],
+                );
+                return Ok(());
+            }
+
+            // Steps 6.4-6.5
+            // This never pre-fills buffered entries, and
+            // any existing types are replaced.
+            self.global()
+                .performance()
+                .add_multiple_type_observer(self, entry_types);
+            Ok(())
+        } else if let Some(entry_type) = &options.type_ {
+            // Step 7.2
+            if !VALID_ENTRY_TYPES.contains(&entry_type.as_ref()) {
+                Console::Warn(
+                    &*self.global(),
+                    vec![DOMString::from(
+                        "No valid entry type provided to observe().",
+                    )],
+                );
+                return Ok(());
+            }
+
+            // Steps 7.3-7.5
+            // This may pre-fill buffered entries, and
+            // existing types are appended to.
+            self.global().performance().add_single_type_observer(
+                self,
+                entry_type,
+                options.buffered.unwrap_or(false),
+            );
+            Ok(())
+        } else {
+            // Step 7.1
+            unreachable!()
+        }
     }
 
-    // https://w3c.github.io/performance-timeline/#dom-performanceobserver-disconnect()
+    // https://w3c.github.io/performance-timeline/#dom-performanceobserver-disconnect
     fn Disconnect(&self) {
         self.global().performance().remove_observer(self);
         self.entries.borrow_mut().clear();
     }
+
+    // https://w3c.github.io/performance-timeline/#takerecords-method
+    fn TakeRecords(&self) -> Vec<DomRoot<PerformanceEntry>> {
+        let mut entries = self.entries.borrow_mut();
+        let taken = entries
+            .iter()
+            .map(|entry| DomRoot::from_ref(&**entry))
+            .collect();
+        entries.clear();
+        return taken;
+    }
 }

components/script/dom/webidls/PerformanceObserver.webidl

@@ -7,8 +7,9 @@
  */
 
 dictionary PerformanceObserverInit {
-  required sequence<DOMString> entryTypes;
+  sequence<DOMString> entryTypes;
-  boolean buffered = false;
+  DOMString type;
+  boolean buffered;
 };
 
 callback PerformanceObserverCallback = void (PerformanceObserverEntryList entries, PerformanceObserver observer);
@@ -17,6 +18,8 @@ callback PerformanceObserverCallback = void (PerformanceObserverEntryList entrie
 interface PerformanceObserver {
   [Throws] constructor(PerformanceObserverCallback callback);
   [Throws]
-  void observe(PerformanceObserverInit options);
+  void observe(optional PerformanceObserverInit options = {});
   void disconnect();
+  PerformanceEntryList takeRecords();
+  // [SameObject] static readonly attribute FrozenArray<DOMString> supportedEntryTypes;
 };

tests/wpt/metadata/performance-timeline/buffered-flag-after-timeout.any.js.ini

@@ -1,9 +0,0 @@
-[buffered-flag-after-timeout.any.html]
-  [PerformanceObserver with buffered flag sees entry after timeout]
-    expected: FAIL
-
-
-[buffered-flag-after-timeout.any.worker.html]
-  [PerformanceObserver with buffered flag sees entry after timeout]
-    expected: FAIL
-

tests/wpt/metadata/performance-timeline/buffered-flag-observer.any.js.ini

@@ -1,9 +0,0 @@
-[buffered-flag-observer.any.html]
-  [PerformanceObserver with buffered flag should see past and future entries.]
-    expected: FAIL
-
-
-[buffered-flag-observer.any.worker.html]
-  [PerformanceObserver with buffered flag should see past and future entries.]
-    expected: FAIL
-

tests/wpt/metadata/performance-timeline/idlharness.any.js.ini

@@ -8,12 +8,6 @@
 
 
 [idlharness.any.worker.html]
-  [PerformanceObserver interface: operation takeRecords()]
-    expected: FAIL
-
-  [PerformanceObserver interface: observer must inherit property "takeRecords()" with the proper type]
-    expected: FAIL
-
   [Test default toJSON operation of PerformanceMark]
     expected: FAIL
 
@@ -29,20 +23,11 @@
   [PerformanceMark interface object length]
     expected: FAIL
 
-  [PerformanceObserver interface: operation observe(PerformanceObserverInit)]
-    expected: FAIL
-
 
 [idlharness.any.html]
   [Untitled]
     expected: FAIL
 
-  [PerformanceObserver interface: operation takeRecords()]
-    expected: FAIL
-
-  [PerformanceObserver interface: observer must inherit property "takeRecords()" with the proper type]
-    expected: FAIL
-
   [Test default toJSON operation of PerformanceMark]
     expected: FAIL
 
@@ -58,9 +43,6 @@
   [PerformanceMark interface object length]
     expected: FAIL
 
-  [PerformanceObserver interface: operation observe(PerformanceObserverInit)]
-    expected: FAIL
-
 
 [idlharness.https.any.serviceworker.html]
   type: testharness

tests/wpt/metadata/performance-timeline/multiple-buffered-flag-observers.any.js.ini

@@ -1,11 +0,0 @@
-[multiple-buffered-flag-observers.any.html]
-  expected: ERROR
-  [Multiple PerformanceObservers with buffered flag see all entries]
-    expected: TIMEOUT
-
-
-[multiple-buffered-flag-observers.any.worker.html]
-  expected: ERROR
-  [Multiple PerformanceObservers with buffered flag see all entries]
-    expected: TIMEOUT
-

tests/wpt/metadata/performance-timeline/observer-buffered-false.any.js.ini

@@ -1,9 +0,0 @@
-[observer-buffered-false.any.html]
-  [PerformanceObserver without buffered flag set to false cannot see past entries.]
-    expected: FAIL
-
-
-[observer-buffered-false.any.worker.html]
-  [PerformanceObserver without buffered flag set to false cannot see past entries.]
-    expected: FAIL
-

tests/wpt/metadata/performance-timeline/po-callback-mutate.any.js.ini

@@ -1,11 +0,0 @@
-[po-callback-mutate.any.worker.html]
-  type: testharness
-  [PerformanceObserver modifications inside callback should update filtering and not clear buffer]
-    expected: FAIL
-
-
-[po-callback-mutate.any.html]
-  type: testharness
-  [PerformanceObserver modifications inside callback should update filtering and not clear buffer]
-    expected: FAIL
-

tests/wpt/metadata/performance-timeline/po-observe-type.any.js.ini

@@ -1,33 +0,0 @@
-[po-observe-type.any.html]
-  [Calling observe() with entryTypes and then type should throw an InvalidModificationError]
-    expected: FAIL
-
-  [Calling observe() with type and then entryTypes should throw an InvalidModificationError]
-    expected: FAIL
-
-  [Calling observe() with type and entryTypes should throw a SyntaxError]
-    expected: FAIL
-
-  [Passing in unknown values to type does throw an exception.]
-    expected: FAIL
-
-  [observe() with different type values stacks.]
-    expected: FAIL
-
-
-[po-observe-type.any.worker.html]
-  [Calling observe() with entryTypes and then type should throw an InvalidModificationError]
-    expected: FAIL
-
-  [Calling observe() with type and then entryTypes should throw an InvalidModificationError]
-    expected: FAIL
-
-  [Calling observe() with type and entryTypes should throw a SyntaxError]
-    expected: FAIL
-
-  [Passing in unknown values to type does throw an exception.]
-    expected: FAIL
-
-  [observe() with different type values stacks.]
-    expected: FAIL
-

tests/wpt/metadata/performance-timeline/po-observe.any.js.ini

@@ -1,41 +0,0 @@
-[po-observe.any.worker.html]
-  type: testharness
-  [Empty sequence entryTypes is a no-op]
-    expected: FAIL
-
-  [Unknown entryTypes are no-op]
-    expected: FAIL
-
-  [Check observer callback parameter and this values]
-    expected: FAIL
-
-  [no 'type' or 'entryTypes' throws a SyntaxError]
-    expected: FAIL
-
-  [Empty sequence entryTypes does not throw an exception.]
-    expected: FAIL
-
-  [Unknown entryTypes do not throw an exception.]
-    expected: FAIL
-
-
-[po-observe.any.html]
-  type: testharness
-  [Empty sequence entryTypes is a no-op]
-    expected: FAIL
-
-  [Unknown entryTypes are no-op]
-    expected: FAIL
-
-  [Check observer callback parameter and this values]
-    expected: FAIL
-
-  [no 'type' or 'entryTypes' throws a SyntaxError]
-    expected: FAIL
-
-  [Empty sequence entryTypes does not throw an exception.]
-    expected: FAIL
-
-  [Unknown entryTypes do not throw an exception.]
-    expected: FAIL
-

tests/wpt/metadata/performance-timeline/po-observe.html.ini

@@ -1,3 +1,5 @@
 [po-observe.html]
   type: testharness
-  expected: CRASH
+  expected: TIMEOUT
+  [PerformanceObserverInit.buffered should retrieve previously buffered entries]
+    expected: TIMEOUT

tests/wpt/metadata/performance-timeline/po-takeRecords.any.js.ini

@@ -1,9 +0,0 @@
-[po-takeRecords.any.worker.html]
-  [Test PerformanceObserver's takeRecords()]
-    expected: FAIL
-
-
-[po-takeRecords.any.html]
-  [Test PerformanceObserver's takeRecords()]
-    expected: FAIL
-

tests/wpt/metadata/resource-timing/buffered-flag.any.js.ini

@@ -1,11 +1,7 @@
 [buffered-flag.any.worker.html]
-  expected: ERROR
   [PerformanceObserver with buffered flag sees previous resource entries.]
-    expected: TIMEOUT
+    expected: FAIL
-
 
 [buffered-flag.any.html]
-  expected: ERROR
   [PerformanceObserver with buffered flag sees previous resource entries.]
-    expected: TIMEOUT
+    expected: FAIL
-