PerformanceObserver option buffered:true causes a double-borrow crash

[pshaughn]

The spec now only allows buffered in combination with the type option and not the entryTypes option, and since we don't support the type option yet, we're failing early in all the WPT tests that use buffered and not getting to the actual behavior being tested. This crash thus needs a modified test to detect.

In the course of #24781 I saw this crash and I wanted to make sure I was uncovering a pre-existing bug rather than creating a new one, so I modified performance-timeline/multiple-buffered-flag-observers.any.js. This modified test is non-spec-compliant since it's combining entryTypes with buffered, but it demonstrates the problem when run on our current master which still allows that combination.

Modified test code

promise_test(() => {
  // The first promise waits for one buffered flag observer to receive 3 entries.
  const promise1 = new Promise(resolve1 => {
    let numObserved1 = 0;
    new PerformanceObserver((entryList, obs) => {
      // This buffered flag observer is constructed after a regular observer detects a mark.
      new PerformanceObserver(list => {
        numObserved1 += list.getEntries().length;
        if (numObserved1 == 3)
          resolve1();
      }).observe({entryTypes: ['mark'], buffered: true});
      obs.disconnect();
    }).observe({entryTypes: ['mark']});
    performance.mark('foo');
  });
  // The second promise waits for another buffered flag observer to receive 3 entries.
  const promise2 = new Promise(resolve2 => {
    step_timeout(() => {
      let numObserved2 = 0;
      // This buffered flag observer is constructed after a delay of 100ms.
      new PerformanceObserver(list => {
        numObserved2 += list.getEntries().length;
        if (numObserved2 == 3)
          resolve2();
      }).observe({entryTypes: ['mark'], buffered: true});
    }, 100);
    performance.mark('bar');
  });
  performance.mark('meow');
  // Pass if and only if both buffered observers received all 3 mark entries.
  return Promise.all([promise1, promise2]);
}, 'Multiple PerformanceObservers with buffered flag see all entries');

It appears that some inappropriate sharing is happening between one PerformanceObserver's entry list and the buffer of entries that another PerformanceObserver is supposed to see, so when one PerformanceObserver tries to clear its list on .disconnect() it's re-borrowing an already borrowed value.

[pshaughn]

I see nothing obvious to cause this, but there are a couple uses of .clone() in the code that I imagine might be shallow-copying cells they are supposed to be deep-copying, and someone more familiar with reference semantics in Servo's cell types might want to look at those first.

[jdm]

Fun fact - on recent revisions of master, you can build with --features refcell_backtrace and you should receive a more useful output for double borrow failures that show both the location of the triggering borrow and the original borrow that is conflicting.

[jdm]

That being said, I suspect the use of let entries = self.entries.borrow(); in PerformanceObserver::notify, which will cause the borrow to be held while we execute an arbitrary JS callback. We should scope that borrow smaller and obtain what we need from it before invoking the callback.

[pshaughn]

Oh, does the following let mut entries not itself drop the earlier let entries?

[pshaughn]

You were correct; test is now timing out safely for some other reason instead of crashing.