Attachment 359046 Details for Bug 193359 – Patch

[patch] Patch

bug-193359-20190114094547.patch (text/plain), 7.86 KB, created by youenn fablet on 2019-01-14 09:45:48 PST

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2019-01-14 09:45:48 PST

Size: 7.86 KB

patch

obsolete

>Subversion Revision: 239787
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index f16d4a1f404877b159cd14371d0affdf5f36b185..f3a193b9e78e948f3ee810f3e880370518a3f7e2 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,39 @@
>+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().
>+
>+        * inspector/WorkerScriptDebugServer.cpp:
>+        (WebCore::WorkerScriptDebugServer::runEventLoopWhilePaused):
>+        * workers/WorkerRunLoop.cpp:
>+        (WebCore::ModePredicate::ModePredicate):
>+        (WebCore::WorkerRunLoop::WorkerRunLoop):
>+        (WebCore::RunLoopSetup::RunLoopSetup):
>+        (WebCore::RunLoopSetup::~RunLoopSetup):
>+        (WebCore::WorkerRunLoop::run):
>+        (WebCore::WorkerRunLoop::runInDebuggerMode):
>+        (WebCore::WorkerRunLoop::runInMode):
>+        * workers/WorkerRunLoop.h:
>+        (WebCore::WorkerRunLoop::isBeingDebugged const):
>+
> 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/WorkerRunLoop.cpp b/Source/WebCore/workers/WorkerRunLoop.cpp
>index 6038123ddd026df512f3d6c544babb4978921b54..adc345c0a2f89b9b09e253747245a78b5f2695c6 100644
>--- a/Source/WebCore/workers/WorkerRunLoop.cpp
>+++ b/Source/WebCore/workers/WorkerRunLoop.cpp
>@@ -64,8 +64,8 @@ private:
> 
> class ModePredicate {
> public:
>-    ModePredicate(const String& mode)
>-        : m_mode(mode)
>+    explicit ModePredicate(String&& mode)
>+        : m_mode(WTFMove(mode))
>         , m_defaultMode(mode == WorkerRunLoop::defaultMode())
>     {
>     }
>@@ -87,8 +87,6 @@ private:
> 
> WorkerRunLoop::WorkerRunLoop()
>     : m_sharedTimer(std::make_unique<WorkerSharedTimer>())
>-    , m_nestedCount(0)
>-    , m_uniqueId(0)
> {
> }
> 
>@@ -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 { WorkerRunLoop::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;
>     }
>diff --git a/Source/WebCore/workers/WorkerRunLoop.h b/Source/WebCore/workers/WorkerRunLoop.h
>index e1e01e609122a432f484baf98fbf16b65862e64c..eafff3d75324a0fdb4a1a210b6fc1558be0969af 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(); }
>@@ -89,12 +90,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

Actions: View | Formatted Diff | Diff

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