From 6d15c0682d3c800886b6a92f8200d7f907a3116f Mon Sep 17 00:00:00 2001 From: Patrick Shaughnessy Date: Thu, 23 Jan 2020 12:48:58 -0500 Subject: [PATCH 1/2] Performance observers better, not perfect --- components/script/dom/performance.rs | 70 ++++++--- components/script/dom/performanceobserver.rs | 140 ++++++++++++++---- .../dom/webidls/PerformanceObserver.webidl | 9 +- .../buffered-flag-after-timeout.any.js.ini | 9 -- .../buffered-flag-observer.any.js.ini | 9 -- .../idlharness.any.js.ini | 18 --- ...ultiple-buffered-flag-observers.any.js.ini | 11 -- .../observer-buffered-false.any.js.ini | 9 -- .../po-callback-mutate.any.js.ini | 11 -- .../po-observe-type.any.js.ini | 33 ----- .../po-observe.any.js.ini | 41 ----- .../performance-timeline/po-observe.html.ini | 4 +- .../po-takeRecords.any.js.ini | 9 -- .../resource-timing/buffered-flag.any.js.ini | 8 +- 14 files changed, 176 insertions(+), 205 deletions(-) delete mode 100644 tests/wpt/metadata/performance-timeline/buffered-flag-after-timeout.any.js.ini delete mode 100644 tests/wpt/metadata/performance-timeline/buffered-flag-observer.any.js.ini delete mode 100644 tests/wpt/metadata/performance-timeline/multiple-buffered-flag-observers.any.js.ini delete mode 100644 tests/wpt/metadata/performance-timeline/observer-buffered-false.any.js.ini delete mode 100644 tests/wpt/metadata/performance-timeline/po-callback-mutate.any.js.ini delete mode 100644 tests/wpt/metadata/performance-timeline/po-observe-type.any.js.ini delete mode 100644 tests/wpt/metadata/performance-timeline/po-observe.any.js.ini delete mode 100644 tests/wpt/metadata/performance-timeline/po-takeRecords.any.js.ini diff --git a/components/script/dom/performance.rs b/components/script/dom/performance.rs index b1f72f405f2..52fbf46860e 100644 --- a/components/script/dom/performance.rs +++ b/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, - 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::(); - 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> = self .observers .borrow() .iter() - .map(|o| { - DOMPerformanceObserver::new( - &self.global(), - o.observer.callback(), - o.observer.entries(), - ) - }) + .map(|o| DomRoot::from_ref(&*o.observer)) .collect(); // Step 7.3. diff --git a/components/script/dom/performanceobserver.rs b/components/script/dom/performanceobserver.rs index 0683a3cb46e..e107c4a08d9 100644 --- a/components/script/dom/performanceobserver.rs +++ b/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, entries: DomRefCell, + observer_type: Cell, } 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 entries.is_empty() { + if self.entries.borrow().is_empty() { return; } - let mut entries = entries.clone(); - let global = self.global(); - let entry_list = PerformanceEntryList::new(entries.drain(..).collect()); - let observer_entry_list = PerformanceObserverEntryList::new(&global, entry_list); + let entry_list = PerformanceEntryList::new(self.entries.borrow_mut().drain(..).collect()); + let observer_entry_list = PerformanceObserverEntryList::new(&self.global(), entry_list); + // using self both as thisArg and as the second formal argument let _ = self .callback - .Call__(&observer_entry_list, self, ExceptionHandling::Report); + .Call_(self, &observer_entry_list, self, ExceptionHandling::Report); } pub fn callback(&self) -> Rc { @@ -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 - // Make sure the client is asking to observe events from allowed entry types. - let entry_types = options - .entryTypes - .iter() - .filter(|e| VALID_ENTRY_TYPES.contains(&e.as_ref())) - .map(|e| e.clone()) - .collect::>(); - // 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 1 is self + + // Step 2 is self.global() + + // Step 3 + if options.entryTypes.is_none() && options.type_.is_none() { + return Err(Error::Syntax); } - // step 3-4-5 - self.global() - .performance() - .add_observer(self, entry_types, options.buffered); + // Step 4 + if options.entryTypes.is_some() && (options.buffered.is_some() || options.type_.is_some()) { + return Err(Error::Syntax); + } - 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::>(); + + // 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> { + let mut entries = self.entries.borrow_mut(); + let taken = entries + .iter() + .map(|entry| DomRoot::from_ref(&**entry)) + .collect(); + entries.clear(); + return taken; + } } diff --git a/components/script/dom/webidls/PerformanceObserver.webidl b/components/script/dom/webidls/PerformanceObserver.webidl index 257ff96c46f..dd3a66b299d 100644 --- a/components/script/dom/webidls/PerformanceObserver.webidl +++ b/components/script/dom/webidls/PerformanceObserver.webidl @@ -7,8 +7,9 @@ */ dictionary PerformanceObserverInit { - required sequence entryTypes; - boolean buffered = false; + sequence entryTypes; + 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 supportedEntryTypes; }; diff --git a/tests/wpt/metadata/performance-timeline/buffered-flag-after-timeout.any.js.ini b/tests/wpt/metadata/performance-timeline/buffered-flag-after-timeout.any.js.ini deleted file mode 100644 index 1069b54261c..00000000000 --- a/tests/wpt/metadata/performance-timeline/buffered-flag-after-timeout.any.js.ini +++ /dev/null @@ -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 - diff --git a/tests/wpt/metadata/performance-timeline/buffered-flag-observer.any.js.ini b/tests/wpt/metadata/performance-timeline/buffered-flag-observer.any.js.ini deleted file mode 100644 index b076184db3e..00000000000 --- a/tests/wpt/metadata/performance-timeline/buffered-flag-observer.any.js.ini +++ /dev/null @@ -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 - diff --git a/tests/wpt/metadata/performance-timeline/idlharness.any.js.ini b/tests/wpt/metadata/performance-timeline/idlharness.any.js.ini index 00d9f1831b0..73206303830 100644 --- a/tests/wpt/metadata/performance-timeline/idlharness.any.js.ini +++ b/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 diff --git a/tests/wpt/metadata/performance-timeline/multiple-buffered-flag-observers.any.js.ini b/tests/wpt/metadata/performance-timeline/multiple-buffered-flag-observers.any.js.ini deleted file mode 100644 index 390c2e4cec2..00000000000 --- a/tests/wpt/metadata/performance-timeline/multiple-buffered-flag-observers.any.js.ini +++ /dev/null @@ -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 - diff --git a/tests/wpt/metadata/performance-timeline/observer-buffered-false.any.js.ini b/tests/wpt/metadata/performance-timeline/observer-buffered-false.any.js.ini deleted file mode 100644 index 5939a89c650..00000000000 --- a/tests/wpt/metadata/performance-timeline/observer-buffered-false.any.js.ini +++ /dev/null @@ -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 - diff --git a/tests/wpt/metadata/performance-timeline/po-callback-mutate.any.js.ini b/tests/wpt/metadata/performance-timeline/po-callback-mutate.any.js.ini deleted file mode 100644 index 4d80c38a5cb..00000000000 --- a/tests/wpt/metadata/performance-timeline/po-callback-mutate.any.js.ini +++ /dev/null @@ -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 - diff --git a/tests/wpt/metadata/performance-timeline/po-observe-type.any.js.ini b/tests/wpt/metadata/performance-timeline/po-observe-type.any.js.ini deleted file mode 100644 index 928849cd870..00000000000 --- a/tests/wpt/metadata/performance-timeline/po-observe-type.any.js.ini +++ /dev/null @@ -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 - diff --git a/tests/wpt/metadata/performance-timeline/po-observe.any.js.ini b/tests/wpt/metadata/performance-timeline/po-observe.any.js.ini deleted file mode 100644 index 9f2213a6720..00000000000 --- a/tests/wpt/metadata/performance-timeline/po-observe.any.js.ini +++ /dev/null @@ -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 - diff --git a/tests/wpt/metadata/performance-timeline/po-observe.html.ini b/tests/wpt/metadata/performance-timeline/po-observe.html.ini index d38a5fd3ac5..e8bdb8ddd77 100644 --- a/tests/wpt/metadata/performance-timeline/po-observe.html.ini +++ b/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 diff --git a/tests/wpt/metadata/performance-timeline/po-takeRecords.any.js.ini b/tests/wpt/metadata/performance-timeline/po-takeRecords.any.js.ini deleted file mode 100644 index 851af386c48..00000000000 --- a/tests/wpt/metadata/performance-timeline/po-takeRecords.any.js.ini +++ /dev/null @@ -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 - diff --git a/tests/wpt/metadata/resource-timing/buffered-flag.any.js.ini b/tests/wpt/metadata/resource-timing/buffered-flag.any.js.ini index dfdea1a3197..9c7751a519d 100644 --- a/tests/wpt/metadata/resource-timing/buffered-flag.any.js.ini +++ b/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 From 5c00acca9828f6d35ecec7b55b2bcb19e1a73fd9 Mon Sep 17 00:00:00 2001 From: Patrick Shaughnessy Date: Wed, 29 Jan 2020 14:59:43 -0500 Subject: [PATCH 2/2] w3c/performance-timeline#159 --- components/script/dom/performance.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/components/script/dom/performance.rs b/components/script/dom/performance.rs index 52fbf46860e..82eec0e4996 100644 --- a/components/script/dom/performance.rs +++ b/components/script/dom/performance.rs @@ -218,17 +218,12 @@ impl Performance { 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()); - } + 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();