156409 2016-04-08 11:17:03 -0700 [WK1] Wheel event callback removing the window causes crash in WebCore 2016-04-08 17:21:48 -0700 1 1 1 Unclassified WebKit UI Events WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 150871 156420 1 bfulgham bfulgham bfulgham simon.fraser webkit-bug-importer oldest_to_newest 1182218 0 bfulgham 2016-04-08 11:17:03 -0700 This is a follow-up to Bug 150871, which dealt with this problem in a WK2 context. If a window is removed while it is triggering a wheel event, it will crash with a bad memory access, because WebKit will attempt to use the destroyed view. While we corrected the WK2 version of this problem, WK1 takes a slightly different code path that needs to be fixed. The crash is shown by fast/events/wheel-event-destroys-frame.html. 1182263 1 webkit-bug-importer 2016-04-08 12:47:55 -0700 <rdar://problem/25631267> 1182264 2 276027 bfulgham 2016-04-08 12:51:22 -0700 Created attachment 276027 Patch 1182265 3 276027 bfulgham 2016-04-08 12:53:50 -0700 Comment on attachment 276027 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276027&action=review > Source/WebCore/page/EventHandler.cpp:2679 > + return platformCompletePlatformWidgetWheelEvent(event, *widget, scrollableContainer.get()); Most of this is whitespace change due to removing a level of indent. 1182282 4 276027 simon.fraser 2016-04-08 13:17:36 -0700 Comment on attachment 276027 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276027&action=review > Source/WebCore/page/EventHandler.cpp:-2654 > - if (is<RenderWidget>(target)) { I don't get the change because is<RenderWidget>(target) already null-checks. 1182286 5 276027 bfulgham 2016-04-08 13:27:16 -0700 Comment on attachment 276027 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276027&action=review >> Source/WebCore/page/EventHandler.cpp:-2654 >> - if (is<RenderWidget>(target)) { > > I don't get the change because is<RenderWidget>(target) already null-checks. This isn't the relevant part of the change. This stuff only changed because I moved it into a helper function so I could call it twice. > Source/WebCore/page/EventHandler.cpp:2670 > + Widget* widget = widgetForElement(*element); THIS is the change. I found that in WK1 the underlying RenderWidget gets destroyed when the iFrame is destroyed, so the "widget" pointer pointed to deallocated memory. This "widgetForElement" call re-gets it, and will return nullptr in the case of a destroyed iFrame. 1182287 6 276027 simon.fraser 2016-04-08 13:29:50 -0700 Comment on attachment 276027 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276027&action=review >> Source/WebCore/page/EventHandler.cpp:2670 >> + Widget* widget = widgetForElement(*element); > > THIS is the change. I found that in WK1 the underlying RenderWidget gets destroyed when the iFrame is destroyed, so the "widget" pointer pointed to deallocated memory. This "widgetForElement" call re-gets it, and will return nullptr in the case of a destroyed iFrame. I see. I think the fact that passWheelEventToWidget() is in the condition makes it a little harder to see that it could make the widget invalid. 1182295 7 276027 bfulgham 2016-04-08 13:43:27 -0700 Comment on attachment 276027 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276027&action=review >>> Source/WebCore/page/EventHandler.cpp:2670 >>> + Widget* widget = widgetForElement(*element); >> >> THIS is the change. I found that in WK1 the underlying RenderWidget gets destroyed when the iFrame is destroyed, so the "widget" pointer pointed to deallocated memory. This "widgetForElement" call re-gets it, and will return nullptr in the case of a destroyed iFrame. > > I see. I think the fact that passWheelEventToWidget() is in the condition makes it a little harder to see that it could make the widget invalid. That is a little confusing. That's actually not new to this patch, but I agree it would be easier to understand without doing both tests on the same line. I think it was done that way originally to avoid using two levels of indent. Of course, I probably did the original code, too :-\ 1182296 8 bfulgham 2016-04-08 13:46:22 -0700 Committed r199245: <http://trac.webkit.org/changeset/199245> 276027 2016-04-08 12:51:22 -0700 2016-04-08 13:29:50 -0700 Patch bug-156409-20160408125127.patch text/plain 3580 bfulgham SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE5OTI0MSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI0IEBACisyMDE2LTA0LTA4ICBCcmVudCBG dWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFtXSzFdIFdoZWVsIGV2ZW50 IGNhbGxiYWNrIHJlbW92aW5nIHRoZSB3aW5kb3cgY2F1c2VzIGNyYXNoIGluIFdlYkNvcmUKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1NjQwOQorICAg ICAgICA8cmRhcjovL3Byb2JsZW0vMjU2MzEyNjc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgTnVsbCBjaGVjayB0aGUgV2lkZ2V0IGJlZm9yZSB1c2lu ZyBpdCwgc2luY2UgdGhlIGlmcmFtZSBtYXkgaGF2ZSBiZWVuIHJlbW92ZWQKKyAgICAgICAgZnJv bSBpdHMgcGFyZW50IGRvY3VtZW50IGluc2lkZSB0aGUgZXZlbnQgaGFuZGxlci4KKworICAgICAg ICBUaGlzIGlzIHRoZSBXSzEgZml4IGZvciBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MTUwODcxLgorCisgICAgICAgIFRlc3RlZCBieSBmYXN0L2V2ZW50cy93aGVlbC1l dmVudC1kZXN0cm95cy1mcmFtZS5odG1sCisKKyAgICAgICAgKiBwYWdlL0V2ZW50SGFuZGxlci5j cHA6CisgICAgICAgIChXZWJDb3JlOjp3aWRnZXRGb3JFbGVtZW50KTogQWRkZWQuCisgICAgICAg IChXZWJDb3JlOjpFdmVudEhhbmRsZXI6OmhhbmRsZVdoZWVsRXZlbnQpOiBVc2UgbmV3IGhlbHBl ciBmdW5jdGlvbiB0bworICAgICAgICBjbGVhbiB1cCB0aGUgY29kZSwgYW5kIGFsbG93IHVzIHRv IGNoZWNrIHRoYXQgdGhlIFdpZGdldCBoYXMgbm90IGJlZW4KKyAgICAgICAgZGVzdHJveWVkIGR1 cmluZyB0aGUgZXZlbnQgaGFuZGxlci4KKwogMjAxNi0wNC0wOCAgQmV0aCBEYWtpbiAgPGJkYWtp bkBhcHBsZS5jb20+CiAKICAgICAgICAgRml4IGxlYWtzIGluIFdlYkFWTWVkaWFTZWxlY3Rpb25P cHRpb25NYWMgYW5kIFdlYlBsYXliYWNrQ29udHJvbHNNYW5hZ2VyCkluZGV4OiBTb3VyY2UvV2Vi Q29yZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUv cGFnZS9FdmVudEhhbmRsZXIuY3BwCShyZXZpc2lvbiAxOTkyMzEpCisrKyBTb3VyY2UvV2ViQ29y ZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTI2MTAsNiArMjYxMCwx OCBAQCB2b2lkIEV2ZW50SGFuZGxlcjo6Y2xlYXJPclNjaGVkdWxlQ2xlYXJpCiB9CiAjZW5kaWYK IAorc3RhdGljIFdpZGdldCogd2lkZ2V0Rm9yRWxlbWVudChjb25zdCBFbGVtZW50JiBlbGVtZW50 KQoreworICAgIFJlbmRlckVsZW1lbnQqIHRhcmdldCA9IGVsZW1lbnQucmVuZGVyZXIoKTsKKyAg ICBpZiAoIXRhcmdldCkKKyAgICAgICAgcmV0dXJuIG51bGxwdHI7CisKKyAgICBpZiAoIWlzPFJl bmRlcldpZGdldD4odGFyZ2V0KSkKKyAgICAgICAgcmV0dXJuIG51bGxwdHI7CisKKyAgICByZXR1 cm4gZG93bmNhc3Q8UmVuZGVyV2lkZ2V0PigqdGFyZ2V0KS53aWRnZXQoKTsKK30KKwogYm9vbCBF dmVudEhhbmRsZXI6OmhhbmRsZVdoZWVsRXZlbnQoY29uc3QgUGxhdGZvcm1XaGVlbEV2ZW50JiBl dmVudCkKIHsKICAgICBSZW5kZXJWaWV3KiByZW5kZXJWaWV3ID0gbV9mcmFtZS5jb250ZW50UmVu ZGVyZXIoKTsKQEAgLTI2NTAsMTggKzI2NjIsMjEgQEAgYm9vbCBFdmVudEhhbmRsZXI6OmhhbmRs ZVdoZWVsRXZlbnQoY29ucwogCiAgICAgaWYgKGVsZW1lbnQpIHsKICAgICAgICAgaWYgKGlzT3Zl cldpZGdldCkgewotICAgICAgICAgICAgUmVuZGVyRWxlbWVudCogdGFyZ2V0ID0gZWxlbWVudC0+ cmVuZGVyZXIoKTsKLSAgICAgICAgICAgIGlmIChpczxSZW5kZXJXaWRnZXQ+KHRhcmdldCkpIHsK LSAgICAgICAgICAgICAgICBXaWRnZXQqIHdpZGdldCA9IGRvd25jYXN0PFJlbmRlcldpZGdldD4o KnRhcmdldCkud2lkZ2V0KCk7Ci0gICAgICAgICAgICAgICAgaWYgKHdpZGdldCAmJiBwYXNzV2hl ZWxFdmVudFRvV2lkZ2V0KGV2ZW50LCAqd2lkZ2V0KSkgewotICAgICAgICAgICAgICAgICAgICBt X2lzSGFuZGxpbmdXaGVlbEV2ZW50ID0gZmFsc2U7Ci0gICAgICAgICAgICAgICAgICAgIGlmIChz Y3JvbGxhYmxlQXJlYSkKLSAgICAgICAgICAgICAgICAgICAgICAgIHNjcm9sbGFibGVBcmVhLT5z ZXRTY3JvbGxlZFByb2dyYW1tYXRpY2FsbHkoZmFsc2UpOwotICAgICAgICAgICAgICAgICAgICBw bGF0Zm9ybU5vdGlmeUlmRW5kR2VzdHVyZShhZGp1c3RlZEV2ZW50LCBzY3JvbGxhYmxlQXJlYSk7 Ci0gICAgICAgICAgICAgICAgICAgIGlmICghd2lkZ2V0LT5wbGF0Zm9ybVdpZGdldCgpKQotICAg ICAgICAgICAgICAgICAgICAgICAgcmV0dXJuIHRydWU7Ci0gICAgICAgICAgICAgICAgICAgIHJl dHVybiBwbGF0Zm9ybUNvbXBsZXRlUGxhdGZvcm1XaWRnZXRXaGVlbEV2ZW50KGV2ZW50LCAqd2lk Z2V0LCBzY3JvbGxhYmxlQ29udGFpbmVyLmdldCgpKTsKLSAgICAgICAgICAgICAgICB9CisgICAg ICAgICAgICBXaWRnZXQqIHdpZGdldCA9IHdpZGdldEZvckVsZW1lbnQoKmVsZW1lbnQpOworICAg ICAgICAgICAgaWYgKHdpZGdldCAmJiBwYXNzV2hlZWxFdmVudFRvV2lkZ2V0KGV2ZW50LCAqd2lk Z2V0KSkgeworICAgICAgICAgICAgICAgIG1faXNIYW5kbGluZ1doZWVsRXZlbnQgPSBmYWxzZTsK KworICAgICAgICAgICAgICAgIC8vIFdlIGRvIGFub3RoZXIgY2hlY2sgb24gdGhlIHdpZGdldCBi ZWNhdXNlIHRoZSBldmVudCBoYW5kbGVyIGNhbiBydW4gSlMgd2hpY2ggcmVzdWx0cyBpbiB0aGUg ZnJhbWUgZ2V0dGluZyBkZXN0cm95ZWQuCisgICAgICAgICAgICAgICAgV2lkZ2V0KiB3aWRnZXQg PSB3aWRnZXRGb3JFbGVtZW50KCplbGVtZW50KTsKKyAgICAgICAgICAgICAgICBpZiAoIXdpZGdl dCkKKyAgICAgICAgICAgICAgICAgICAgcmV0dXJuIGZhbHNlOworCisgICAgICAgICAgICAgICAg aWYgKHNjcm9sbGFibGVBcmVhKQorICAgICAgICAgICAgICAgICAgICBzY3JvbGxhYmxlQXJlYS0+ c2V0U2Nyb2xsZWRQcm9ncmFtbWF0aWNhbGx5KGZhbHNlKTsKKyAgICAgICAgICAgICAgICBwbGF0 Zm9ybU5vdGlmeUlmRW5kR2VzdHVyZShhZGp1c3RlZEV2ZW50LCBzY3JvbGxhYmxlQXJlYSk7Cisg ICAgICAgICAgICAgICAgaWYgKCF3aWRnZXQtPnBsYXRmb3JtV2lkZ2V0KCkpCisgICAgICAgICAg ICAgICAgICAgIHJldHVybiB0cnVlOworICAgICAgICAgICAgICAgIHJldHVybiBwbGF0Zm9ybUNv bXBsZXRlUGxhdGZvcm1XaWRnZXRXaGVlbEV2ZW50KGV2ZW50LCAqd2lkZ2V0LCBzY3JvbGxhYmxl Q29udGFpbmVyLmdldCgpKTsKICAgICAgICAgICAgIH0KICAgICAgICAgfQogCg==