[webkit-changes] [250530] branches/safari-608-branch/Source/WebKit

Mon, 30 Sep 2019 14:09:51 -0700

Title: [250530] branches/safari-608-branch/Source/WebKit
Revision
250530
Author
[email protected]
Date
2019-09-30 14:09:04 -0700 (Mon, 30 Sep 2019)

Log Message

Cherry-pick r250428. rdar://problem/55825357

    [iOS] Lock screen controls can fail to play web content
    https://bugs.webkit.org/show_bug.cgi?id=202279

    Reviewed by Chris Dumez.

    When playback is paused from the lock screen via Now Playing controls, the WebProcess,
    UIProcess, and Network Process will all be suspended. MediaRemote will take an assertion
    and wake up the WebProcess when a remote control command to "play" is sent via the Now
    Playing controls. However, if a synchronous message to the (suspended) Network or UIProcess
    is issued before the notification that the process was unexpectedly unsuspended can be
    issued (which will subsequently unsuspend the UIProcess and Network process), we can get
    into a deadlocked state where the main thread is blocked on the sync message to a suspended
    process.

    To work around this problem, move all the processing from ProcessTaskStateObserver to a
    WorkQueue / background thread. This requires making the ProcessTaskStateObserver thread-safe,
    though its only current client is a Singleton (the WebProcess class), and so the risk of
    thread safety issues is currently minimal. Regardless, access to the Client pointer must be
    guarded by a Lock, and the Client itself must become ref-counted, so that the
    ProcessTaskStateObserver can ref its Client (the WebProcess) during callback processing.

    Unfortunately, sendWithAsyncReply() is not thread safe, nor is ProcessAssertion, so instead
    just use send() and set a 5-second timeout before expiring the assertion, and just use
    BKSProcessStateAssertion directly.

    * Shared/Cocoa/ProcessTaskStateObserver.h:
    (WebKit::ProcessTaskStateObserver::Client::ref):
    (WebKit::ProcessTaskStateObserver::Client::deref):
    (WebKit::ProcessTaskStateObserver::setClient): Deleted.
    (WebKit::ProcessTaskStateObserver::client): Deleted.
    * Shared/Cocoa/ProcessTaskStateObserver.mm:
    (-[WKProcessTaskStateObserverDelegate process:taskStateDidChange:]):
    (WebKit::ProcessTaskStateObserver::create):
    (WebKit::ProcessTaskStateObserver::ProcessTaskStateObserver):
    (WebKit::ProcessTaskStateObserver::~ProcessTaskStateObserver):
    (WebKit::ProcessTaskStateObserver::invalidate):
    (WebKit::ProcessTaskStateObserver::client):
    (WebKit::ProcessTaskStateObserver::setTaskState):
    * WebProcess/WebProcess.cpp:
    (WebKit::m_taskStateObserver):
    * WebProcess/WebProcess.h:
    * UIProcess/Cocoa/WebProcessProxyCocoa.mm:
    (WebKit::WebProcessProxy::processWasUnexpectedlyUnsuspended):
    * UIProcess/WebProcessProxy.h:
    * UIProcess/WebProcessProxy.messages.in:
    * WebProcess/cocoa/WebProcessCocoa.mm:
    (WebKit::WebProcess::processTaskStateDidChange):

    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@250428 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Modified Paths

Diff

Modified: branches/safari-608-branch/Source/WebKit/ChangeLog (250529 => 250530)


--- branches/safari-608-branch/Source/WebKit/ChangeLog	2019-09-30 21:08:59 UTC (rev 250529)
+++ branches/safari-608-branch/Source/WebKit/ChangeLog	2019-09-30 21:09:04 UTC (rev 250530)
@@ -1,5 +1,109 @@
 2019-09-30  Kocsen Chung  <[email protected]>
 
+        Cherry-pick r250428. rdar://problem/55825357
+
+    [iOS] Lock screen controls can fail to play web content
+    https://bugs.webkit.org/show_bug.cgi?id=202279
+    
+    Reviewed by Chris Dumez.
+    
+    When playback is paused from the lock screen via Now Playing controls, the WebProcess,
+    UIProcess, and Network Process will all be suspended. MediaRemote will take an assertion
+    and wake up the WebProcess when a remote control command to "play" is sent via the Now
+    Playing controls. However, if a synchronous message to the (suspended) Network or UIProcess
+    is issued before the notification that the process was unexpectedly unsuspended can be
+    issued (which will subsequently unsuspend the UIProcess and Network process), we can get
+    into a deadlocked state where the main thread is blocked on the sync message to a suspended
+    process.
+    
+    To work around this problem, move all the processing from ProcessTaskStateObserver to a
+    WorkQueue / background thread. This requires making the ProcessTaskStateObserver thread-safe,
+    though its only current client is a Singleton (the WebProcess class), and so the risk of
+    thread safety issues is currently minimal. Regardless, access to the Client pointer must be
+    guarded by a Lock, and the Client itself must become ref-counted, so that the
+    ProcessTaskStateObserver can ref its Client (the WebProcess) during callback processing.
+    
+    Unfortunately, sendWithAsyncReply() is not thread safe, nor is ProcessAssertion, so instead
+    just use send() and set a 5-second timeout before expiring the assertion, and just use
+    BKSProcessStateAssertion directly.
+    
+    * Shared/Cocoa/ProcessTaskStateObserver.h:
+    (WebKit::ProcessTaskStateObserver::Client::ref):
+    (WebKit::ProcessTaskStateObserver::Client::deref):
+    (WebKit::ProcessTaskStateObserver::setClient): Deleted.
+    (WebKit::ProcessTaskStateObserver::client): Deleted.
+    * Shared/Cocoa/ProcessTaskStateObserver.mm:
+    (-[WKProcessTaskStateObserverDelegate process:taskStateDidChange:]):
+    (WebKit::ProcessTaskStateObserver::create):
+    (WebKit::ProcessTaskStateObserver::ProcessTaskStateObserver):
+    (WebKit::ProcessTaskStateObserver::~ProcessTaskStateObserver):
+    (WebKit::ProcessTaskStateObserver::invalidate):
+    (WebKit::ProcessTaskStateObserver::client):
+    (WebKit::ProcessTaskStateObserver::setTaskState):
+    * WebProcess/WebProcess.cpp:
+    (WebKit::m_taskStateObserver):
+    * WebProcess/WebProcess.h:
+    * UIProcess/Cocoa/WebProcessProxyCocoa.mm:
+    (WebKit::WebProcessProxy::processWasUnexpectedlyUnsuspended):
+    * UIProcess/WebProcessProxy.h:
+    * UIProcess/WebProcessProxy.messages.in:
+    * WebProcess/cocoa/WebProcessCocoa.mm:
+    (WebKit::WebProcess::processTaskStateDidChange):
+    
+    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@250428 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+    2019-09-27  Jer Noble  <[email protected]>
+
+            [iOS] Lock screen controls can fail to play web content
+            https://bugs.webkit.org/show_bug.cgi?id=202279
+
+            Reviewed by Chris Dumez.
+
+            When playback is paused from the lock screen via Now Playing controls, the WebProcess,
+            UIProcess, and Network Process will all be suspended. MediaRemote will take an assertion
+            and wake up the WebProcess when a remote control command to "play" is sent via the Now
+            Playing controls. However, if a synchronous message to the (suspended) Network or UIProcess
+            is issued before the notification that the process was unexpectedly unsuspended can be
+            issued (which will subsequently unsuspend the UIProcess and Network process), we can get
+            into a deadlocked state where the main thread is blocked on the sync message to a suspended
+            process.
+
+            To work around this problem, move all the processing from ProcessTaskStateObserver to a
+            WorkQueue / background thread. This requires making the ProcessTaskStateObserver thread-safe,
+            though its only current client is a Singleton (the WebProcess class), and so the risk of
+            thread safety issues is currently minimal. Regardless, access to the Client pointer must be
+            guarded by a Lock, and the Client itself must become ref-counted, so that the
+            ProcessTaskStateObserver can ref its Client (the WebProcess) during callback processing.
+
+            Unfortunately, sendWithAsyncReply() is not thread safe, nor is ProcessAssertion, so instead
+            just use send() and set a 5-second timeout before expiring the assertion, and just use
+            BKSProcessStateAssertion directly.
+
+            * Shared/Cocoa/ProcessTaskStateObserver.h:
+            (WebKit::ProcessTaskStateObserver::Client::ref):
+            (WebKit::ProcessTaskStateObserver::Client::deref):
+            (WebKit::ProcessTaskStateObserver::setClient): Deleted.
+            (WebKit::ProcessTaskStateObserver::client): Deleted.
+            * Shared/Cocoa/ProcessTaskStateObserver.mm:
+            (-[WKProcessTaskStateObserverDelegate process:taskStateDidChange:]):
+            (WebKit::ProcessTaskStateObserver::create):
+            (WebKit::ProcessTaskStateObserver::ProcessTaskStateObserver):
+            (WebKit::ProcessTaskStateObserver::~ProcessTaskStateObserver):
+            (WebKit::ProcessTaskStateObserver::invalidate):
+            (WebKit::ProcessTaskStateObserver::client):
+            (WebKit::ProcessTaskStateObserver::setTaskState):
+            * WebProcess/WebProcess.cpp:
+            (WebKit::m_taskStateObserver):
+            * WebProcess/WebProcess.h:
+            * UIProcess/Cocoa/WebProcessProxyCocoa.mm:
+            (WebKit::WebProcessProxy::processWasUnexpectedlyUnsuspended):
+            * UIProcess/WebProcessProxy.h:
+            * UIProcess/WebProcessProxy.messages.in:
+            * WebProcess/cocoa/WebProcessCocoa.mm:
+            (WebKit::WebProcess::processTaskStateDidChange):
+
+2019-09-30  Kocsen Chung  <[email protected]>
+
         Cherry-pick r250405. rdar://problem/55825353
 
     Add some logging to help diagnose blank or stuck WKWebViews

Modified: branches/safari-608-branch/Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.h (250529 => 250530)


--- branches/safari-608-branch/Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.h	2019-09-30 21:08:59 UTC (rev 250529)
+++ branches/safari-608-branch/Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.h	2019-09-30 21:09:04 UTC (rev 250530)
@@ -27,7 +27,9 @@
 
 #if PLATFORM(IOS_FAMILY)
 
+#include <wtf/Lock.h>
 #include <wtf/RetainPtr.h>
+#include <wtf/ThreadSafeRefCounted.h>
 
 OBJC_CLASS WKProcessTaskStateObserverDelegate;
 OBJC_CLASS BKSProcess;
@@ -34,12 +36,11 @@
 
 namespace WebKit {
 
-class ProcessTaskStateObserver {
+class ProcessTaskStateObserver : public ThreadSafeRefCounted<ProcessTaskStateObserver> {
 public:
     class Client;
 
-    ProcessTaskStateObserver();
-    explicit ProcessTaskStateObserver(Client&);
+    static Ref<ProcessTaskStateObserver> create(Client&);
     ~ProcessTaskStateObserver();
     
     enum TaskState {
@@ -54,15 +55,15 @@
         virtual void processTaskStateDidChange(TaskState) = 0;
     };
 
-    void setClient(Client& client) { m_client = &client; }
-    Client* client() { return m_client; }
-
+    void invalidate();
     TaskState taskState() const { return m_taskState; }
 
 private:
+    explicit ProcessTaskStateObserver(Client&);
     void setTaskState(TaskState);
 
-    Client* m_client { nullptr };
+    Client* m_client;
+    Lock m_clientLock;
     TaskState m_taskState { None };
     RetainPtr<BKSProcess> m_process;
     RetainPtr<WKProcessTaskStateObserverDelegate> m_delegate;

Modified: branches/safari-608-branch/Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.mm (250529 => 250530)


--- branches/safari-608-branch/Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.mm	2019-09-30 21:08:59 UTC (rev 250529)
+++ branches/safari-608-branch/Source/WebKit/Shared/Cocoa/ProcessTaskStateObserver.mm	2019-09-30 21:09:04 UTC (rev 250530)
@@ -47,10 +47,8 @@
 {
     RELEASE_LOG(ProcessSuspension, "%p -[WKProcessTaskStateObserverDelegate process:taskStateDidChange:], process(%p), newState(%d)", self, process, (int)newState);
 
-    dispatch_async(dispatch_get_main_queue(), ^{
-        if (self.taskStateChangedCallback)
-            self.taskStateChangedCallback(newState);
-    });
+    if (auto callback = _taskStateChangedCallback)
+        callback(newState);
 }
 @end
 
@@ -64,25 +62,32 @@
     return static_cast<ProcessTaskStateObserver::TaskState>(state);
 }
 
-ProcessTaskStateObserver::ProcessTaskStateObserver()
-    : m_process([getBKSProcessClass() currentProcess])
+Ref<ProcessTaskStateObserver> ProcessTaskStateObserver::create(Client& client)
+{
+    return adoptRef(*new ProcessTaskStateObserver(client));
+}
+
+ProcessTaskStateObserver::ProcessTaskStateObserver(Client& client)
+    : m_client(&client)
+    , m_process([getBKSProcessClass() currentProcess])
     , m_delegate(adoptNS([[WKProcessTaskStateObserverDelegate alloc] init]))
 {
     RELEASE_LOG(ProcessSuspension, "%p - ProcessTaskStateObserver::ProcessTaskStateObserver(), m_process(%p)", this, m_process.get());
-    m_delegate.get().taskStateChangedCallback = [this] (BKSProcessTaskState state) {
-        setTaskState(toProcessTaskStateObserverTaskState(state));
+    m_delegate.get().taskStateChangedCallback = [protectedThis = makeRefPtr(this)] (BKSProcessTaskState state) {
+        protectedThis->setTaskState(toProcessTaskStateObserverTaskState(state));
     };
     m_process.get().delegate = m_delegate.get();
 }
 
-ProcessTaskStateObserver::ProcessTaskStateObserver(Client& client)
-    : ProcessTaskStateObserver()
+ProcessTaskStateObserver::~ProcessTaskStateObserver()
 {
-    setClient(client);
+    m_delegate.get().taskStateChangedCallback = nil;
 }
 
-ProcessTaskStateObserver::~ProcessTaskStateObserver()
+void ProcessTaskStateObserver::invalidate()
 {
+    LockHolder holder(m_clientLock);
+    m_client = nullptr;
     m_delegate.get().taskStateChangedCallback = nil;
 }
 
@@ -92,6 +97,8 @@
         return;
 
     m_taskState = state;
+
+    LockHolder holder(m_clientLock);
     if (m_client)
         m_client->processTaskStateDidChange(state);
 }

Modified: branches/safari-608-branch/Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm (250529 => 250530)


--- branches/safari-608-branch/Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm	2019-09-30 21:08:59 UTC (rev 250529)
+++ branches/safari-608-branch/Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm	2019-09-30 21:09:04 UTC (rev 250530)
@@ -190,12 +190,11 @@
 #endif
 
 #if PLATFORM(IOS_FAMILY)
-void WebProcessProxy::processWasUnexpectedlyUnsuspended(CompletionHandler<void()>&& completion)
+void WebProcessProxy::processWasUnexpectedlyUnsuspended()
 {
     if (m_throttler.shouldBeRunnable()) {
         // The process becoming unsuspended was not unexpected; it likely was notified of its running state
         // before receiving a procsessDidResume() message from the UIProcess.
-        completion();
         return;
     }
 
@@ -206,7 +205,6 @@
         RELEASE_LOG(ProcessSuspension, "%p - WebProcessProxy::processWasUnexpectedlyUnsuspended() - lambda, background activity timed out", weakThis.get());
     };
     m_unexpectedActivityTimer = std::make_unique<WebCore::DeferrableOneShotTimer>(WTFMove(backgroundActivityTimeoutHandler), unexpectedActivityDuration);
-    completion();
 }
 #endif

Modified: branches/safari-608-branch/Source/WebKit/UIProcess/WebProcessProxy.h (250529 => 250530)


--- branches/safari-608-branch/Source/WebKit/UIProcess/WebProcessProxy.h	2019-09-30 21:08:59 UTC (rev 250529)
+++ branches/safari-608-branch/Source/WebKit/UIProcess/WebProcessProxy.h	2019-09-30 21:09:04 UTC (rev 250530)
@@ -303,7 +303,7 @@
 #endif
 
 #if PLATFORM(IOS_FAMILY)
-    void processWasUnexpectedlyUnsuspended(CompletionHandler<void()>&&);
+    void processWasUnexpectedlyUnsuspended();
 #endif
 
     void webPageMediaStateDidChange(WebPageProxy&);

Modified: branches/safari-608-branch/Source/WebKit/UIProcess/WebProcessProxy.messages.in (250529 => 250530)


--- branches/safari-608-branch/Source/WebKit/UIProcess/WebProcessProxy.messages.in	2019-09-30 21:08:59 UTC (rev 250529)
+++ branches/safari-608-branch/Source/WebKit/UIProcess/WebProcessProxy.messages.in	2019-09-30 21:09:04 UTC (rev 250530)
@@ -81,6 +81,6 @@
 #endif
 
 #if PLATFORM(IOS_FAMILY)
-    ProcessWasUnexpectedlyUnsuspended() -> () Async
+    ProcessWasUnexpectedlyUnsuspended()
 #endif
 }

Modified: branches/safari-608-branch/Source/WebKit/WebProcess/WebProcess.cpp (250529 => 250530)


--- branches/safari-608-branch/Source/WebKit/WebProcess/WebProcess.cpp	2019-09-30 21:08:59 UTC (rev 250529)
+++ branches/safari-608-branch/Source/WebKit/WebProcess/WebProcess.cpp	2019-09-30 21:09:04 UTC (rev 250530)
@@ -192,6 +192,7 @@
     , m_nonVisibleProcessCleanupTimer(*this, &WebProcess::nonVisibleProcessCleanupTimerFired)
 #if PLATFORM(IOS_FAMILY)
     , m_webSQLiteDatabaseTracker([this](bool isHoldingLockedFiles) { parentProcessConnection()->send(Messages::WebProcessProxy::SetIsHoldingLockedFiles(isHoldingLockedFiles), 0); })
+    , m_taskStateObserver(ProcessTaskStateObserver::create(*this))
 #endif
 {
     // Initialize our platform strategies.
@@ -231,6 +232,9 @@
 
 WebProcess::~WebProcess()
 {
+#if PLATFORM(IOS_FAMILY)
+    m_taskStateObserver->invalidate();
+#endif
 }
 
 void WebProcess::initializeProcess(const AuxiliaryProcessInitializationParameters& parameters)

Modified: branches/safari-608-branch/Source/WebKit/WebProcess/WebProcess.h (250529 => 250530)


--- branches/safari-608-branch/Source/WebKit/WebProcess/WebProcess.h	2019-09-30 21:08:59 UTC (rev 250529)
+++ branches/safari-608-branch/Source/WebKit/WebProcess/WebProcess.h	2019-09-30 21:09:04 UTC (rev 250530)
@@ -529,7 +529,7 @@
 
 #if PLATFORM(IOS_FAMILY)
     WebSQLiteDatabaseTracker m_webSQLiteDatabaseTracker;
-    ProcessTaskStateObserver m_taskStateObserver { *this };
+    Ref<ProcessTaskStateObserver> m_taskStateObserver;
 #endif
 
     enum PageMarkingLayersAsVolatileCounterType { };

Modified: branches/safari-608-branch/Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm (250529 => 250530)


--- branches/safari-608-branch/Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm	2019-09-30 21:08:59 UTC (rev 250529)
+++ branches/safari-608-branch/Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm	2019-09-30 21:09:04 UTC (rev 250530)
@@ -27,6 +27,7 @@
 #import "WebProcess.h"
 #import "WebProcessCocoa.h"
 
+#import "AssertionServicesSPI.h"
 #import "LegacyCustomProtocolManager.h"
 #import "LogInitialization.h"
 #import "Logging.h"
@@ -294,6 +295,7 @@
 #if PLATFORM(IOS_FAMILY)
 void WebProcess::processTaskStateDidChange(ProcessTaskStateObserver::TaskState taskState)
 {
+    // NOTE: This will be called from a background thread.
     RELEASE_LOG(ProcessSuspension, "%p - WebProcess::processTaskStateDidChange() - taskState(%d)", this, taskState);
     if (taskState == ProcessTaskStateObserver::None)
         return;
@@ -311,8 +313,10 @@
 
     // We were awakened from suspension unexpectedly. Notify the WebProcessProxy, but take a process assertion on our parent PID
     // to ensure that it too is awakened.
-    auto uiProcessAssertion = std::make_unique<ProcessAssertion>(parentProcessConnection()->remoteProcessID(), "Unexpectedly resumed", AssertionState::Background, AssertionReason::FinishTask);
-    parentProcessConnection()->sendWithAsyncReply(Messages::WebProcessProxy::ProcessWasUnexpectedlyUnsuspended(), [uiProcessAssertion = WTFMove(uiProcessAssertion)] { });
+
+    auto uiProcessAssertion = adoptNS([[BKSProcessAssertion alloc] initWithPID:parentProcessConnection()->remoteProcessID() flags:BKSProcessAssertionPreventTaskSuspend reason:BKSProcessAssertionReasonFinishTask name:@"Unexpectedly resumed" withHandler:nil]);
+    parentProcessConnection()->send(Messages::WebProcessProxy::ProcessWasUnexpectedlyUnsuspended(), 0);
+    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(5 * NSEC_PER_SEC)), dispatch_get_main_queue(), [assertion = WTFMove(uiProcessAssertion)] { [assertion invalidate]; });
 }
 #endif
 

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to