Although RemoteRenderingBackend::stopListeningForIPC() removes its m_workQueue from the connection WorkQueueMessageReceiver HashMap, m_workQueue can still process accumulated received messages. Removing WorkQueueMessageReceiver will call the destructor ~WorkQueueMessageReceiver but the m_queue of the deleted object can still be running. The reason is the lambda of the WorkQueue::dispatch() in WorkQueueMessageReceiverQueue::enqueueMessage() captures the receiver which is RemoteRenderingBackend as a Ref pointer. Because RemoteRenderingBackend holds the last reference to WorkQueue, m_workQueue can run even after RemoteRenderingBackend::stopListeningForIPC() is called.
<rdar://78430639>
Created attachment 433122 [details] Patch
Created attachment 433124 [details] Patch
This does not fix the "navigating away from the page" case. What's the plan for that?
Comment on attachment 433124 [details] Patch r- pending an answer to the question
Created attachment 433568 [details] Patch Patch for EWS
Created attachment 433620 [details] Patch Patch for EWS
Created attachment 433729 [details] Patch for EWS
Created attachment 433746 [details] Patch for EWS
Created attachment 433771 [details] Patch
Comment on attachment 433771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433771&action=review > Tools/WebKitTestRunner/TestOptions.cpp:124 > + { "RenderingUpdateRegulateEnabled", false }, This should not be needed.
Created attachment 433805 [details] Patch
Comment on attachment 433805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433805&action=review > Source/WebCore/page/RenderingUpdateScheduler.h:70 > + unsigned m_rescheduledRenderingUpdates { 0 }; m_rescheduledRenderingUpdateCount Move it up before the bools to optimize padding. > Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:293 > + m_didFinalizeRenderingUpdateVersion = std::min(didFinalizeRenderingUpdateVersion, m_finalizeRenderingUpdateVersion); Isn't it a bug if didFinalizeRenderingUpdateVersion and m_finalizeRenderingUpdateVersion are out of order? > Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h:158 > + uint64_t m_finalizeRenderingUpdateVersion { 1 }; > + uint64_t m_didFinalizeRenderingUpdateVersion { 1 }; I think these should be a type defined as MonotonicObjectIdentifier<>. Can one be named as "last completed" or something? This is closer to the naming we use for TransactionIDs.
Created attachment 433861 [details] Patch
Created attachment 433985 [details] Patch
Comment on attachment 433805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433805&action=review >> Source/WebCore/page/RenderingUpdateScheduler.h:70 >> + unsigned m_rescheduledRenderingUpdates { 0 }; > > m_rescheduledRenderingUpdateCount > > Move it up before the bools to optimize padding. Done. >> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:293 >> + m_didFinalizeRenderingUpdateVersion = std::min(didFinalizeRenderingUpdateVersion, m_finalizeRenderingUpdateVersion); > > Isn't it a bug if didFinalizeRenderingUpdateVersion and m_finalizeRenderingUpdateVersion are out of order? Yes it is a bug if we are the only who sends didFinalizeRenderingUpdateVersion larger than m_finalizeRenderingUpdateVersion. And this is why I added the debug ASSERT above. But if the argument of the IPC message RemoteRenderingBackendProxy::DidFinalizeRenderingUpdate was altered when sending it from GPUProcess to WebProcess, I thought we can protect ourselves from this underflow if we just use std::min(). unsigned delayedRenderingUpdates() const { return m_finalizeRenderingUpdateVersion - m_didFinalizeRenderingUpdateVersion; } >> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.h:158 >> + uint64_t m_didFinalizeRenderingUpdateVersion { 1 }; > > I think these should be a type defined as MonotonicObjectIdentifier<>. > > Can one be named as "last completed" or something? This is closer to the naming we use for TransactionIDs. Done.
Created attachment 434227 [details] Patch
Comment on attachment 434227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434227&action=review > Source/WebCore/ChangeLog:12 > + certain number of rescheduling. reschedules > Source/WebKit/Shared/MonotonicObjectIdentifier.h:129 > +template<typename T> > +inline unsigned operator-(const MonotonicObjectIdentifier<T>& a, const MonotonicObjectIdentifier<T>& b) > +{ > + return a.toUInt64() - b.toUInt64(); > +} I think this needs to return a signed Int64 value to avoid mistakes. > Source/WebKit/WebProcess/WebPage/WebPage.h:402 > + bool canTriggerRenderingUpdate(unsigned rescheduledRenderingUpdateCount) const; canTriggerRenderingUpdate -> shouldTriggerRenderingUpdate
Comment on attachment 434227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434227&action=review > Source/WebKit/Shared/MonotonicObjectIdentifier.h:35 > +template<typename T> class MonotonicObjectIdentifier { I think this could use a comment to explain its purpose and how it differs from ObjectIdentifier. Isn't an ObjectIdentifier monotonic? It definitely can only increment.
Created attachment 434253 [details] Patch
Comment on attachment 434227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434227&action=review >> Source/WebCore/ChangeLog:12 >> + certain number of rescheduling. > > reschedules Fixed. >> Source/WebKit/Shared/MonotonicObjectIdentifier.h:35 >> +template<typename T> class MonotonicObjectIdentifier { > > I think this could use a comment to explain its purpose and how it differs from ObjectIdentifier. Isn't an ObjectIdentifier monotonic? It definitely can only increment. I added a comment describing the difference between the two and a FIXME suggesting merging them. >> Source/WebKit/Shared/MonotonicObjectIdentifier.h:129 >> +} > > I think this needs to return a signed Int64 value to avoid mistakes. Done. I added a Checked calculation to deal with the overflow. >> Source/WebKit/WebProcess/WebPage/WebPage.h:402 >> + bool canTriggerRenderingUpdate(unsigned rescheduledRenderingUpdateCount) const; > > canTriggerRenderingUpdate -> shouldTriggerRenderingUpdate Fixed.
Tools/Scripts/svn-apply failed to apply attachment 434253 [details] to trunk. Please resolve the conflicts and upload a new patch.
Created attachment 434265 [details] Patch
Committed r280337 (239984@main): <https://commits.webkit.org/239984@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434265 [details].