Make ProcessThrottler::sendPrepareToSuspendIPC more robust to send failures
<rdar://problem/59921309>
Created attachment 392690 [details] Patch
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.
See https://bugs.webkit.org/show_bug.cgi?id=208701
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.
(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.
(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.
*** This bug has been marked as a duplicate of bug 208701 ***