223832 2021-03-26 17:42:39 -0700 Safari crashed and lost all tabs, after unlocking sleeping device 2021-03-30 11:56:11 -0700 1 1 1 Unclassified WebKit WebKit Misc. WebKit Nightly Build Unspecified Unspecified REOPENED InRadar P2 Normal --- 1 jiewen_tan jiewen_tan bfulgham ddkilzer ggaren jiewen_tan webkit-bug-importer oldest_to_newest 1744411 0 jiewen_tan 2021-03-26 17:42:39 -0700 Safari crashed and lost all tabs, after unlocking sleeping device. 1744412 1 jiewen_tan 2021-03-26 17:42:49 -0700 rdar://75555287 1744416 2 424426 jiewen_tan 2021-03-26 17:49:31 -0700 Created attachment 424426 Patch 1744508 3 424426 ddkilzer 2021-03-27 08:43:00 -0700 Comment on attachment 424426 Patch r=me 1744515 4 424426 ap 2021-03-27 09:59:12 -0700 Comment on attachment 424426 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424426&action=review > Source/WebKit/ChangeLog:11 > + A speculative fix for this crash. A possible explanation from the crash log suggests that the protectedThis > + could be elided because of compiler optimization given it is not used in the block. To prevent such optimization, > + protectedThis is therefore used explicitly in the block. Is there a reason to stop at speculation? This would be visible in disassembly of the build where this occurred. 1744818 5 jiewen_tan 2021-03-29 12:00:31 -0700 (In reply to Alexey Proskuryakov from comment #4) > Comment on attachment 424426 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424426&action=review > > > Source/WebKit/ChangeLog:11 > > + A speculative fix for this crash. A possible explanation from the crash log suggests that the protectedThis > > + could be elided because of compiler optimization given it is not used in the block. To prevent such optimization, > > + protectedThis is therefore used explicitly in the block. > > Is there a reason to stop at speculation? This would be visible in > disassembly of the build where this occurred. Can we get this info just from the crash reports? 1745035 6 424426 jiewen_tan 2021-03-29 23:31:45 -0700 Comment on attachment 424426 Patch Thanks Dave for r+ this patch. 1745036 7 ews-feeder 2021-03-29 23:35:18 -0700 Committed r275197: <https://commits.webkit.org/r275197> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424426. 1745162 8 424426 ggaren 2021-03-30 10:03:59 -0700 Comment on attachment 424426 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424426&action=review >>> Source/WebKit/ChangeLog:11 >>> + protectedThis is therefore used explicitly in the block. >> >> Is there a reason to stop at speculation? This would be visible in disassembly of the build where this occurred. > > Can we get this info just from the crash reports? You can get this information as follows: (1) otool -tvV /path/to/WebKit (2) type '/' to do a regex search (3) search for dismissViewController (the function names will be mangled; you can use c++filt to unmangle a name and be sure you've got the right one) (4) see if the disassembled code contains calls to ref/deref or, more likely, since ref/deref are usually inlined, see if there's a call to the SOAuthorizationSession operator delete or destructor (which would happen in the unlikely deref path). But also: This theory of the bug is definitely wrong. The compiler has no basis to eliminate a capture when the act of capturing obviously has side effects (by calling ref() in the capture and deref() when out of scope). Also, if the compiler did have such a bug, other WebKit code would crash all the time. 1745163 9 ggaren 2021-03-30 10:04:25 -0700 I think we should revert this patch and continue investigating. If you'd like to do the otool exercise first, to prove the patch had no effect, that's OK too. 1745165 10 ggaren 2021-03-30 10:07:32 -0700 To explain more, the reason I think we should revert is that it is harmful to have "spooky" code that is not verified by any test, and that contradicts our mental model of the computer. Once we enter that spooky universe / weird machine state, it is no longer possible to reason about our code, or understand whether future changes are OK or not. 1745213 11 424426 jiewen_tan 2021-03-30 11:51:26 -0700 Comment on attachment 424426 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424426&action=review >>>> Source/WebKit/ChangeLog:11 >>>> + protectedThis is therefore used explicitly in the block. >>> >>> Is there a reason to stop at speculation? This would be visible in disassembly of the build where this occurred. >> >> Can we get this info just from the crash reports? > > You can get this information as follows: > > (1) otool -tvV /path/to/WebKit > > (2) type '/' to do a regex search > > (3) search for dismissViewController (the function names will be mangled; you can use c++filt to unmangle a name and be sure you've got the right one) > > (4) see if the disassembled code contains calls to ref/deref or, more likely, since ref/deref are usually inlined, see if there's a call to the SOAuthorizationSession operator delete or destructor (which would happen in the unlikely deref path). > > But also: > > This theory of the bug is definitely wrong. The compiler has no basis to eliminate a capture when the act of capturing obviously has side effects (by calling ref() in the capture and deref() when out of scope). Also, if the compiler did have such a bug, other WebKit code would crash all the time. Let's revert the code change then. 1745217 12 jiewen_tan 2021-03-30 11:56:11 -0700 Reverted r275197 for reason: The change is spooky. Committed r275219 (235913@main): <https://commits.webkit.org/235913@main> 424426 2021-03-26 17:49:31 -0700 2021-03-29 23:35:18 -0700 Patch bug-223832-20210326174930.patch text/plain 3883 jiewen_tan U3VidmVyc2lvbiBSZXZpc2lvbjogMjc1MTA1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGU5NTg3ZjVkZjNjNDNmNjNk YTg5NmIwZWQxNWJmMjllZTk4ZDFkNjEuLjE4NjlmOTJmYzllN2VmNGZmYWMxYjNjNzM3ZWUwM2Jm Yjk0NTg5MWEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMjEtMDMtMjYgIEppZXdlbiBU YW4gIDxqaWV3ZW5fdGFuQGFwcGxlLmNvbT4KKworICAgICAgICBTYWZhcmkgY3Jhc2hlZCBhbmQg bG9zdCBhbGwgdGFicywgYWZ0ZXIgdW5sb2NraW5nIHNsZWVwaW5nIGRldmljZQorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjIzODMyCisgICAgICAgIDxy ZGFyOi8vNzU1NTUyODc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgQSBzcGVjdWxhdGl2ZSBmaXggZm9yIHRoaXMgY3Jhc2guIEEgcG9zc2libGUgZXhw bGFuYXRpb24gZnJvbSB0aGUgY3Jhc2ggbG9nIHN1Z2dlc3RzIHRoYXQgdGhlIHByb3RlY3RlZFRo aXMKKyAgICAgICAgY291bGQgYmUgZWxpZGVkIGJlY2F1c2Ugb2YgY29tcGlsZXIgb3B0aW1pemF0 aW9uIGdpdmVuIGl0IGlzIG5vdCB1c2VkIGluIHRoZSBibG9jay4gVG8gcHJldmVudCBzdWNoIG9w dGltaXphdGlvbiwKKyAgICAgICAgcHJvdGVjdGVkVGhpcyBpcyB0aGVyZWZvcmUgdXNlZCBleHBs aWNpdGx5IGluIHRoZSBibG9jay4KKworICAgICAgICAqIFVJUHJvY2Vzcy9Db2NvYS9TT0F1dGhv cml6YXRpb24vU09BdXRob3JpemF0aW9uU2Vzc2lvbi5tbToKKyAgICAgICAgKFdlYktpdDo6U09B dXRob3JpemF0aW9uU2Vzc2lvbjo6ZGlzbWlzc1ZpZXdDb250cm9sbGVyKToKKwogMjAyMS0wMy0y NiAgU2FpZCBBYm91LUhhbGxhd2EgIDxzYWlkQGFwcGxlLmNvbT4KIAogICAgICAgICBBbGxvdyBs b2dnaW5nIG1pbmltYWwgaW5mbyBhYm91dCB1cGxvYWRpbmcgbWVkaWEgZmlsZXMgaW4gdGhlIHN5 c3RlbSBkaWFnbm9zZQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQ29jb2Ev U09BdXRob3JpemF0aW9uL1NPQXV0aG9yaXphdGlvblNlc3Npb24ubW0gYi9Tb3VyY2UvV2ViS2l0 L1VJUHJvY2Vzcy9Db2NvYS9TT0F1dGhvcml6YXRpb24vU09BdXRob3JpemF0aW9uU2Vzc2lvbi5t bQppbmRleCBkZTA0MTNjZWNjZGZiOWE1MmYwNTgyZGJlYzc3MmM0ZTQzZWE2MGViLi4wNGUwZDIz YWMwMmNhODdiYzY2ZmFjMTI3MjU0Zjg0NWJiZDZiODk5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi S2l0L1VJUHJvY2Vzcy9Db2NvYS9TT0F1dGhvcml6YXRpb24vU09BdXRob3JpemF0aW9uU2Vzc2lv bi5tbQorKysgYi9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9Db2NvYS9TT0F1dGhvcml6YXRpb24v U09BdXRob3JpemF0aW9uU2Vzc2lvbi5tbQpAQCAtMzAyLDEwICszMDIsMTAgQEAgdm9pZCBTT0F1 dGhvcml6YXRpb25TZXNzaW9uOjpkaXNtaXNzVmlld0NvbnRyb2xsZXIoKQogICAgICAgICBpZiAo cHJlc2VudGluZ1dpbmRvdy5taW5pYXR1cml6ZWQpIHsKICAgICAgICAgICAgIGlmIChtX3ByZXNl bnRpbmdXaW5kb3dEaWREZW1pbmlhdHVyaXplT2JzZXJ2ZXIpCiAgICAgICAgICAgICAgICAgcmV0 dXJuOwotICAgICAgICAgICAgbV9wcmVzZW50aW5nV2luZG93RGlkRGVtaW5pYXR1cml6ZU9ic2Vy dmVyID0gW1tOU05vdGlmaWNhdGlvbkNlbnRlciBkZWZhdWx0Q2VudGVyXSBhZGRPYnNlcnZlckZv ck5hbWU6TlNXaW5kb3dEaWREZW1pbmlhdHVyaXplTm90aWZpY2F0aW9uIG9iamVjdDpwcmVzZW50 aW5nV2luZG93IHF1ZXVlOm5pbCB1c2luZ0Jsb2NrOltwcm90ZWN0ZWRUaGlzID0gbWFrZVJlZlB0 cih0aGlzKSwgdGhpc10gKE5TTm90aWZpY2F0aW9uICopIHsKLSAgICAgICAgICAgICAgICBkaXNt aXNzVmlld0NvbnRyb2xsZXIoKTsKLSAgICAgICAgICAgICAgICBbW05TTm90aWZpY2F0aW9uQ2Vu dGVyIGRlZmF1bHRDZW50ZXJdIHJlbW92ZU9ic2VydmVyOm1fcHJlc2VudGluZ1dpbmRvd0RpZERl bWluaWF0dXJpemVPYnNlcnZlci5nZXQoKV07Ci0gICAgICAgICAgICAgICAgbV9wcmVzZW50aW5n V2luZG93RGlkRGVtaW5pYXR1cml6ZU9ic2VydmVyID0gbnVsbHB0cjsKKyAgICAgICAgICAgIG1f cHJlc2VudGluZ1dpbmRvd0RpZERlbWluaWF0dXJpemVPYnNlcnZlciA9IFtbTlNOb3RpZmljYXRp b25DZW50ZXIgZGVmYXVsdENlbnRlcl0gYWRkT2JzZXJ2ZXJGb3JOYW1lOk5TV2luZG93RGlkRGVt aW5pYXR1cml6ZU5vdGlmaWNhdGlvbiBvYmplY3Q6cHJlc2VudGluZ1dpbmRvdyBxdWV1ZTpuaWwg dXNpbmdCbG9jazpbcHJvdGVjdGVkVGhpcyA9IG1ha2VSZWYoKnRoaXMpXSAoTlNOb3RpZmljYXRp b24gKikgeworICAgICAgICAgICAgICAgIHByb3RlY3RlZFRoaXMtPmRpc21pc3NWaWV3Q29udHJv bGxlcigpOworICAgICAgICAgICAgICAgIFtbTlNOb3RpZmljYXRpb25DZW50ZXIgZGVmYXVsdENl bnRlcl0gcmVtb3ZlT2JzZXJ2ZXI6cHJvdGVjdGVkVGhpcy0+bV9wcmVzZW50aW5nV2luZG93RGlk RGVtaW5pYXR1cml6ZU9ic2VydmVyLmdldCgpXTsKKyAgICAgICAgICAgICAgICBwcm90ZWN0ZWRU aGlzLT5tX3ByZXNlbnRpbmdXaW5kb3dEaWREZW1pbmlhdHVyaXplT2JzZXJ2ZXIgPSBudWxscHRy OwogICAgICAgICAgICAgfV07CiAgICAgICAgICAgICByZXR1cm47CiAgICAgICAgIH0KQEAgLTMx NCwxMCArMzE0LDEwIEBAIHZvaWQgU09BdXRob3JpemF0aW9uU2Vzc2lvbjo6ZGlzbWlzc1ZpZXdD b250cm9sbGVyKCkKICAgICBpZiAoTlNBcHAuaGlkZGVuKSB7CiAgICAgICAgIGlmIChtX2FwcGxp Y2F0aW9uRGlkVW5oaWRlT2JzZXJ2ZXIpCiAgICAgICAgICAgICByZXR1cm47Ci0gICAgICAgIG1f YXBwbGljYXRpb25EaWRVbmhpZGVPYnNlcnZlciA9IFtbTlNOb3RpZmljYXRpb25DZW50ZXIgZGVm YXVsdENlbnRlcl0gYWRkT2JzZXJ2ZXJGb3JOYW1lOk5TQXBwbGljYXRpb25EaWRVbmhpZGVOb3Rp ZmljYXRpb24gb2JqZWN0Ok5TQXBwIHF1ZXVlOm5pbCB1c2luZ0Jsb2NrOltwcm90ZWN0ZWRUaGlz ID0gbWFrZVJlZlB0cih0aGlzKSwgdGhpc10gKE5TTm90aWZpY2F0aW9uICopIHsKLSAgICAgICAg ICAgIGRpc21pc3NWaWV3Q29udHJvbGxlcigpOwotICAgICAgICAgICAgW1tOU05vdGlmaWNhdGlv bkNlbnRlciBkZWZhdWx0Q2VudGVyXSByZW1vdmVPYnNlcnZlcjptX2FwcGxpY2F0aW9uRGlkVW5o aWRlT2JzZXJ2ZXIuZ2V0KCldOwotICAgICAgICAgICAgbV9hcHBsaWNhdGlvbkRpZFVuaGlkZU9i c2VydmVyID0gbnVsbHB0cjsKKyAgICAgICAgbV9hcHBsaWNhdGlvbkRpZFVuaGlkZU9ic2VydmVy ID0gW1tOU05vdGlmaWNhdGlvbkNlbnRlciBkZWZhdWx0Q2VudGVyXSBhZGRPYnNlcnZlckZvck5h bWU6TlNBcHBsaWNhdGlvbkRpZFVuaGlkZU5vdGlmaWNhdGlvbiBvYmplY3Q6TlNBcHAgcXVldWU6 bmlsIHVzaW5nQmxvY2s6W3Byb3RlY3RlZFRoaXMgPSBtYWtlUmVmKCp0aGlzKV0gKE5TTm90aWZp Y2F0aW9uICopIHsKKyAgICAgICAgICAgIHByb3RlY3RlZFRoaXMtPmRpc21pc3NWaWV3Q29udHJv bGxlcigpOworICAgICAgICAgICAgW1tOU05vdGlmaWNhdGlvbkNlbnRlciBkZWZhdWx0Q2VudGVy XSByZW1vdmVPYnNlcnZlcjpwcm90ZWN0ZWRUaGlzLT5tX2FwcGxpY2F0aW9uRGlkVW5oaWRlT2Jz ZXJ2ZXIuZ2V0KCldOworICAgICAgICAgICAgcHJvdGVjdGVkVGhpcy0+bV9hcHBsaWNhdGlvbkRp ZFVuaGlkZU9ic2VydmVyID0gbnVsbHB0cjsKICAgICAgICAgfV07CiAgICAgICAgIHJldHVybjsK ICAgICB9Cg==