Attachment 359278 Details for Bug 193359 – Patch for landing

[patch] Patch for landing

bug-193359-20190116102615.patch (text/plain), 15.77 KB, created by youenn fablet on 2019-01-16 10:26:16 PST

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2019-01-16 10:26:16 PST

Size: 15.77 KB

patch

obsolete

>Subversion Revision: 239989
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 47e24b6b5ff676bdf306515f4a033b06c8919021..ab9de5fc4c4559e2e70c6d7d666c3cd4b0296ef5 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,54 @@
>+2019-01-15  Youenn Fablet  <youenn@apple.com>
>+
>+        Prevent WorkerRunLoop::runInMode from spinning in nested cases
>+        https://bugs.webkit.org/show_bug.cgi?id=193359
>+        <rdar://problem/46345353>
>+
>+        Reviewed by Joseph Pecoraro.
>+
>+        Speculative fix for some cases where service worker is spinning and consuming a lot of CPU.
>+        The hypothesis is that:
>+        - Service Worker is checking for its script freshness through WorkerScriptLoader.
>+        This triggers the worker run loop to be nested.
>+        - The run loop timer is active and needs to fire immediately.
>+        The hypothesis is that this happens in some cases like restarting a device after sleep mode.
>+
>+        WorkerRunLoop::runInMode will then compute a 0 timeout value for getting a message.
>+        This will trigger a timeout while waiting for the message queue.
>+        Since the run loop is nested,  the run loop timer will not be able to fire,
>+        and it will keep ask to fire immediately.
>+        runInMode will return timeout as a result and WorkerRunLoop::run will call it immediately.
>+
>+        The fix is to prevent the shared timer to fire only when the run loop is being debugged through the web inspector.
>+        We compute this by checking the run loop mode as debuggerMode().
>+        Did some refactoring by introducing helper routines for running the loop and posting task in debugger mode.
>+
>+        * inspector/WorkerScriptDebugServer.cpp:
>+        (WebCore::WorkerScriptDebugServer::runEventLoopWhilePaused):
>+        * workers/WorkerInspectorProxy.cpp:
>+        (WebCore::WorkerInspectorProxy::resumeWorkerIfPaused):
>+        (WebCore::WorkerInspectorProxy::connectToWorkerInspectorController):
>+        (WebCore::WorkerInspectorProxy::disconnectFromWorkerInspectorController):
>+        (WebCore::WorkerInspectorProxy::sendMessageToWorkerInspectorController):
>+        * workers/WorkerRunLoop.cpp:
>+        (WebCore::ModePredicate::ModePredicate):
>+        (WebCore::WorkerRunLoop::WorkerRunLoop):
>+        (WebCore::debuggerMode):
>+        (WebCore::RunLoopSetup::RunLoopSetup):
>+        (WebCore::RunLoopSetup::~RunLoopSetup):
>+        (WebCore::WorkerRunLoop::run):
>+        (WebCore::WorkerRunLoop::runInDebuggerMode):
>+        (WebCore::WorkerRunLoop::runInMode):
>+        (WebCore::WorkerRunLoop::Task::performTask):
>+        * workers/WorkerRunLoop.h:
>+        (WebCore::WorkerRunLoop::isBeingDebugged const):
>+        * workers/WorkerThread.cpp:
>+        (WebCore::WorkerThread::startRunningDebuggerTasks):
>+        * workers/service/context/ServiceWorkerInspectorProxy.cpp:
>+        (WebCore::ServiceWorkerInspectorProxy::connectToWorker):
>+        (WebCore::ServiceWorkerInspectorProxy::disconnectFromWorker):
>+        (WebCore::ServiceWorkerInspectorProxy::sendMessageToWorker):
>+
> 2019-01-15  Youenn Fablet  <youenn@apple.com>
> 
>         ServiceWorkerContainer is leaking due to a ref cycle
>diff --git a/Source/WebCore/inspector/WorkerScriptDebugServer.cpp b/Source/WebCore/inspector/WorkerScriptDebugServer.cpp
>index 2a846ae57ff8b44533e36f9d923357712ad95f95..7f36874ebcaf98f19910396ac16a20e9236ab8f0 100644
>--- a/Source/WebCore/inspector/WorkerScriptDebugServer.cpp
>+++ b/Source/WebCore/inspector/WorkerScriptDebugServer.cpp
>@@ -74,7 +74,7 @@ void WorkerScriptDebugServer::runEventLoopWhilePaused()
> 
>     MessageQueueWaitResult result;
>     do {
>-        result = m_workerGlobalScope.thread().runLoop().runInMode(&m_workerGlobalScope, WorkerRunLoop::debuggerMode());
>+        result = m_workerGlobalScope.thread().runLoop().runInDebuggerMode(m_workerGlobalScope);
>     } while (result != MessageQueueTerminated && !m_doneProcessingDebuggerEvents);
> }
> 
>diff --git a/Source/WebCore/workers/WorkerInspectorProxy.cpp b/Source/WebCore/workers/WorkerInspectorProxy.cpp
>index 059a3734734c6924bb5001c02ed682a40a36e171..5f7d1734bb60a4bb2ed17aab815af1ea48d46da1 100644
>--- a/Source/WebCore/workers/WorkerInspectorProxy.cpp
>+++ b/Source/WebCore/workers/WorkerInspectorProxy.cpp
>@@ -90,9 +90,9 @@ void WorkerInspectorProxy::workerTerminated()
> 
> void WorkerInspectorProxy::resumeWorkerIfPaused()
> {
>-    m_workerThread->runLoop().postTaskForMode([] (ScriptExecutionContext& context) {
>+    m_workerThread->runLoop().postDebuggerTask([] (ScriptExecutionContext& context) {
>         downcast<WorkerGlobalScope>(context).thread().stopRunningDebuggerTasks();
>-    }, WorkerRunLoop::debuggerMode());
>+    });
> }
> 
> void WorkerInspectorProxy::connectToWorkerInspectorController(PageChannel* channel)
>@@ -103,9 +103,9 @@ void WorkerInspectorProxy::connectToWorkerInspectorController(PageChannel* chann
> 
>     m_pageChannel = channel;
> 
>-    m_workerThread->runLoop().postTaskForMode([] (ScriptExecutionContext& context) {
>+    m_workerThread->runLoop().postDebuggerTask([] (ScriptExecutionContext& context) {
>         downcast<WorkerGlobalScope>(context).inspectorController().connectFrontend();
>-    }, WorkerRunLoop::debuggerMode());
>+    });
> }
> 
> void WorkerInspectorProxy::disconnectFromWorkerInspectorController()
>@@ -116,13 +116,13 @@ void WorkerInspectorProxy::disconnectFromWorkerInspectorController()
> 
>     m_pageChannel = nullptr;
> 
>-    m_workerThread->runLoop().postTaskForMode([] (ScriptExecutionContext& context) {
>+    m_workerThread->runLoop().postDebuggerTask([] (ScriptExecutionContext& context) {
>         downcast<WorkerGlobalScope>(context).inspectorController().disconnectFrontend(DisconnectReason::InspectorDestroyed);
> 
>         // In case the worker is paused running debugger tasks, ensure we break out of
>         // the pause since this will be the last debugger task we send to the worker.
>         downcast<WorkerGlobalScope>(context).thread().stopRunningDebuggerTasks();
>-    }, WorkerRunLoop::debuggerMode());
>+    });
> }
> 
> void WorkerInspectorProxy::sendMessageToWorkerInspectorController(const String& message)
>@@ -131,9 +131,9 @@ void WorkerInspectorProxy::sendMessageToWorkerInspectorController(const String&
>     if (!m_workerThread)
>         return;
> 
>-    m_workerThread->runLoop().postTaskForMode([message = message.isolatedCopy()] (ScriptExecutionContext& context) {
>+    m_workerThread->runLoop().postDebuggerTask([message = message.isolatedCopy()] (ScriptExecutionContext& context) {
>         downcast<WorkerGlobalScope>(context).inspectorController().dispatchMessageFromFrontend(message);
>-    }, WorkerRunLoop::debuggerMode());
>+    });
> }
> 
> void WorkerInspectorProxy::sendMessageFromWorkerToFrontend(const String& message)
>diff --git a/Source/WebCore/workers/WorkerRunLoop.cpp b/Source/WebCore/workers/WorkerRunLoop.cpp
>index 6038123ddd026df512f3d6c544babb4978921b54..8f7d2c6bdb04cc14d63bd42cf2ac135bcd2b1ffa 100644
>--- a/Source/WebCore/workers/WorkerRunLoop.cpp
>+++ b/Source/WebCore/workers/WorkerRunLoop.cpp
>@@ -64,9 +64,9 @@ private:
> 
> class ModePredicate {
> public:
>-    ModePredicate(const String& mode)
>-        : m_mode(mode)
>-        , m_defaultMode(mode == WorkerRunLoop::defaultMode())
>+    explicit ModePredicate(String&& mode)
>+        : m_mode(WTFMove(mode))
>+        , m_defaultMode(m_mode == WorkerRunLoop::defaultMode())
>     {
>     }
> 
>@@ -87,8 +87,6 @@ private:
> 
> WorkerRunLoop::WorkerRunLoop()
>     : m_sharedTimer(std::make_unique<WorkerSharedTimer>())
>-    , m_nestedCount(0)
>-    , m_uniqueId(0)
> {
> }
> 
>@@ -102,7 +100,7 @@ String WorkerRunLoop::defaultMode()
>     return String();
> }
> 
>-String WorkerRunLoop::debuggerMode()
>+static String debuggerMode()
> {
>     return "debugger"_s;
> }
>@@ -110,12 +108,16 @@ String WorkerRunLoop::debuggerMode()
> class RunLoopSetup {
>     WTF_MAKE_NONCOPYABLE(RunLoopSetup);
> public:
>-    RunLoopSetup(WorkerRunLoop& runLoop)
>+    enum class IsForDebugging { No, Yes };
>+    RunLoopSetup(WorkerRunLoop& runLoop, IsForDebugging isForDebugging)
>         : m_runLoop(runLoop)
>+        , m_isForDebugging(isForDebugging)
>     {
>         if (!m_runLoop.m_nestedCount)
>             threadGlobalData().threadTimers().setSharedTimer(m_runLoop.m_sharedTimer.get());
>         m_runLoop.m_nestedCount++;
>+        if (m_isForDebugging == IsForDebugging::Yes)
>+            m_runLoop.m_debugCount++;
>     }
> 
>     ~RunLoopSetup()
>@@ -123,14 +125,17 @@ public:
>         m_runLoop.m_nestedCount--;
>         if (!m_runLoop.m_nestedCount)
>             threadGlobalData().threadTimers().setSharedTimer(nullptr);
>+        if (m_isForDebugging == IsForDebugging::Yes)
>+            m_runLoop.m_debugCount--;
>     }
> private:
>     WorkerRunLoop& m_runLoop;
>+    IsForDebugging m_isForDebugging { IsForDebugging::No };
> };
> 
> void WorkerRunLoop::run(WorkerGlobalScope* context)
> {
>-    RunLoopSetup setup(*this);
>+    RunLoopSetup setup(*this, RunLoopSetup::IsForDebugging::No);
>     ModePredicate modePredicate(defaultMode());
>     MessageQueueWaitResult result;
>     do {
>@@ -139,10 +144,17 @@ void WorkerRunLoop::run(WorkerGlobalScope* context)
>     runCleanupTasks(context);
> }
> 
>+MessageQueueWaitResult WorkerRunLoop::runInDebuggerMode(WorkerGlobalScope& context)
>+{
>+    RunLoopSetup setup(*this, RunLoopSetup::IsForDebugging::Yes);
>+    return runInMode(&context, ModePredicate { debuggerMode() }, WaitForMessage);
>+}
>+
> MessageQueueWaitResult WorkerRunLoop::runInMode(WorkerGlobalScope* context, const String& mode, WaitMode waitMode)
> {
>-    RunLoopSetup setup(*this);
>-    ModePredicate modePredicate(mode);
>+    ASSERT(mode != debuggerMode());
>+    RunLoopSetup setup(*this, RunLoopSetup::IsForDebugging::No);
>+    ModePredicate modePredicate(String { mode });
>     MessageQueueWaitResult result = runInMode(context, modePredicate, waitMode);
>     return result;
> }
>@@ -155,7 +167,7 @@ MessageQueueWaitResult WorkerRunLoop::runInMode(WorkerGlobalScope* context, cons
>     JSC::JSRunLoopTimer::TimerNotificationCallback timerAddedTask = WTF::createSharedTask<JSC::JSRunLoopTimer::TimerNotificationType>([this] {
>         // We don't actually do anything here, we just want to loop around runInMode
>         // to both recalculate our deadline and to potentially run the run loop.
>-        this->postTask([](ScriptExecutionContext&) { }); 
>+        this->postTask([](ScriptExecutionContext&) { });
>     });
> 
> #if USE(GLIB)
>@@ -202,7 +214,7 @@ MessageQueueWaitResult WorkerRunLoop::runInMode(WorkerGlobalScope* context, cons
>         break;
> 
>     case MessageQueueTimeout:
>-        if (!context->isClosing() && !isNested())
>+        if (!context->isClosing() && !isBeingDebugged())
>             m_sharedTimer->fire();
>         break;
>     }
>@@ -251,6 +263,11 @@ void WorkerRunLoop::postTaskForMode(ScriptExecutionContext::Task&& task, const S
>     m_messageQueue.append(std::make_unique<Task>(WTFMove(task), mode));
> }
> 
>+void WorkerRunLoop::postDebuggerTask(ScriptExecutionContext::Task&& task)
>+{
>+    postTaskForMode(WTFMove(task), debuggerMode());
>+}
>+
> void WorkerRunLoop::Task::performTask(WorkerGlobalScope* context)
> {
>     if ((!context->isClosing() && context->script() && !context->script()->isTerminatingExecution()) || m_task.isCleanupTask())
>diff --git a/Source/WebCore/workers/WorkerRunLoop.h b/Source/WebCore/workers/WorkerRunLoop.h
>index e1e01e609122a432f484baf98fbf16b65862e64c..a8c9debd80d809954a7a9d690f77406d25282668 100644
>--- a/Source/WebCore/workers/WorkerRunLoop.h
>+++ b/Source/WebCore/workers/WorkerRunLoop.h
>@@ -53,6 +53,7 @@ namespace WebCore {
> 
>         // Waits for a single task and returns.
>         MessageQueueWaitResult runInMode(WorkerGlobalScope*, const String& mode, WaitMode = WaitForMessage);
>+        MessageQueueWaitResult runInDebuggerMode(WorkerGlobalScope&);
> 
>         void terminate();
>         bool terminated() const { return m_messageQueue.killed(); }
>@@ -60,12 +61,11 @@ namespace WebCore {
>         void postTask(ScriptExecutionContext::Task&&);
>         void postTaskAndTerminate(ScriptExecutionContext::Task&&);
>         void postTaskForMode(ScriptExecutionContext::Task&&, const String& mode);
>+        void postDebuggerTask(ScriptExecutionContext::Task&&);
> 
>         unsigned long createUniqueId() { return ++m_uniqueId; }
> 
>         static String defaultMode();
>-        static String debuggerMode();
>-
>         class Task {
>             WTF_MAKE_NONCOPYABLE(Task); WTF_MAKE_FAST_ALLOCATED;
>         public:
>@@ -89,12 +89,13 @@ namespace WebCore {
>         // This should only be called when the context is closed or loop has been terminated.
>         void runCleanupTasks(WorkerGlobalScope*);
> 
>-        bool isNested() const { return m_nestedCount > 1; }
>+        bool isBeingDebugged() const { return m_debugCount >= 1; }
> 
>         MessageQueue<Task> m_messageQueue;
>         std::unique_ptr<WorkerSharedTimer> m_sharedTimer;
>-        int m_nestedCount;
>-        unsigned long m_uniqueId;
>+        int m_nestedCount { 0 };
>+        int m_debugCount { 0 };
>+        unsigned long m_uniqueId { 0 };
>     };
> 
> } // namespace WebCore
>diff --git a/Source/WebCore/workers/WorkerThread.cpp b/Source/WebCore/workers/WorkerThread.cpp
>index eb5631c3918008fa25658698828886cfc8369e36..22ea488e2d518d97a5d0b1d2dc035ab0de228638 100644
>--- a/Source/WebCore/workers/WorkerThread.cpp
>+++ b/Source/WebCore/workers/WorkerThread.cpp
>@@ -249,7 +249,7 @@ void WorkerThread::startRunningDebuggerTasks()
> 
>     MessageQueueWaitResult result;
>     do {
>-        result = m_runLoop.runInMode(m_workerGlobalScope.get(), WorkerRunLoop::debuggerMode());
>+        result = m_runLoop.runInDebuggerMode(*m_workerGlobalScope);
>     } while (result != MessageQueueTerminated && m_pausedForDebugger);
> }
> 
>diff --git a/Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.cpp b/Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.cpp
>index 5f0f1ee02daf9e21477abd0121ae7d6643229ac4..73f2c5ce916acb6552113bc82c9889c917238f85 100644
>--- a/Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.cpp
>+++ b/Source/WebCore/workers/service/context/ServiceWorkerInspectorProxy.cpp
>@@ -61,9 +61,9 @@ void ServiceWorkerInspectorProxy::connectToWorker(FrontendChannel& channel)
> {
>     m_channel = &channel;
> 
>-    m_serviceWorkerThreadProxy.thread().runLoop().postTaskForMode([] (ScriptExecutionContext& context) {
>+    m_serviceWorkerThreadProxy.thread().runLoop().postDebuggerTask([] (ScriptExecutionContext& context) {
>         downcast<WorkerGlobalScope>(context).inspectorController().connectFrontend();
>-    }, WorkerRunLoop::debuggerMode());
>+    });
> }
> 
> void ServiceWorkerInspectorProxy::disconnectFromWorker(FrontendChannel& channel)
>@@ -71,20 +71,20 @@ void ServiceWorkerInspectorProxy::disconnectFromWorker(FrontendChannel& channel)
>     ASSERT_UNUSED(channel, &channel == m_channel);
>     m_channel = nullptr;
> 
>-    m_serviceWorkerThreadProxy.thread().runLoop().postTaskForMode([] (ScriptExecutionContext& context) {
>+    m_serviceWorkerThreadProxy.thread().runLoop().postDebuggerTask([] (ScriptExecutionContext& context) {
>         downcast<WorkerGlobalScope>(context).inspectorController().disconnectFrontend(DisconnectReason::InspectorDestroyed);
> 
>         // In case the worker is paused running debugger tasks, ensure we break out of
>         // the pause since this will be the last debugger task we send to the worker.
>         downcast<WorkerGlobalScope>(context).thread().stopRunningDebuggerTasks();
>-    }, WorkerRunLoop::debuggerMode());
>+    });
> }
> 
> void ServiceWorkerInspectorProxy::sendMessageToWorker(const String& message)
> {
>-    m_serviceWorkerThreadProxy.thread().runLoop().postTaskForMode([message = message.isolatedCopy()] (ScriptExecutionContext& context) {
>+    m_serviceWorkerThreadProxy.thread().runLoop().postDebuggerTask([message = message.isolatedCopy()] (ScriptExecutionContext& context) {
>         downcast<WorkerGlobalScope>(context).inspectorController().dispatchMessageFromFrontend(message);
>-    }, WorkerRunLoop::debuggerMode());
>+    });
> }
> 
> void ServiceWorkerInspectorProxy::sendMessageFromWorkerToFrontend(const String& message)

Actions: View | Formatted Diff | Diff

Attachments on bug 193359: 359046 | 359087 | 359105 | 359108 | 359278