214745 2020-07-24 09:34:29 -0700 CrashTracer: com.apple.WebKit.WebContent at com.apple.WebKit: WebKit::WebFrame::shouldEnableInAppBrowserPrivacyProtections 2020-07-28 09:20:31 -0700 1 1 1 Unclassified WebKit WebKit Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 katherine_cheney katherine_cheney ap bfulgham cdumez ddkilzer oldest_to_newest 1674698 0 katherine_cheney 2020-07-24 09:34:29 -0700 WebKit: WebKit::WebFrame::shouldEnableInAppBrowserPrivacyProtections() is crashing. 1674699 1 katherine_cheney 2020-07-24 09:34:45 -0700 <rdar://66018965> 1674709 2 405160 katherine_cheney 2020-07-24 09:44:37 -0700 Created attachment 405160 Patch 1674713 3 405160 cdumez 2020-07-24 09:54:43 -0700 Comment on attachment 405160 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405160&action=review > Source/WebKit/WebProcess/WebPage/WebFrame.cpp:-865 > - for (WebFrame* frame = this; !frame->isMainFrame(); frame = frame->parentFrame()) { To maintain previous behavior, we should have used: frame && !frame->isMainFrame() > Source/WebKit/WebProcess/WebPage/WebFrame.cpp:865 > + for (WebFrame* frame = this; frame; frame = frame->parentFrame()) { This is a behavior change because we are now going to run the code in the loop for the main frame. Is that OK? 1674751 4 katherine_cheney 2020-07-24 10:56:48 -0700 (In reply to Chris Dumez from comment #3) > Comment on attachment 405160 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405160&action=review > > > Source/WebKit/WebProcess/WebPage/WebFrame.cpp:-865 > > - for (WebFrame* frame = this; !frame->isMainFrame(); frame = frame->parentFrame()) { > > To maintain previous behavior, we should have used: > frame && !frame->isMainFrame() > > > Source/WebKit/WebProcess/WebPage/WebFrame.cpp:865 > > + for (WebFrame* frame = this; frame; frame = frame->parentFrame()) { > > This is a behavior change because we are now going to run the code in the > loop for the main frame. Is that OK? Yes, this is ok. Right now we have a separate check in place in WebPageProxy.cpp where if the main frame is not app-bound, we will enable app-bound domain protections for all further loads, so we don't rely on this loop being run for the main frame. But it doesn't hurt to check it here as well. 1674756 5 405160 ap 2020-07-24 11:04:50 -0700 Comment on attachment 405160 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405160&action=review > Source/WebKit/ChangeLog:10 > + We should stop iterating if the frame is null, even if it is not > + the main frame, to avoid calling functions on a null frame. This means that we are running JS in a detached frame, which seems like a bigger problem that a mere crash. 1674798 6 katherine_cheney 2020-07-24 13:06:48 -0700 (In reply to Alexey Proskuryakov from comment #5) > Comment on attachment 405160 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405160&action=review > > > Source/WebKit/ChangeLog:10 > > + We should stop iterating if the frame is null, even if it is not > > + the main frame, to avoid calling functions on a null frame. > > This means that we are running JS in a detached frame, which seems like a > bigger problem that a mere crash. Agreed, perhaps this should be a separate investigation though. 1674810 7 405160 ap 2020-07-24 14:00:06 -0700 Comment on attachment 405160 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405160&action=review >>> Source/WebKit/ChangeLog:10 >>> + the main frame, to avoid calling functions on a null frame. >> >> This means that we are running JS in a detached frame, which seems like a bigger problem that a mere crash. > > Agreed, perhaps this should be a separate investigation though. I'm not sure if adding the null check will improve customer experience. Running JS in a detached frame will likely crash anyway, just in a different place, but it wold be just as bad for the user. It can also expose a security vulnerability that used to be prevented by the null pointer crash, but turns into a logic error without the crash. Sometimes we don't know what's happening and spray null pointer checks without much thinking, but this specific case seems to be more suspicious than most. 1674815 8 405160 cdumez 2020-07-24 14:13:22 -0700 Comment on attachment 405160 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405160&action=review >>>> Source/WebKit/ChangeLog:10 >>>> + the main frame, to avoid calling functions on a null frame. >>> >>> This means that we are running JS in a detached frame, which seems like a bigger problem that a mere crash. >> >> Agreed, perhaps this should be a separate investigation though. > > I'm not sure if adding the null check will improve customer experience. Running JS in a detached frame will likely crash anyway, just in a different place, but it wold be just as bad for the user. It can also expose a security vulnerability that used to be prevented by the null pointer crash, but turns into a logic error without the crash. > > Sometimes we don't know what's happening and spray null pointer checks without much thinking, but this specific case seems to be more suspicious than most. This is a top crasher and a very recent regression from Kate adding this shouldEnableInAppBrowserPrivacyProtections() check. I think a fix in this shouldEnableInAppBrowserPrivacyProtections() is very reasonable for such crash. Whether or not this function was called on detached frames did NOT change when Kate added the shouldEnableInAppBrowserPrivacyProtections() check. Also note that if we were crashing before Kate's patch then we would have seen a top crasher in this code path. I am not aware that we have. We don't have evidence that this method crashes or does something bad when called on a detached frame. We do have evidence that Kate's recent addition of the shouldEnableInAppBrowserPrivacyProtections() introduced a crash and she is fixing that crash. 1674816 9 katherine_cheney 2020-07-24 14:17:16 -0700 (In reply to Alexey Proskuryakov from comment #7) > Comment on attachment 405160 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405160&action=review > > >>> Source/WebKit/ChangeLog:10 > >>> + the main frame, to avoid calling functions on a null frame. > >> > >> This means that we are running JS in a detached frame, which seems like a bigger problem that a mere crash. > > > > Agreed, perhaps this should be a separate investigation though. > > I'm not sure if adding the null check will improve customer experience. > Running JS in a detached frame will likely crash anyway, just in a different > place, but it wold be just as bad for the user. It can also expose a > security vulnerability that used to be prevented by the null pointer crash, > but turns into a logic error without the crash. > > Sometimes we don't know what's happening and spray null pointer checks > without much thinking, but this specific case seems to be more suspicious > than most. My thought is that fixing this will return customer experience to whatever it was prior to this code being added, which is probably like you said another crash or protection put in place to prevent JS execution in a detached frame. If it's another crash, we will see it in CrashTracer. If it's a handled error, we probably don't need to do anything. And if it is not addressed, we can fix it in a separate patch. Do you have another idea for addressing this bug? 1674818 10 ap 2020-07-24 14:18:21 -0700 > a very recent regression from Kate adding this shouldEnableInAppBrowserPrivacyProtections() check OK then. 1674824 11 ews-feeder 2020-07-24 14:32:24 -0700 Committed r264859: <https://trac.webkit.org/changeset/264859> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405160. 1674941 12 ap 2020-07-24 18:15:23 -0700 What is the next step for addressing the root cause? 1675636 13 katherine_cheney 2020-07-28 09:20:31 -0700 (In reply to Alexey Proskuryakov from comment #12) > What is the next step for addressing the root cause? <rdar://problem/66221089> 405160 2020-07-24 09:44:37 -0700 2020-07-24 14:32:24 -0700 Patch bug-214745-20200724094435.patch text/plain 1908 katherine_cheney U3VidmVyc2lvbiBSZXZpc2lvbjogMjY0NjYzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDY4ODU2NWRhY2E5NWU0YWIy ZThmMzAyMmU3NTA2YjcwMDc0MjNkMDMuLmQ0MDcyNTg5MDJlMDhhOTVhN2JhNTA0ODAwNDdkMDU2 MmEzMDQ4NTggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMjAtMDctMjQgIEthdGUgQ2hl bmV5ICA8a2F0aGVyaW5lX2NoZW5leUBhcHBsZS5jb20+CisKKyAgICAgICAgQ3Jhc2hUcmFjZXI6 IGNvbS5hcHBsZS5XZWJLaXQuV2ViQ29udGVudCBhdCBjb20uYXBwbGUuV2ViS2l0OiBXZWJLaXQ6 OldlYkZyYW1lOjpzaG91bGRFbmFibGVJbkFwcEJyb3dzZXJQcml2YWN5UHJvdGVjdGlvbnMKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIxNDc0NQorICAg ICAgICA8cmRhcjovLzY2MDE4OTY1PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIFdlIHNob3VsZCBzdG9wIGl0ZXJhdGluZyBpZiB0aGUgZnJhbWUgaXMg bnVsbCwgZXZlbiBpZiBpdCBpcyBub3QKKyAgICAgICAgdGhlIG1haW4gZnJhbWUsIHRvIGF2b2lk IGNhbGxpbmcgZnVuY3Rpb25zIG9uIGEgbnVsbCBmcmFtZS4KKworICAgICAgICAqIFdlYlByb2Nl c3MvV2ViUGFnZS9XZWJGcmFtZS5jcHA6CisgICAgICAgIChXZWJLaXQ6OldlYkZyYW1lOjpzaG91 bGRFbmFibGVJbkFwcEJyb3dzZXJQcml2YWN5UHJvdGVjdGlvbnMpOgorCiAyMDIwLTA3LTIxICBF cmljIENhcmxzb24gIDxlcmljLmNhcmxzb25AYXBwbGUuY29tPgogCiAgICAgICAgIFVzZSBBVlJv dXRlUGlja2VyVmlldyB3aGVuIGF2YWlsYWJsZSBmb3IgY2hvb3NpbmcgQWlyUGxheSBkZXZpY2Vz CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L1dlYlByb2Nlc3MvV2ViUGFnZS9XZWJGcmFtZS5j cHAgYi9Tb3VyY2UvV2ViS2l0L1dlYlByb2Nlc3MvV2ViUGFnZS9XZWJGcmFtZS5jcHAKaW5kZXgg NzVkYjY0YWY0YTJkMWNjODllODBkZDNlN2FhMzBhOTNlOGI3NjhmMS4uZDFhMDY4OWUwY2ZmMWQ0 ODM2Y2UxZmMxYzEzN2IxZGIyYTI2Mjk1MCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9XZWJQ cm9jZXNzL1dlYlBhZ2UvV2ViRnJhbWUuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvV2ViUHJvY2Vz cy9XZWJQYWdlL1dlYkZyYW1lLmNwcApAQCAtODYyLDcgKzg2Miw3IEBAIGJvb2wgV2ViRnJhbWU6 OnNob3VsZEVuYWJsZUluQXBwQnJvd3NlclByaXZhY3lQcm90ZWN0aW9ucygpCiAKICAgICBib29s IHRyZWVIYXNOb25BcHBCb3VuZEZyYW1lID0gbV9pc05hdmlnYXRpbmdUb0FwcEJvdW5kRG9tYWlu ICYmIG1faXNOYXZpZ2F0aW5nVG9BcHBCb3VuZERvbWFpbiA9PSBOYXZpZ2F0aW5nVG9BcHBCb3Vu ZERvbWFpbjo6Tm87CiAgICAgaWYgKCF0cmVlSGFzTm9uQXBwQm91bmRGcmFtZSkgewotICAgICAg ICBmb3IgKFdlYkZyYW1lKiBmcmFtZSA9IHRoaXM7ICFmcmFtZS0+aXNNYWluRnJhbWUoKTsgZnJh bWUgPSBmcmFtZS0+cGFyZW50RnJhbWUoKSkgeworICAgICAgICBmb3IgKFdlYkZyYW1lKiBmcmFt ZSA9IHRoaXM7IGZyYW1lOyBmcmFtZSA9IGZyYW1lLT5wYXJlbnRGcmFtZSgpKSB7CiAgICAgICAg ICAgICBpZiAoZnJhbWUtPmlzTmF2aWdhdGluZ1RvQXBwQm91bmREb21haW4oKSAmJiBmcmFtZS0+ aXNOYXZpZ2F0aW5nVG9BcHBCb3VuZERvbWFpbigpID09IE5hdmlnYXRpbmdUb0FwcEJvdW5kRG9t YWluOjpObykgewogICAgICAgICAgICAgICAgIHRyZWVIYXNOb25BcHBCb3VuZEZyYW1lID0gdHJ1 ZTsKICAgICAgICAgICAgICAgICBicmVhazsK