Changeset 242580 in webkit

Timestamp:

Mar 6, 2019 4:43:24 PM (5 years ago)

Author:

Chris Dumez

Message:

[iOS] ProcessDidResume() IPC should be sent upon resuming when ProcessWillSuspendImminently() IPC was sent
​https://bugs.webkit.org/show_bug.cgi?id=195382
<rdar://problem/48642739>

Reviewed by Geoffrey Garen.

ProcessDidResume() IPC should be sent upon resuming when ProcessWillSuspendImminently() IPC was sent. Previously,
we only send ProcessDidResume() to the WebProcess if PrepareToSuspend() was sent, not when the more urgent
ProcessWillSuspendImminently() IPC was sent.

This mismatch has lead to bugs because the WebProcess does not know it got resumed and failed to unfreeze the
layers it froze upon suspension.

• UIProcess/ProcessAssertion.h:

(WebKit::ProcessAssertionClient::~ProcessAssertionClient):

• UIProcess/ProcessThrottler.cpp:

(WebKit::ProcessThrottler::updateAssertion):
(WebKit::ProcessThrottler::uiAssertionWillExpireImminently):
(WebKit::ProcessThrottler::assertionWillExpireImminently): Deleted.

• UIProcess/ProcessThrottler.h:
• UIProcess/ios/ProcessAssertionIOS.mm:

(-[WKProcessAssertionBackgroundTaskManager _notifyClientsOfImminentSuspension]):
(WebKit::ProcessAssertion::~ProcessAssertion):
(WebKit::ProcessAndUIAssertion::~ProcessAndUIAssertion):
(WebKit::ProcessAndUIAssertion::setClient):

• WebProcess/WebPage/WebPage.cpp:

(WebKit::WebPage::didCompletePageTransition):
Revert change that was landed in r242098 to work around this ProcessThrottler bug.

• WebProcess/WebPage/ios/WebPageIOS.mm:

(WebKit::WebPage::applicationWillEnterForeground):
Revert change that was landed in r242554 to work around this ProcessThrottler bug.

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/ProcessAssertion.h (modified) (1 diff)
• UIProcess/ProcessThrottler.cpp (modified) (3 diffs)
• UIProcess/ProcessThrottler.h (modified) (2 diffs)
• UIProcess/ios/ProcessAssertionIOS.mm (modified) (4 diffs)
• WebProcess/WebPage/WebPage.cpp (modified) (1 diff)
• WebProcess/WebPage/ios/WebPageIOS.mm (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-03-06  Chris Dumez  <cdumez@apple.com>
+
+        [iOS] ProcessDidResume() IPC should be sent upon resuming when ProcessWillSuspendImminently() IPC was sent
+        https://bugs.webkit.org/show_bug.cgi?id=195382
+        <rdar://problem/48642739>
+
+        Reviewed by Geoffrey Garen.
+
+        ProcessDidResume() IPC should be sent upon resuming when ProcessWillSuspendImminently() IPC was sent. Previously,
+        we only send ProcessDidResume() to the WebProcess if PrepareToSuspend() was sent, not when the more urgent
+        ProcessWillSuspendImminently() IPC was sent.
+
+        This mismatch has lead to bugs because the WebProcess does not know it got resumed and failed to unfreeze the
+        layers it froze upon suspension.
+
+        * UIProcess/ProcessAssertion.h:
+        (WebKit::ProcessAssertionClient::~ProcessAssertionClient):
+
+        * UIProcess/ProcessThrottler.cpp:
+        (WebKit::ProcessThrottler::updateAssertion):
+        (WebKit::ProcessThrottler::uiAssertionWillExpireImminently):
+        (WebKit::ProcessThrottler::assertionWillExpireImminently): Deleted.
+
+        * UIProcess/ProcessThrottler.h:
+        * UIProcess/ios/ProcessAssertionIOS.mm:
+        (-[WKProcessAssertionBackgroundTaskManager _notifyClientsOfImminentSuspension]):
+        (WebKit::ProcessAssertion::~ProcessAssertion):
+        (WebKit::ProcessAndUIAssertion::~ProcessAndUIAssertion):
+        (WebKit::ProcessAndUIAssertion::setClient):
+
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::didCompletePageTransition):
+        Revert change that was landed in r242098 to work around this ProcessThrottler bug.
+
+        * WebProcess/WebPage/ios/WebPageIOS.mm:
+        (WebKit::WebPage::applicationWillEnterForeground):
+        Revert change that was landed in r242554 to work around this ProcessThrottler bug.
+
 2019-03-06  Alex Christensen  <achristensen@webkit.org>
 

trunk/Source/WebKit/UIProcess/ProcessAssertion.h

@@ -52 +52 @@
 class ProcessAssertionClient {
 public:
-    virtual ~ProcessAssertionClient() { };
-    virtual void assertionWillExpireImminently() = 0;
+    virtual ~ProcessAssertionClient() { }
+    virtual void uiAssertionWillExpireImminently() = 0;
 };
 

trunk/Source/WebKit/UIProcess/ProcessThrottler.cpp

@@ -69 +69 @@
 void ProcessThrottler::updateAssertion()
 {
+    bool shouldBeRunnable = m_foregroundCounter.value() || m_backgroundCounter.value();
+
     // If the process is currently runnable but will be suspended then first give it a chance to complete what it was doing
     // and clean up - move it to the background and send it a message to notify. Schedule a timeout so it can't stay running
     // in the background for too long.
-    if (m_assertion && m_assertion->state() != AssertionState::Suspended && !m_foregroundCounter.value() && !m_backgroundCounter.value()) {
+    if (m_assertion && m_assertion->state() != AssertionState::Suspended && !shouldBeRunnable) {
         ++m_suspendMessageCount;
         RELEASE_LOG(ProcessSuspension, "%p - ProcessThrottler::updateAssertion() sending PrepareToSuspend IPC", this);
@@ -82 +84 @@
     }
 
-    bool shouldBeRunnable = m_foregroundCounter.value() || m_backgroundCounter.value();
+    if (shouldBeRunnable) {
+        // If we're currently waiting for the Web process to do suspension cleanup, but no longer need to be suspended, tell the Web process to cancel the cleanup.
+        if (m_suspendTimer.isActive())
+            m_process.sendCancelPrepareToSuspend();
 
-    // If we're currently waiting for the Web process to do suspension cleanup, but no longer need to be suspended, tell the Web process to cancel the cleanup.
-    if (m_suspendTimer.isActive() && shouldBeRunnable)
-        m_process.sendCancelPrepareToSuspend();
-    
-    if (m_assertion && m_assertion->state() == AssertionState::Suspended && shouldBeRunnable)
-        m_process.sendProcessDidResume();
+        if ((m_assertion && m_assertion->state() == AssertionState::Suspended) || m_uiAssertionExpired) {
+            m_process.sendProcessDidResume();
+            m_uiAssertionExpired = false;
+        }
+    }
 
     updateAssertionNow();
@@ -126 +130 @@
 }
 
-void ProcessThrottler::assertionWillExpireImminently()
+void ProcessThrottler::uiAssertionWillExpireImminently()
 {
     m_process.sendProcessWillSuspendImminently();
+    m_uiAssertionExpired = true;
 }
 

trunk/Source/WebKit/UIProcess/ProcessThrottler.h

@@ -69 +69 @@
 
     // ProcessAssertionClient
-    void assertionWillExpireImminently() override;
+    void uiAssertionWillExpireImminently() override;
 
     ProcessThrottlerClient& m_process;
@@ -78 +78 @@
     int m_suspendMessageCount { 0 };
     bool m_shouldTakeUIBackgroundAssertion;
+    bool m_uiAssertionExpired { false };
 };
 

trunk/Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm

@@ -99 +99 @@
 
     for (auto* client : copyToVector(_clients))
-        client->assertionWillExpireImminently();
+        client->uiAssertionWillExpireImminently();
 }
 
@@ -209 +209 @@
     m_assertion.get().invalidationHandler = nil;
 
-    if (ProcessAssertionClient* client = this->client())
-        [[WKProcessAssertionBackgroundTaskManager shared] removeClient:*client];
-
     RELEASE_LOG(ProcessSuspension, "%p - ~ProcessAssertion() Releasing process assertion", this);
     [m_assertion invalidate];
@@ -260 +257 @@
     if (m_isHoldingBackgroundAssertion)
         [[WKProcessAssertionBackgroundTaskManager shared] decrementNeedsToRunInBackgroundCount];
+
+    if (auto* client = this->client())
+        [[WKProcessAssertionBackgroundTaskManager shared] removeClient:*client];
 }
 
@@ -271 +271 @@
 {
     [[WKProcessAssertionBackgroundTaskManager shared] addClient:newClient];
-    if (ProcessAssertionClient* oldClient = this->client())
+    if (auto* oldClient = this->client())
         [[WKProcessAssertionBackgroundTaskManager shared] removeClient:*oldClient];
     ProcessAssertion::setClient(newClient);

trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp

@@ -3148 +3148 @@
         RELEASE_LOG_ERROR(ProcessSuspension, "%p - WebPage (PageID=%" PRIu64 ") - LayerTreeFreezeReason::ProcessSuspended was set when removing LayerTreeFreezeReason::PageTransition; current reasons are %d",
             this, m_pageID, m_LayerTreeFreezeReasons.toRaw());
-    }
-
-    // FIXME: In iOS, we sometimes never unset ProcessSuspended. See <rdar://problem/48154508>.
-    unfreezeLayerTree(LayerTreeFreezeReason::ProcessSuspended);
+        ASSERT_NOT_REACHED();
+    }
+
     RELEASE_LOG_IF_ALLOWED("%p - WebPage - Did complete page transition", this);
 

trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm

@@ -3011 +3011 @@
     cancelMarkLayersVolatile();
 
-    // FIXME: In iOS, we sometimes never unset ProcessSuspended. See <rdar://problem/48538837>.
-    unfreezeLayerTree(LayerTreeFreezeReason::ProcessSuspended);
     unfreezeLayerTree(LayerTreeFreezeReason::BackgroundApplication);