Attachment 368288 Details for Bug 197300 – Patch

[patch] Patch

bug-197300-20190425165104.patch (text/plain), 4.96 KB, created by Chris Dumez on 2019-04-25 16:51:05 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-04-25 16:51:05 PDT

Size: 4.96 KB

patch

obsolete

>Subversion Revision: 244669
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 392a5b91bb7cffa7a5f1952d2080cb02fa243f76..fce9e311e530d80c100693f437853a2e76adb545 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -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.
>diff --git a/Source/WebCore/page/DOMWindow.cpp b/Source/WebCore/page/DOMWindow.cpp
>index 7341ec739749e49520e09bde78ae94f8c0ec3e79..9583062ade9c46b478a8b3abc95d983d1d2599b2 100644
>--- a/Source/WebCore/page/DOMWindow.cpp
>+++ b/Source/WebCore/page/DOMWindow.cpp
>@@ -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;
> }
> 
>diff --git a/Source/WebCore/page/Performance.cpp b/Source/WebCore/page/Performance.cpp
>index a83d0c0a84678f49ffb8a421998babaf4b346f30..57a2a391386386d2c440af2adeff95bced820079 100644
>--- a/Source/WebCore/page/Performance.cpp
>+++ b/Source/WebCore/page/Performance.cpp
>@@ -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) {

Actions: View | Formatted Diff | Diff

Attachments on bug 197300: 368286 | 368288