177960 2017-10-05 13:00:25 -0700 RenderButton should not hold raw pointers to its direct children. 2017-10-05 14:14:49 -0700 1 1 1 Unclassified WebKit Layout and Rendering WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 zalan zalan bfulgham commit-queue koivisto simon.fraser webkit-bug-importer zalan oldest_to_newest 1357184 0 zalan 2017-10-05 13:00:25 -0700 m_buttonText and m_inner 1357185 1 webkit-bug-importer 2017-10-05 13:01:04 -0700 <rdar://problem/34840807> 1357189 2 322888 zalan 2017-10-05 13:05:08 -0700 Created attachment 322888 Patch 1357192 3 322888 koivisto 2017-10-05 13:10:00 -0700 Comment on attachment 322888 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322888&action=review > Source/WebCore/rendering/RenderButton.cpp:83 > // violated. > if (&oldChild == m_inner || !m_inner || oldChild.parent() == this) { > ASSERT(&oldChild == m_inner || !m_inner); The comment above talks about "security problems" that I don't think exist with the recent improvements. Maybe the comment, oldChild.parent() == this test and the assert can go? 1357193 4 322888 simon.fraser 2017-10-05 13:10:22 -0700 Comment on attachment 322888 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322888&action=review > Source/WebCore/rendering/RenderButton.cpp:139 > + return { }; Is this as efficient as returning emptyString()? > Source/WebCore/rendering/RenderButton.h:74 > + WeakPtr<RenderBlock> m_inner; Do we actually need to reference this, or can we find it via a tree walk? 1357210 5 322888 koivisto 2017-10-05 13:17:45 -0700 Comment on attachment 322888 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322888&action=review >> Source/WebCore/rendering/RenderButton.cpp:139 >> + return { }; > > Is this as efficient as returning emptyString()? That returns a null string, not an empty string (and is more efficient). 1357229 6 zalan 2017-10-05 13:45:43 -0700 (In reply to Antti Koivisto from comment #3) > Comment on attachment 322888 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322888&action=review > > > Source/WebCore/rendering/RenderButton.cpp:83 > > // violated. > > if (&oldChild == m_inner || !m_inner || oldChild.parent() == this) { > > ASSERT(&oldChild == m_inner || !m_inner); > > The comment above talks about "security problems" that I don't think exist > with the recent improvements. Maybe the comment, oldChild.parent() == this > test and the assert can go? The comment/assert actually attempts to catch cases where the RenderButton's inner subtree gets split up for whatever reason (the original bug was column spanning). So I think this is still valid (though not super useful, I admit). 1357240 7 322888 commit-queue 2017-10-05 14:14:47 -0700 Comment on attachment 322888 Patch Clearing flags on attachment: 322888 Committed r222932: <http://trac.webkit.org/changeset/222932> 1357241 8 commit-queue 2017-10-05 14:14:49 -0700 All reviewed patches have been landed. Closing bug. 322888 2017-10-05 13:05:08 -0700 2017-10-05 14:14:47 -0700 Patch bug-177960-20171005130508.patch text/plain 4764 zalan U3VidmVyc2lvbiBSZXZpc2lvbjogMjIyOTExCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYWRlN2Y4MWMyZDY2ZGRk MTNmMjVkYWZlZDIyNzIxNWUwYzUzN2QzNS4uZTNhMDQyYTBiNTljOTM5ODQ0ZThkN2I5NGYxODU5 OTgxMTBhZDMzNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDI2IEBACiAyMDE3LTEwLTA1ICBaYWxh biBCdWp0YXMgIDx6YWxhbkBhcHBsZS5jb20+CiAKKyAgICAgICAgUmVuZGVyQnV0dG9uIHNob3Vs ZCBub3QgaG9sZCByYXcgcG9pbnRlcnMgdG8gaXRzIGRpcmVjdCBjaGlsZHJlbi4KKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE3Nzk2MAorICAgICAgICA8 cmRhcjovL3Byb2JsZW0vMzQ4NDA4MDc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgVGhlIGNvcnJlY3Qgd2F5IG9mIGRlc3Ryb3lpbmcgYSByZW5kZXJl ciBpcyB0byBjYWxsIDo6cmVtb3ZlRnJvbVBhcmVudEFuZERlc3Ryb3koKS4KKworICAgICAgICBD b3ZlcmVkIGJ5IGV4aXN0aW5nIHRlc3RzLgorCisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlckJ1 dHRvbi5jcHA6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJCdXR0b246OlJlbmRlckJ1dHRvbik6 CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJCdXR0b246OmFkZENoaWxkKToKKyAgICAgICAgKFdl YkNvcmU6OlJlbmRlckJ1dHRvbjo6dGFrZUNoaWxkKToKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRl ckJ1dHRvbjo6dXBkYXRlQW5vbnltb3VzQ2hpbGRTdHlsZSBjb25zdCk6CisgICAgICAgIChXZWJD b3JlOjpSZW5kZXJCdXR0b246OnNldFRleHQpOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyQnV0 dG9uOjp0ZXh0IGNvbnN0KToKKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyQnV0dG9uLmg6CisK KzIwMTctMTAtMDUgIFphbGFuIEJ1anRhcyAgPHphbGFuQGFwcGxlLmNvbT4KKwogICAgICAgICBN b3ZlIG11bHRpY29sdW1uIGZsb3cgY2xlYXIgdG8gUmVuZGVyVHJlZVVwZGF0ZXIKICAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE3Nzg5OAogICAgICAgICA8 cmRhcjovL3Byb2JsZW0vMzQ4MjAxNTc+CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9yZW5k ZXJpbmcvUmVuZGVyQnV0dG9uLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJC dXR0b24uY3BwCmluZGV4IGY1NTIzNDQ5YjJiOGUzNzlhYzcxZTM0NzdiOGUxYTMxNWQ0YjUxNmMu LjE4YmYyM2UxZjE4NWRhYWIwNGJmYTEwYjUxNGQ5MjQ0M2E3ZjYwZDIgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJCdXR0b24uY3BwCisrKyBiL1NvdXJjZS9XZWJD b3JlL3JlbmRlcmluZy9SZW5kZXJCdXR0b24uY3BwCkBAIC0zOSw4ICszOSw2IEBAIHVzaW5nIG5h bWVzcGFjZSBIVE1MTmFtZXM7CiAKIFJlbmRlckJ1dHRvbjo6UmVuZGVyQnV0dG9uKEhUTUxGb3Jt Q29udHJvbEVsZW1lbnQmIGVsZW1lbnQsIFJlbmRlclN0eWxlJiYgc3R5bGUpCiAgICAgOiBSZW5k ZXJGbGV4aWJsZUJveChlbGVtZW50LCBXVEZNb3ZlKHN0eWxlKSkKLSAgICAsIG1fYnV0dG9uVGV4 dCgwKQotICAgICwgbV9pbm5lcigwKQogewogfQogCkBAIC03MCwxMCArNjgsOSBAQCB2b2lkIFJl bmRlckJ1dHRvbjo6YWRkQ2hpbGQoUmVuZGVyUHRyPFJlbmRlck9iamVjdD4gbmV3Q2hpbGQsIFJl bmRlck9iamVjdCogYmVmbwogICAgICAgICBBU1NFUlQoIWZpcnN0Q2hpbGQoKSk7CiAgICAgICAg IGF1dG8gbmV3SW5uZXIgPSBjcmVhdGVBbm9ueW1vdXNCbG9jayhzdHlsZSgpLmRpc3BsYXkoKSk7 CiAgICAgICAgIHVwZGF0ZUFub255bW91c0NoaWxkU3R5bGUoKm5ld0lubmVyLCBuZXdJbm5lci0+ bXV0YWJsZVN0eWxlKCkpOwotICAgICAgICBtX2lubmVyID0gbmV3SW5uZXIuZ2V0KCk7CisgICAg ICAgIG1faW5uZXIgPSBtYWtlV2Vha1B0cigqbmV3SW5uZXIpOwogICAgICAgICBSZW5kZXJGbGV4 aWJsZUJveDo6YWRkQ2hpbGQoV1RGTW92ZShuZXdJbm5lcikpOwotICAgIH0KLSAgICAKKyAgICB9 ICAgIAogICAgIG1faW5uZXItPmFkZENoaWxkKFdURk1vdmUobmV3Q2hpbGQpLCBiZWZvcmVDaGls ZCk7CiB9CiAKQEAgLTg0LDcgKzgxLDYgQEAgUmVuZGVyUHRyPFJlbmRlck9iamVjdD4gUmVuZGVy QnV0dG9uOjp0YWtlQ2hpbGQoUmVuZGVyT2JqZWN0JiBvbGRDaGlsZCkKICAgICAvLyB2aW9sYXRl ZC4KICAgICBpZiAoJm9sZENoaWxkID09IG1faW5uZXIgfHwgIW1faW5uZXIgfHwgb2xkQ2hpbGQu cGFyZW50KCkgPT0gdGhpcykgewogICAgICAgICBBU1NFUlQoJm9sZENoaWxkID09IG1faW5uZXIg fHwgIW1faW5uZXIpOwotICAgICAgICBtX2lubmVyID0gbnVsbHB0cjsKICAgICAgICAgcmV0dXJu IFJlbmRlckZsZXhpYmxlQm94Ojp0YWtlQ2hpbGQob2xkQ2hpbGQpOwogICAgIH0KICAgICByZXR1 cm4gbV9pbm5lci0+dGFrZUNoaWxkKG9sZENoaWxkKTsKQEAgLTkzLDcgKzg5LDYgQEAgUmVuZGVy UHRyPFJlbmRlck9iamVjdD4gUmVuZGVyQnV0dG9uOjp0YWtlQ2hpbGQoUmVuZGVyT2JqZWN0JiBv bGRDaGlsZCkKIHZvaWQgUmVuZGVyQnV0dG9uOjp1cGRhdGVBbm9ueW1vdXNDaGlsZFN0eWxlKGNv bnN0IFJlbmRlck9iamVjdCYgY2hpbGQsIFJlbmRlclN0eWxlJiBjaGlsZFN0eWxlKSBjb25zdAog ewogICAgIEFTU0VSVF9VTlVTRUQoY2hpbGQsICFtX2lubmVyIHx8ICZjaGlsZCA9PSBtX2lubmVy KTsKLSAgICAKICAgICBjaGlsZFN0eWxlLnNldEZsZXhHcm93KDEuMGYpOwogICAgIC8vIG1pbi13 aWR0aDogMDsgaXMgbmVlZGVkIGZvciBjb3JyZWN0IHNocmlua2luZy4KICAgICBjaGlsZFN0eWxl LnNldE1pbldpZHRoKExlbmd0aCgwLCBGaXhlZCkpOwpAQCAtMTIwLDI1ICsxMTUsMjggQEAgdm9p ZCBSZW5kZXJCdXR0b246OnVwZGF0ZUZyb21FbGVtZW50KCkKIAogdm9pZCBSZW5kZXJCdXR0b246 OnNldFRleHQoY29uc3QgU3RyaW5nJiBzdHIpCiB7Ci0gICAgaWYgKHN0ci5pc0VtcHR5KCkpIHsK LSAgICAgICAgaWYgKG1fYnV0dG9uVGV4dCkgewotICAgICAgICAgICAgbV9idXR0b25UZXh0LT5k ZXN0cm95KCk7Ci0gICAgICAgICAgICBtX2J1dHRvblRleHQgPSAwOwotICAgICAgICB9Ci0gICAg fSBlbHNlIHsKLSAgICAgICAgaWYgKG1fYnV0dG9uVGV4dCkKLSAgICAgICAgICAgIG1fYnV0dG9u VGV4dC0+c2V0VGV4dChzdHIuaW1wbCgpKTsKLSAgICAgICAgZWxzZSB7Ci0gICAgICAgICAgICBh dXRvIG5ld0J1dHRvblRleHQgPSBjcmVhdGVSZW5kZXJlcjxSZW5kZXJUZXh0RnJhZ21lbnQ+KGRv Y3VtZW50KCksIHN0cik7Ci0gICAgICAgICAgICBtX2J1dHRvblRleHQgPSBuZXdCdXR0b25UZXh0 LmdldCgpOwotICAgICAgICAgICAgYWRkQ2hpbGQoV1RGTW92ZShuZXdCdXR0b25UZXh0KSk7Ci0g ICAgICAgIH0KKyAgICBpZiAoIW1fYnV0dG9uVGV4dCAmJiBzdHIuaXNFbXB0eSgpKQorICAgICAg ICByZXR1cm47CisKKyAgICBpZiAoIW1fYnV0dG9uVGV4dCkgeworICAgICAgICBhdXRvIG5ld0J1 dHRvblRleHQgPSBjcmVhdGVSZW5kZXJlcjxSZW5kZXJUZXh0RnJhZ21lbnQ+KGRvY3VtZW50KCks IHN0cik7CisgICAgICAgIG1fYnV0dG9uVGV4dCA9IG1ha2VXZWFrUHRyKCpuZXdCdXR0b25UZXh0 KTsKKyAgICAgICAgYWRkQ2hpbGQoV1RGTW92ZShuZXdCdXR0b25UZXh0KSk7CisgICAgICAgIHJl dHVybjsKKyAgICB9CisKKyAgICBpZiAoIXN0ci5pc0VtcHR5KCkpIHsKKyAgICAgICAgbV9idXR0 b25UZXh0LT5zZXRUZXh0KHN0ci5pbXBsKCkpOworICAgICAgICByZXR1cm47CiAgICAgfQorICAg IG1fYnV0dG9uVGV4dC0+cmVtb3ZlRnJvbVBhcmVudEFuZERlc3Ryb3koKTsKIH0KIAogU3RyaW5n IFJlbmRlckJ1dHRvbjo6dGV4dCgpIGNvbnN0CiB7Ci0gICAgcmV0dXJuIG1fYnV0dG9uVGV4dCA/ IG1fYnV0dG9uVGV4dC0+dGV4dCgpIDogMDsKKyAgICBpZiAobV9idXR0b25UZXh0KQorICAgICAg ICByZXR1cm4gbV9idXR0b25UZXh0LT50ZXh0KCk7CisgICAgcmV0dXJuIHsgfTsKIH0KIAogYm9v bCBSZW5kZXJCdXR0b246OmNhbkhhdmVHZW5lcmF0ZWRDaGlsZHJlbigpIGNvbnN0CmRpZmYgLS1n aXQgYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQnV0dG9uLmggYi9Tb3VyY2UvV2Vi Q29yZS9yZW5kZXJpbmcvUmVuZGVyQnV0dG9uLmgKaW5kZXggMjg2ZGU3ODRlMTM1N2FhNTA4YWIy ZWVjZWE3MzI1Nzc4YWNiZTYzYy4uMGE2ZjEwNzgyZGMyZWI5NDJhMGJjMDk2Yjg0Yzc1YWJkMjkz OGZmOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckJ1dHRvbi5o CisrKyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJCdXR0b24uaApAQCAtNzAsOCAr NzAsOCBAQCBwcml2YXRlOgogCiAgICAgYm9vbCBpc0ZsZXhpYmxlQm94SW1wbCgpIGNvbnN0IG92 ZXJyaWRlIHsgcmV0dXJuIHRydWU7IH0KIAotICAgIFJlbmRlclRleHRGcmFnbWVudCogbV9idXR0 b25UZXh0OwotICAgIFJlbmRlckJsb2NrKiBtX2lubmVyOworICAgIFdlYWtQdHI8UmVuZGVyVGV4 dEZyYWdtZW50PiBtX2J1dHRvblRleHQ7CisgICAgV2Vha1B0cjxSZW5kZXJCbG9jaz4gbV9pbm5l cjsKIH07CiAKIH0gLy8gbmFtZXNwYWNlIFdlYkNvcmUK