Bug 208700 – Make ProcessThrottler::sendPrepareToSuspendIPC more robust to send failures

• Component: WebKit2
• Status: RESOLVED DUPLICATE

[youenn fablet]

Make ProcessThrottler::sendPrepareToSuspendIPC more robust to send failures

[youenn fablet]

<rdar://problem/59921309>

[youenn fablet]

Created attachment 392690 [details]
Patch

[youenn fablet]

An alternative (or a related fix) would be to ensure AuxiliaryProcessProxy::sendMessage is calling the completion handler asynchronously in case of error.
I do not think we loose much efficiency and this might get us safer.

[youenn fablet]

See https://bugs.webkit.org/show_bug.cgi?id=208701

[Chris Dumez]

Comment on attachment 392690 [details]
Patch

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

> Source/WebKit/UIProcess/ProcessThrottler.cpp:185
> +    setAssertionState(isSuspensionImminent == IsSuspensionImminent::Yes ? AssertionState::Suspended : AssertionState::Background);

I am some worries in cases where isSuspensionImminent is true. The previous code already seemed a bit risky but now it seems even riskier. We release the process assertion for the child process *before* even sending the async IPC. The odds of the child process receiving the prepareToSuspend IPC before suspending are not good :/

I am trying to think of a better solution.

[youenn fablet]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 392690 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392690&action=review
> 
> > Source/WebKit/UIProcess/ProcessThrottler.cpp:185
> > +    setAssertionState(isSuspensionImminent == IsSuspensionImminent::Yes ? AssertionState::Suspended : AssertionState::Background);
> 
> I am some worries in cases where isSuspensionImminent is true. The previous
> code already seemed a bit risky but now it seems even riskier. We release
> the process assertion for the child process *before* even sending the async
> IPC. The odds of the child process receiving the prepareToSuspend IPC before
> suspending are not good :/
> 
> I am trying to think of a better solution.

https://bugs.webkit.org/show_bug.cgi?id=208701 should fix the same issue as here by making sure to call the completion handler asynchronously.

As of Imminent==Yes, I am not sure how much this will change things in practice.

[Chris Dumez]

(In reply to youenn fablet from comment #6)
> (In reply to Chris Dumez from comment #5)
> > Comment on attachment 392690 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=392690&action=review
> > 
> > > Source/WebKit/UIProcess/ProcessThrottler.cpp:185
> > > +    setAssertionState(isSuspensionImminent == IsSuspensionImminent::Yes ? AssertionState::Suspended : AssertionState::Background);
> > 
> > I am some worries in cases where isSuspensionImminent is true. The previous
> > code already seemed a bit risky but now it seems even riskier. We release
> > the process assertion for the child process *before* even sending the async
> > IPC. The odds of the child process receiving the prepareToSuspend IPC before
> > suspending are not good :/
> > 
> > I am trying to think of a better solution.
> 
> https://bugs.webkit.org/show_bug.cgi?id=208701 should fix the same issue as
> here by making sure to call the completion handler asynchronously.
> 
> As of Imminent==Yes, I am not sure how much this will change things in
> practice.

I prefer the other patch, seems less risky.

[youenn fablet]

*** This bug has been marked as a duplicate of bug 208701 ***