208700 2020-03-06 01:39:11 -0800 Make ProcessThrottler::sendPrepareToSuspendIPC more robust to send failures 2020-03-06 09:33:51 -0800 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED DUPLICATE 208701 InRadar P2 Normal --- 1 youennf youennf achristensen cdumez koivisto oldest_to_newest 1626551 0 youennf 2020-03-06 01:39:11 -0800 Make ProcessThrottler::sendPrepareToSuspendIPC more robust to send failures 1626552 1 youennf 2020-03-06 01:39:31 -0800 <rdar://problem/59921309> 1626553 2 392690 youennf 2020-03-06 01:42:00 -0800 Created attachment 392690 Patch 1626560 3 youennf 2020-03-06 01:55:57 -0800 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. 1626569 4 youennf 2020-03-06 02:02:55 -0800 See https://bugs.webkit.org/show_bug.cgi?id=208701 1626686 5 392690 cdumez 2020-03-06 08:53:26 -0800 Comment on attachment 392690 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. 1626690 6 youennf 2020-03-06 09:02:10 -0800 (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. 1626693 7 cdumez 2020-03-06 09:08:10 -0800 (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. 1626702 8 youennf 2020-03-06 09:33:40 -0800 *** This bug has been marked as a duplicate of bug 208701 *** 392690 2020-03-06 01:42:00 -0800 2020-03-06 09:33:51 -0800 Patch bug-208700-20200306104158.patch text/plain 3687 youennf U3VidmVyc2lvbiBSZXZpc2lvbjogMjU3OTcwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDlmYjM4NjQ4NzI2OTJmMDkw MDBlMWQ4NTA0MDBiZGQwZWE2NGI2Y2QuLmRjZWFlOTE4ZDgwN2RlNjQ2ZTY0NGQzZmUzMDFlN2Ex YmM5MzZkOTMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMjAtMDMtMDYgIFlvdWVubiBG YWJsZXQgIDx5b3Vlbm5AYXBwbGUuY29tPgorCisgICAgICAgIE1ha2UgUHJvY2Vzc1Rocm90dGxl cjo6c2VuZFByZXBhcmVUb1N1c3BlbmRJUEMgbW9yZSByb2J1c3QgdG8gc2VuZCBmYWlsdXJlcwor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjA4NzAwCisg ICAgICAgIDxyZGFyOi8vcHJvYmxlbS81OTkyMTMwOT4KKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGlzIGlzIGRpZmZpY3VsdCB0byB0ZXN0IGFzIHdl IGNoYW5nZSB0aGUgYmVoYXZpb3IgaW4gdGhlIGNhc2UgYSBzZW5kV2l0aEFzeW5jUmVwbHkgZmFp bHMgc3luY2hyb25vdXNseS4KKworICAgICAgICAqIFVJUHJvY2Vzcy9Qcm9jZXNzVGhyb3R0bGVy LmNwcDoKKyAgICAgICAgKFdlYktpdDo6UHJvY2Vzc1Rocm90dGxlcjo6c2VuZFByZXBhcmVUb1N1 c3BlbmRJUEMpOgorICAgICAgICBVcGRhdGUgdGhlIGFzc2VydGlvbiBzdGF0ZSBiZWZvcmUgc2Vu ZGluZyB0aGUgYXN5bmMgSVBDIHNvIHRoYXQgdGhlIGNvbXBsZXRpb24gaGFuZGxlciB3aWwgYWx3 YXlzIGJlIGV4ZWN1dGVkCisgICAgICAgIGFmdGVyIHdlIHRha2UgYSBiYWNrZ3JvdW5kIGFzc2Vy dGlvbi4KKwogMjAyMC0wMy0wNSAgQnJlbnQgRnVsZ2hhbSAgPGJmdWxnaGFtQGFwcGxlLmNvbT4K IAogICAgICAgICBbaU9TXSBSZW1vdmUgcmVwb3J0aW5nIGZvciBzb21lIHdlbGwtdW5kZXJzdG9v ZCBmcmFtZWJ1ZmZlciByb3V0aW5lcwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9VSVByb2Nl c3MvUHJvY2Vzc1Rocm90dGxlci5jcHAgYi9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9Qcm9jZXNz VGhyb3R0bGVyLmNwcAppbmRleCA2OTBiMjQxNTUxOWRjNmEyNjNkZWU5YmVmOGJlNmRiNzEwZTgz N2Y4Li4yOTAxZWI5ZGRhYmM3NmZlNjBkNmNiYzlhN2UxMjFiM2RjMWVkZjE0IDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9Qcm9jZXNzVGhyb3R0bGVyLmNwcAorKysgYi9Tb3Vy Y2UvV2ViS2l0L1VJUHJvY2Vzcy9Qcm9jZXNzVGhyb3R0bGVyLmNwcApAQCAtMTgxLDIwICsxODEs MjIgQEAgdm9pZCBQcm9jZXNzVGhyb3R0bGVyOjpjbGVhclBlbmRpbmdSZXF1ZXN0VG9TdXNwZW5k KCkKIHZvaWQgUHJvY2Vzc1Rocm90dGxlcjo6c2VuZFByZXBhcmVUb1N1c3BlbmRJUEMoSXNTdXNw ZW5zaW9uSW1taW5lbnQgaXNTdXNwZW5zaW9uSW1taW5lbnQpCiB7CiAgICAgUFJPQ0VTU1RIUk9U VExFUl9SRUxFQVNFX0xPRygic2VuZFByZXBhcmVUb1N1c3BlbmRJUEM6IGlzU3VzcGVuc2lvbklt bWluZW50OiAlZCIsIGlzU3VzcGVuc2lvbkltbWluZW50ID09IElzU3VzcGVuc2lvbkltbWluZW50 OjpZZXMpOworCisgICAgc2V0QXNzZXJ0aW9uU3RhdGUoaXNTdXNwZW5zaW9uSW1taW5lbnQgPT0g SXNTdXNwZW5zaW9uSW1taW5lbnQ6OlllcyA/IEFzc2VydGlvblN0YXRlOjpTdXNwZW5kZWQgOiBB c3NlcnRpb25TdGF0ZTo6QmFja2dyb3VuZCk7CisKICAgICBpZiAobV9wZW5kaW5nUmVxdWVzdFRv U3VzcGVuZElEKSB7CiAgICAgICAgIC8vIERvIG5vdCBzZW5kIGEgbmV3IFByZXBhcmVUb1N1c3Bl bmQgSVBDIGZvciBpbW1pbmVudCBzdXNwZW5zaW9uIGlmIHdlJ3ZlIGFscmVhZHkgc2VudCBhIG5v bi1pbW1pbmVudCBQcmVwYXJlVG9TdXNwZW5kIElQQy4KICAgICAgICAgUkVMRUFTRV9BU1NFUlQo aXNTdXNwZW5zaW9uSW1taW5lbnQgPT0gSXNTdXNwZW5zaW9uSW1taW5lbnQ6Olllcyk7CiAgICAg ICAgIFBST0NFU1NUSFJPVFRMRVJfUkVMRUFTRV9MT0coInNlbmRQcmVwYXJlVG9TdXNwZW5kSVBD OiBOb3Qgc2VuZGluZyBQcmVwYXJlVG9TdXNwZW5kKCkgSVBDIGJlY2F1c2UgdGhlcmUgaXMgYWxy ZWFkeSBvbmUgaW4gZmxpZ2h0ICglIiBQUkl1NjQgIikiLCAqbV9wZW5kaW5nUmVxdWVzdFRvU3Vz cGVuZElEKTsKLSAgICB9IGVsc2UgewotICAgICAgICBtX3BlbmRpbmdSZXF1ZXN0VG9TdXNwZW5k SUQgPSBnZW5lcmF0ZVByZXBhcmVUb1N1c3BlbmRSZXF1ZXN0SUQoKTsKLSAgICAgICAgUFJPQ0VT U1RIUk9UVExFUl9SRUxFQVNFX0xPRygic2VuZFByZXBhcmVUb1N1c3BlbmRJUEM6IFNlbmRpbmcg UHJlcGFyZVRvU3VzcGVuZCglIiBQUkl1NjQgIiwgaXNTdXNwZW5zaW9uSW1taW5lbnQ6ICVkKSBJ UEMiLCAqbV9wZW5kaW5nUmVxdWVzdFRvU3VzcGVuZElELCBpc1N1c3BlbnNpb25JbW1pbmVudCA9 PSBJc1N1c3BlbnNpb25JbW1pbmVudDo6WWVzKTsKLSAgICAgICAgbV9wcm9jZXNzLnNlbmRQcmVw YXJlVG9TdXNwZW5kKGlzU3VzcGVuc2lvbkltbWluZW50LCBbdGhpcywgd2Vha1RoaXMgPSBtYWtl V2Vha1B0cigqdGhpcyksIHJlcXVlc3RUb1N1c3BlbmRJRCA9ICptX3BlbmRpbmdSZXF1ZXN0VG9T dXNwZW5kSURdKCkgbXV0YWJsZSB7Ci0gICAgICAgICAgICBpZiAod2Vha1RoaXMgJiYgbV9wZW5k aW5nUmVxdWVzdFRvU3VzcGVuZElEICYmICptX3BlbmRpbmdSZXF1ZXN0VG9TdXNwZW5kSUQgPT0g cmVxdWVzdFRvU3VzcGVuZElEKQotICAgICAgICAgICAgICAgIHByb2Nlc3NSZWFkeVRvU3VzcGVu ZCgpOwotICAgICAgICB9KTsKKyAgICAgICAgcmV0dXJuOwogICAgIH0KIAotICAgIHNldEFzc2Vy dGlvblN0YXRlKGlzU3VzcGVuc2lvbkltbWluZW50ID09IElzU3VzcGVuc2lvbkltbWluZW50OjpZ ZXMgPyBBc3NlcnRpb25TdGF0ZTo6U3VzcGVuZGVkIDogQXNzZXJ0aW9uU3RhdGU6OkJhY2tncm91 bmQpOworICAgIG1fcGVuZGluZ1JlcXVlc3RUb1N1c3BlbmRJRCA9IGdlbmVyYXRlUHJlcGFyZVRv U3VzcGVuZFJlcXVlc3RJRCgpOworICAgIFBST0NFU1NUSFJPVFRMRVJfUkVMRUFTRV9MT0coInNl bmRQcmVwYXJlVG9TdXNwZW5kSVBDOiBTZW5kaW5nIFByZXBhcmVUb1N1c3BlbmQoJSIgUFJJdTY0 ICIsIGlzU3VzcGVuc2lvbkltbWluZW50OiAlZCkgSVBDIiwgKm1fcGVuZGluZ1JlcXVlc3RUb1N1 c3BlbmRJRCwgaXNTdXNwZW5zaW9uSW1taW5lbnQgPT0gSXNTdXNwZW5zaW9uSW1taW5lbnQ6Olll cyk7CisgICAgbV9wcm9jZXNzLnNlbmRQcmVwYXJlVG9TdXNwZW5kKGlzU3VzcGVuc2lvbkltbWlu ZW50LCBbdGhpcywgd2Vha1RoaXMgPSBtYWtlV2Vha1B0cigqdGhpcyksIHJlcXVlc3RUb1N1c3Bl bmRJRCA9ICptX3BlbmRpbmdSZXF1ZXN0VG9TdXNwZW5kSURdKCkgbXV0YWJsZSB7CisgICAgICAg IGlmICh3ZWFrVGhpcyAmJiBtX3BlbmRpbmdSZXF1ZXN0VG9TdXNwZW5kSUQgJiYgKm1fcGVuZGlu Z1JlcXVlc3RUb1N1c3BlbmRJRCA9PSByZXF1ZXN0VG9TdXNwZW5kSUQpCisgICAgICAgICAgICBw cm9jZXNzUmVhZHlUb1N1c3BlbmQoKTsKKyAgICB9KTsKIH0KIAogdm9pZCBQcm9jZXNzVGhyb3R0 bGVyOjp1aUFzc2VydGlvbldpbGxFeHBpcmVJbW1pbmVudGx5KCkK