WebKit Bugzilla
Attachment 359278 Details for
Bug 193359
: Prevent WorkerRunLoop::runInMode from spinning in nested cases
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[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:
Patch for landing
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)
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 193359
:
359046
|
359087
|
359105
|
359108
| 359278