ASSERT(scriptExecutionContext()) in Performance::resourceTimingBufferFullTimerFired()
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Apr 2019 01:28:39 +0000 (01:28 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Apr 2019 01:28:39 +0000 (01:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=197300
<rdar://problem/49965990>

Reviewed by Youenn Fablet.

We crash because the scriptExecutionContext has been destroyed by the time the m_resourceTimingBufferFullTimer
timer fires. However, r241598 already makes sure that we stop the timer when the script execution context
is destroyed. This makes me think that somebody restarts the timer *after* the script execution context has
been destroyed. The thing is that we only start the timer in Performance::addResourceTiming() and there are
only 2 call sites for this method. Both call sites get the Performance object from the Window object, which
they get from the Document object. As a result, I would believe that the Window's document is alive, even
though the Performance object's scriptExecutionContext is not. This could indicate that the Performance
object's scriptExecutionContext gets out of sync with its Window's document. I have found one place where
it could happen in theory (DOMWindow::didSecureTransitionTo()). I have not been able to write a test
confirming my theory though so this is a speculative fix. I have also added a few assertions to help us
track down the issue if my speculative fix turns out to be ineffective.

No new tests, we do not know how to reproduce.

* page/DOMWindow.cpp:
(WebCore::DOMWindow::didSecureTransitionTo):
This is a speculative fix for the crash. When a DOMWindow transitions from one document to
another, reset its data members which store the DOMWindow's document to make sure that they
do not get out of sync.

(WebCore::DOMWindow::crypto const):
(WebCore::DOMWindow::navigator):
(WebCore::DOMWindow::performance const):
Add assertions to make sure that the member's scriptExecutionContext is in sync with
the window's.

* page/Performance.cpp:
(WebCore::Performance::addResourceTiming):
Add assertion to make sure that the scriptExecutionContext() is non-null when calling this
as this may start the m_resourceTimingBufferFullTimer timer. If my speculative fix above
does not work, we should hit this and this should tell us which call site is causing this.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244673 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/page/DOMWindow.cpp
Source/WebCore/page/Performance.cpp

index 392a5b9..fce9e31 100644 (file)
@@ -1,3 +1,43 @@
+2019-04-25  Chris Dumez  <cdumez@apple.com>
+
+        ASSERT(scriptExecutionContext()) in Performance::resourceTimingBufferFullTimerFired()
+        https://bugs.webkit.org/show_bug.cgi?id=197300
+        <rdar://problem/49965990>
+
+        Reviewed by Youenn Fablet.
+
+        We crash because the scriptExecutionContext has been destroyed by the time the m_resourceTimingBufferFullTimer
+        timer fires. However, r241598 already makes sure that we stop the timer when the script execution context
+        is destroyed. This makes me think that somebody restarts the timer *after* the script execution context has
+        been destroyed. The thing is that we only start the timer in Performance::addResourceTiming() and there are
+        only 2 call sites for this method. Both call sites get the Performance object from the Window object, which
+        they get from the Document object. As a result, I would believe that the Window's document is alive, even
+        though the Performance object's scriptExecutionContext is not. This could indicate that the Performance
+        object's scriptExecutionContext gets out of sync with its Window's document. I have found one place where
+        it could happen in theory (DOMWindow::didSecureTransitionTo()). I have not been able to write a test
+        confirming my theory though so this is a speculative fix. I have also added a few assertions to help us
+        track down the issue if my speculative fix turns out to be ineffective.
+
+        No new tests, we do not know how to reproduce.
+
+        * page/DOMWindow.cpp:
+        (WebCore::DOMWindow::didSecureTransitionTo):
+        This is a speculative fix for the crash. When a DOMWindow transitions from one document to
+        another, reset its data members which store the DOMWindow's document to make sure that they
+        do not get out of sync.
+
+        (WebCore::DOMWindow::crypto const):
+        (WebCore::DOMWindow::navigator):
+        (WebCore::DOMWindow::performance const):
+        Add assertions to make sure that the member's scriptExecutionContext is in sync with
+        the window's.
+
+        * page/Performance.cpp:
+        (WebCore::Performance::addResourceTiming):
+        Add assertion to make sure that the scriptExecutionContext() is non-null when calling this
+        as this may start the m_resourceTimingBufferFullTimer timer. If my speculative fix above
+        does not work, we should hit this and this should tell us which call site is causing this.
+
 2019-04-25  Timothy Hatcher  <timothy@apple.com>
 
         Disable ContentChangeObserver on iOSMac.
index 7341ec7..9583062 100644 (file)
@@ -415,6 +415,12 @@ DOMWindow::DOMWindow(Document& document)
 void DOMWindow::didSecureTransitionTo(Document& document)
 {
     observeContext(&document);
+
+    // The Window is being transferred from one document to another so we need to reset data
+    // members that store the window's document (rather than the window itself).
+    m_crypto = nullptr;
+    m_navigator = nullptr;
+    m_performance = nullptr;
 }
 
 DOMWindow::~DOMWindow()
@@ -648,6 +654,7 @@ Crypto& DOMWindow::crypto() const
 {
     if (!m_crypto)
         m_crypto = Crypto::create(document());
+    ASSERT(m_crypto->scriptExecutionContext() == document());
     return *m_crypto;
 }
 
@@ -713,6 +720,7 @@ Navigator& DOMWindow::navigator()
 {
     if (!m_navigator)
         m_navigator = Navigator::create(scriptExecutionContext(), *this);
+    ASSERT(m_navigator->scriptExecutionContext() == document());
 
     return *m_navigator;
 }
@@ -723,6 +731,7 @@ Performance& DOMWindow::performance() const
         MonotonicTime timeOrigin = document() && document()->loader() ? document()->loader()->timing().referenceMonotonicTime() : MonotonicTime::now();
         m_performance = Performance::create(document(), timeOrigin);
     }
+    ASSERT(m_performance->scriptExecutionContext() == document());
     return *m_performance;
 }
 
index a83d0c0..57a2a39 100644 (file)
@@ -181,6 +181,8 @@ void Performance::setResourceTimingBufferSize(unsigned size)
 
 void Performance::addResourceTiming(ResourceTiming&& resourceTiming)
 {
+    ASSERT(scriptExecutionContext());
+
     auto entry = PerformanceResourceTiming::create(m_timeOrigin, WTFMove(resourceTiming));
 
     if (m_waitingForBackupBufferToBeProcessed) {