Bug 203046 – [iOS] "Unexpectedly Resumed" process assertion may cause us to get terminated

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

"Unexpectedly Resumed" process assertion may cause us to get terminated.

[Chris Dumez]

<rdar://problem/56179592>

[Chris Dumez]

Created attachment 381091 [details]
Patch

[Geoffrey Garen]

Comment on attachment 381091 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381091&action=review

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:331
> +    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(5 * NSEC_PER_SEC)), dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), releaseAssertion);

I don't think this approach is thread-safe. I guess the use of 'this' is thread-safe because WebProocess is a global singleton. But I don't see anything that guarantees that loads and stores of m_unexpectedlyResumedUIAssertion will be coherent across threads.

I think you need a lock around uses of m_unexpectedlyResumedUIAssertion. Or maybe it would work to make m_unexpectedlyResumedUIAssertion std::atomic<RetainPtr<BKSProcessAssertion>>. (I've never tried that before.)

[Chris Dumez]

Created attachment 381109 [details]
Patch

[Chris Dumez]

Comment on attachment 381109 [details]
Patch

I updated the patch based on Geoff's feedback.

[Geoffrey Garen]

Comment on attachment 381109 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 381109 [details]
Patch

Clearing flags on attachment: 381109

Committed r251304: <https://trac.webkit.org/changeset/251304>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Comment on attachment 381109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381109&action=review

> Source/WebKit/WebProcess/WebProcess.h:540
> +    Lock m_unexpectedlyResumedUIAssertionLock;
> +    RetainPtr<BKSProcessAssertion> m_unexpectedlyResumedUIAssertion;

I think a better name for this condition would be "resumedBeforeUIProcess". It's not really unexpected. It happens all the time. Sometimes it happens because we're playing media in the background, and in that case, only the WebContent process gets a resume message. Sometimes it happens because we're in the foreground, and the WebContent process wins the race to resumption vs the UI process. There's no way for us to know the difference between these two cases, so we always wake up the UI process (and network process by extension) when we wake up first.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:333
> +    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(5 * NSEC_PER_SEC)), dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), releaseAssertion);

I think this code is either a no-op or a bug.

In the case where the UI process responds right away, it sends us a resume message and we clear m_unexpectedlyResumedUIAssertion. So, it's a no-op.

In the case where the UI process takes longer than five seconds to respond, and the RunninBoard invalidation timeout is less than five seconds, RunningBoard sends us an invalidation message. So, it's a no-op.

In the case where the UI process takes longer than five seconds to respond, and the RunninBoard invalidation timeout is more than five seconds, we give up on resuming, even though we have. more time available. So, it's a bug. (Although a minor bug, which probably never happens in practice.)

So, I think the code would be a little clearer if we removed this timeout.

[Geoffrey Garen]

Comment on attachment 381109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381109&action=review

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:313
>      if (!m_processIsSuspended)
>          return;
>  

If we just trust ProcessTaskStateObserver::TaskState, we can avoid the race in checking m_processIsSuspended, and unconditionally take an assertion and signal the UI process to wake up. (This what often happens in practice now anyway.)

Then, if the UI process responds with a message to indicate it received ProcessWasUnexpectedlyUnsuspended, we can clear our assertion in the implementation of that message. In theory, we always must hold our assertion until we receive this response; otherwise, we have not woken up yet.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #10)
> Comment on attachment 381109 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381109&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:313
> >      if (!m_processIsSuspended)
> >          return;
> >  
> 
> If we just trust ProcessTaskStateObserver::TaskState, we can avoid the race
> in checking m_processIsSuspended, and unconditionally take an assertion and
> signal the UI process to wake up. (This what often happens in practice now
> anyway.)
> 
> Then, if the UI process responds with a message to indicate it received
> ProcessWasUnexpectedlyUnsuspended, we can clear our assertion in the
> implementation of that message. In theory, we always must hold our assertion
> until we receive this response; otherwise, we have not woken up yet.

Implementing suggestions via:
https://bugs.webkit.org/show_bug.cgi?id=203254