Attachment 359108 Details for Bug 193359 – Patch

[patch] Patch

bug-193359-20190114175148.patch (text/plain), 15.76 KB, created by youenn fablet on 2019-01-14 17:51:49 PST

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2019-01-14 17:51:49 PST

Size: 15.76 KB

patch

obsolete

>Subversion Revision: 239787
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index f16d4a1f404877b159cd14371d0affdf5f36b185..8b9841e8d3709d79bb79ac97235da8724664c8e2 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,53 @@
>+2019-01-14  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 NOBODY (OOPS!).
>+
>+        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 so is using WorkerScriptLoader to fetch it.
>+        This triggers the worker run loop to be nested.
>+        - The run loop timer to be active and need to fire immediatly.
>+        This case might happen if putting a device in sleep mode.
>+
>+        In that case, WorkerRunLoop::runInMode will compute a deadline for getting a message of 0.
>+        This will trigger a timeout while waiting for the message queue.
>+        The shared timer is unable to fire since it is in nested run loop.
>+        runInMode will return timeout and WorkerRunLoop::run will call it immediately.
>+
>+        To prevent that case to happen, 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-09  Youenn Fablet  <youenn@apple.com>
> 
>         Pipe cache quota request from Network Process to UIProcess
>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..a25e07d01c4f0ac15f80933e0d7b68691d1190f1 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,27 +108,32 @@ 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)
>+        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()
>     {
>-        m_runLoop.m_nestedCount--;
>-        if (!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 +142,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 +165,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 +212,7 @@ MessageQueueWaitResult WorkerRunLoop::runInMode(WorkerGlobalScope* context, cons
>         break;
> 
>     case MessageQueueTimeout:
>-        if (!context->isClosing() && !isNested())
>+        if (!context->isClosing() && !isBeingDebugged())
>             m_sharedTimer->fire();
>         break;
>     }
>@@ -251,6 +261,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..10a9d7af24f6c762c37e89252e63cf1499d41862 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; }
> 
>         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