Bug 203186

Created attachment 381395 [details] Patch

<rdar://problem/56447493>

Comment on attachment 381395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381395&action=review > Source/WebCore/workers/WorkerMessagingProxy.cpp:138 > + m_workerThread->runLoop().postTask(WTFMove(task)); Is it okay that the worker would continue to run until it gets this message? > Source/WebCore/workers/WorkerMessagingProxy.cpp:140 > + m_queuedEarlyTasks.append(makeUnique<ScriptExecutionContext::Task>(WTFMove(task))); Is it really necessary to enqueue this task if there is no worker thread??

Do we have a mechanism to stop the worker when the cached frame is discarded?

Comment on attachment 381395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381395&action=review >> Source/WebCore/workers/WorkerMessagingProxy.cpp:138 >> + m_workerThread->runLoop().postTask(WTFMove(task)); > > Is it okay that the worker would continue to run until it gets this message? I don’t see why not. It is only an optimization, we are stopping the objects as soon as we can. >> Source/WebCore/workers/WorkerMessagingProxy.cpp:140 >> + m_queuedEarlyTasks.append(makeUnique<ScriptExecutionContext::Task>(WTFMove(task))); > > Is it really necessary to enqueue this task if there is no worker thread?? I followed the pattern in postMessage. It seems safer. Otherwise, if the page gets suspended while the worker thread is still starting, it may not know about suspension and run as if not suspended. Similarly as we don’t want to miss messages posted to workers , we do not want them to miss requests to suspend / resume.

Comment on attachment 381395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381395&action=review > Source/WebCore/ChangeLog:8 > + Suspend ActiveDOMObjects in dedicated workers while in the back/forward cache, to avoid wasting CPU. We may be able to suspend workers more aggressively by leveraging Thread::suspend() / Thread::resume().

Comment on attachment 381395 [details] Patch Ok, perhaps we can do that in a follow up.

Created attachment 381444 [details] Patch

Comment on attachment 381444 [details] Patch Geoff has a better idea.

Created attachment 381458 [details] Patch

Comment on attachment 381458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381458&action=review > Source/WebCore/workers/Worker.h:102 > + bool isSuspendedForBackForwardCache { false }; Maybe put this after m_shouldBypassMainWorldContentSecurityPolicy to pack better? > Source/WebCore/workers/WorkerGlobalScopeProxy.h:57 > + // Back / Forward cache support. > + virtual void suspend() = 0; > + virtual void resume() = 0; Why don't we just call them suspendForBackForwardCache & resumeForBackForwardCache instead of adding a comment? > Source/WebCore/workers/WorkerThread.h:141 > + Lock m_suspensionLock; > + Condition m_suspensionCondition; Use BinarySemaphore instead?

Created attachment 381493 [details] Patch

Created attachment 381498 [details] Patch

Comment on attachment 381498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381498&action=review > Source/WebCore/workers/Worker.h:100 > + bool isSuspendedForBackForwardCache { false }; m_isSuspendedForBackForwardCache

Created attachment 381503 [details] Patch

(In reply to Geoffrey Garen from comment #14) > Comment on attachment 381498 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=381498&action=review > > > Source/WebCore/workers/Worker.h:100 > > + bool isSuspendedForBackForwardCache { false }; > > m_isSuspendedForBackForwardCache Oops. Thanks for catching it.

Comment on attachment 381503 [details] Patch Clearing flags on attachment: 381503 Committed r251416: <https://trac.webkit.org/changeset/251416>

All reviewed patches have been landed. Closing bug.