179844 2017-11-17 14:47:49 -0800 REGRESSION(r224390): Revert unneeded Ref use 2017-11-18 06:56:17 -0800 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 179218 1 bfulgham bfulgham bfulgham cdumez esprehn+autocc ews-watchlist gyuyoung.kim jiewen_tan rniwa webkit-bug-importer zalan oldest_to_newest 1373316 0 bfulgham 2017-11-17 14:47:49 -0800 In Bug 179218, raw auto* pointers were converted to RefPtrs using makeRefPtr. However, in one place it was accidentally typed "makeRef", leading to nullptr crashes. 1373319 1 webkit-bug-importer 2017-11-17 14:50:18 -0800 <rdar://problem/35624718> 1373322 2 327248 bfulgham 2017-11-17 14:53:04 -0800 Created attachment 327248 Patch 1373325 3 bfulgham 2017-11-17 14:54:04 -0800 It's not really clear to me what the benefit of the original change was, but this prevents it from crashing when contentDocument is nullptr. 1373328 4 327248 cdumez 2017-11-17 14:57:43 -0800 Comment on attachment 327248 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327248&action=review > Source/WebCore/html/HTMLFrameOwnerElement.cpp:117 > + auto document = makeRef(*contentDocument); Why do we even want a Refptr here? This seems to cause churn for no reason. 1373329 5 327248 cdumez 2017-11-17 14:59:03 -0800 Comment on attachment 327248 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327248&action=review >> Source/WebCore/html/HTMLFrameOwnerElement.cpp:117 >> + auto document = makeRef(*contentDocument); > > Why do we even want a Refptr here? This seems to cause churn for no reason. I suggest: auto* document = contentDocument(); if (is<SVGDocument>(document)) return *document; return Exception { NotSupportedError }; 1373333 6 327248 cdumez 2017-11-17 15:01:12 -0800 Comment on attachment 327248 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327248&action=review >>> Source/WebCore/html/HTMLFrameOwnerElement.cpp:117 >>> + auto document = makeRef(*contentDocument); >> >> Why do we even want a Refptr here? This seems to cause churn for no reason. > > I suggest: > auto* document = contentDocument(); > if (is<SVGDocument>(document)) > return *document; > return Exception { NotSupportedError }; If you insist on using a RefPtr for some reason, then I'd suggest: auto document = makeRefPtr(contentDocument()); if (is<SVGDocument>(document)) return *document; return Exception { NotSupportedError }; But honestly, it is really not useful to hold a strong reference just to do a type check IMO. 1373339 7 327248 jiewen_tan 2017-11-17 15:07:49 -0800 Comment on attachment 327248 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327248&action=review >>>> Source/WebCore/html/HTMLFrameOwnerElement.cpp:117 >>>> + auto document = makeRef(*contentDocument); >>> >>> Why do we even want a Refptr here? This seems to cause churn for no reason. >> >> I suggest: >> auto* document = contentDocument(); >> if (is<SVGDocument>(document)) >> return *document; >> return Exception { NotSupportedError }; > > If you insist on using a RefPtr for some reason, then I'd suggest: > auto document = makeRefPtr(contentDocument()); > if (is<SVGDocument>(document)) > return *document; > return Exception { NotSupportedError }; > > But honestly, it is really not useful to hold a strong reference just to do a type check IMO. My bad. I agree it is not necessary to do the RefPtr thing in this particular spot. 1373341 8 327248 jiewen_tan 2017-11-17 15:09:41 -0800 Comment on attachment 327248 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327248&action=review >>>>> Source/WebCore/html/HTMLFrameOwnerElement.cpp:117 >>>>> + auto document = makeRef(*contentDocument); >>>> >>>> Why do we even want a Refptr here? This seems to cause churn for no reason. >>> >>> I suggest: >>> auto* document = contentDocument(); >>> if (is<SVGDocument>(document)) >>> return *document; >>> return Exception { NotSupportedError }; >> >> If you insist on using a RefPtr for some reason, then I'd suggest: >> auto document = makeRefPtr(contentDocument()); >> if (is<SVGDocument>(document)) >> return *document; >> return Exception { NotSupportedError }; >> >> But honestly, it is really not useful to hold a strong reference just to do a type check IMO. > > My bad. I agree it is not necessary to do the RefPtr thing in this particular spot. I suggest we could just revert my change in this particular spot. 1373352 9 327248 bfulgham 2017-11-17 15:23:26 -0800 Comment on attachment 327248 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327248&action=review >>>>>> Source/WebCore/html/HTMLFrameOwnerElement.cpp:117 >>>>>> + auto document = makeRef(*contentDocument); >>>>> >>>>> Why do we even want a Refptr here? This seems to cause churn for no reason. >>>> >>>> I suggest: >>>> auto* document = contentDocument(); >>>> if (is<SVGDocument>(document)) >>>> return *document; >>>> return Exception { NotSupportedError }; >>> >>> If you insist on using a RefPtr for some reason, then I'd suggest: >>> auto document = makeRefPtr(contentDocument()); >>> if (is<SVGDocument>(document)) >>> return *document; >>> return Exception { NotSupportedError }; >>> >>> But honestly, it is really not useful to hold a strong reference just to do a type check IMO. >> >> My bad. I agree it is not necessary to do the RefPtr thing in this particular spot. > > I suggest we could just revert my change in this particular spot. Cool. I'll land a revert. 1373353 10 327253 bfulgham 2017-11-17 15:26:34 -0800 Created attachment 327253 Patch 1373354 11 327253 cdumez 2017-11-17 15:27:53 -0800 Comment on attachment 327253 Patch r=me 1373358 12 bfulgham 2017-11-17 15:34:11 -0800 Committed r224995: <https://trac.webkit.org/changeset/224995> 1373444 13 zalan 2017-11-17 17:11:21 -0800 *** Bug 179854 has been marked as a duplicate of this bug. *** 1373522 14 rniwa 2017-11-18 01:56:07 -0800 I think Zalan had a repro for the crash. Could someone add a test case so that we don't introduce the same bug in the future? 1373546 15 zalan 2017-11-18 06:56:17 -0800 (In reply to Ryosuke Niwa from comment #14) > I think Zalan had a repro for the crash. Could someone add a test case so > that we don't introduce the same bug in the future? I already did; see https://trac.webkit.org/changeset/225012/webkit 327248 2017-11-17 14:53:04 -0800 2017-11-17 15:26:32 -0800 Patch bug-179844-20171117145304.patch text/plain 2244 bfulgham SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIyNDk4OCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDE3LTExLTE3ICBCcmVudCBG dWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFJFR1JFU1NJT04ocjIyNDM5 MCk6IENvcnJlY3QgUmVmIC0+IFJlZlB0ciAKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTE3OTg0NAorICAgICAgICA8cmRhcjovL3Byb2JsZW0vMzU2MjQ3 MTg+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBo dG1sL0hUTUxGcmFtZU93bmVyRWxlbWVudC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpIVE1MRnJh bWVPd25lckVsZW1lbnQ6OmNvbnRlbnREb2N1bWVudCBjb25zdCk6IFVzZSAnbnVsbHB0cicgaW5z dGVhZCBvZiAwLgorICAgICAgICAoV2ViQ29yZTo6SFRNTEZyYW1lT3duZXJFbGVtZW50Ojpjb250 ZW50V2luZG93IGNvbnN0KTogRGl0dG8uCisgICAgICAgIChXZWJDb3JlOjpIVE1MRnJhbWVPd25l ckVsZW1lbnQ6OmdldFNWR0RvY3VtZW50IGNvbnN0KTogY29udGVudERvY3VtZW50KCkgbWF5IHJl dHVybiBudWxscHRyLAorICAgICAgICBzbyBoYW5kbGUgdGhhdCBncmFjZWZ1bGx5LgorCiAyMDE3 LTExLTE3ICBBbGV4IENocmlzdGVuc2VuICA8YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+CiAKICAg ICAgICAgQ2xlYW4gdXAgVVJMLmgKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTEZyYW1l T3duZXJFbGVtZW50LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxG cmFtZU93bmVyRWxlbWVudC5jcHAJKHJldmlzaW9uIDIyNDg5NCkKKysrIFNvdXJjZS9XZWJDb3Jl L2h0bWwvSFRNTEZyYW1lT3duZXJFbGVtZW50LmNwcAkod29ya2luZyBjb3B5KQpAQCAtOTMsMTIg KzkzLDEyIEBAIEhUTUxGcmFtZU93bmVyRWxlbWVudDo6fkhUTUxGcmFtZU93bmVyRWwKIAogRG9j dW1lbnQqIEhUTUxGcmFtZU93bmVyRWxlbWVudDo6Y29udGVudERvY3VtZW50KCkgY29uc3QKIHsK LSAgICByZXR1cm4gbV9jb250ZW50RnJhbWUgPyBtX2NvbnRlbnRGcmFtZS0+ZG9jdW1lbnQoKSA6 IDA7CisgICAgcmV0dXJuIG1fY29udGVudEZyYW1lID8gbV9jb250ZW50RnJhbWUtPmRvY3VtZW50 KCkgOiBudWxscHRyOwogfQogCiBET01XaW5kb3cqIEhUTUxGcmFtZU93bmVyRWxlbWVudDo6Y29u dGVudFdpbmRvdygpIGNvbnN0CiB7Ci0gICAgcmV0dXJuIG1fY29udGVudEZyYW1lID8gbV9jb250 ZW50RnJhbWUtPmRvY3VtZW50KCktPmRvbVdpbmRvdygpIDogMDsKKyAgICByZXR1cm4gbV9jb250 ZW50RnJhbWUgPyBtX2NvbnRlbnRGcmFtZS0+ZG9jdW1lbnQoKS0+ZG9tV2luZG93KCkgOiBudWxs cHRyOwogfQogCiB2b2lkIEhUTUxGcmFtZU93bmVyRWxlbWVudDo6c2V0U2FuZGJveEZsYWdzKFNh bmRib3hGbGFncyBmbGFncykKQEAgLTExMyw5ICsxMTMsMTEgQEAgYm9vbCBIVE1MRnJhbWVPd25l ckVsZW1lbnQ6OmlzS2V5Ym9hcmRGbwogCiBFeGNlcHRpb25PcjxEb2N1bWVudCY+IEhUTUxGcmFt ZU93bmVyRWxlbWVudDo6Z2V0U1ZHRG9jdW1lbnQoKSBjb25zdAogewotICAgIGF1dG8gZG9jdW1l bnQgPSBtYWtlUmVmKCpjb250ZW50RG9jdW1lbnQoKSk7Ci0gICAgaWYgKGlzPFNWR0RvY3VtZW50 Pihkb2N1bWVudCkpCi0gICAgICAgIHJldHVybiBkb2N1bWVudC5nZXQoKTsKKyAgICBpZiAoYXV0 byogY29udGVudERvY3VtZW50ID0gdGhpcy0+Y29udGVudERvY3VtZW50KCkpIHsKKyAgICAgICAg YXV0byBkb2N1bWVudCA9IG1ha2VSZWYoKmNvbnRlbnREb2N1bWVudCk7CisgICAgICAgIGlmIChp czxTVkdEb2N1bWVudD4oZG9jdW1lbnQpKQorICAgICAgICAgICAgcmV0dXJuIGRvY3VtZW50Lmdl dCgpOworICAgIH0KICAgICAvLyBTcGVjOiBodHRwOi8vd3d3LnczLm9yZy9UUi9TVkcvc3RydWN0 Lmh0bWwjSW50ZXJmYWNlR2V0U1ZHRG9jdW1lbnQKICAgICByZXR1cm4gRXhjZXB0aW9uIHsgTm90 U3VwcG9ydGVkRXJyb3IgfTsKIH0K 327253 2017-11-17 15:26:34 -0800 2017-11-17 15:27:53 -0800 Patch bug-179844-20171117152633.patch text/plain 2246 bfulgham SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIyNDk4OCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDE3LTExLTE3ICBCcmVudCBG dWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFJFR1JFU1NJT04ocjIyNDM5 MCk6IFJldmVydCB1bm5lZWRlZCBSZWYgdXNlLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTc5ODQ0CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8zNTYy NDcxOD4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBB IFJlZiB3YXMgYWRkZWQgdG8gYSBzaW1wbGUgY29kZSBwYXRoIHVzZWQgZm9yIHR5cGUgY2hlY2tp bmcsIHdoaWNoIGludHJvZHVjZWQgYSBudWxscHRyCisgICAgICAgIGRlcmVmLiBUaGUgb3JpZ2lu YWwgY2hhbmdlIHdhcyBub3QgbmVlZGVkLCBzbyBsZXQncyByZXZlcnQgaXQuCisKKyAgICAgICAg KiBodG1sL0hUTUxGcmFtZU93bmVyRWxlbWVudC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpIVE1M RnJhbWVPd25lckVsZW1lbnQ6OmNvbnRlbnREb2N1bWVudCBjb25zdCk6IFVzZSAnbnVsbHB0cicg aW5zdGVhZCBvZiAwLgorICAgICAgICAoV2ViQ29yZTo6SFRNTEZyYW1lT3duZXJFbGVtZW50Ojpj b250ZW50V2luZG93IGNvbnN0KTogRGl0dG8uCisgICAgICAgIChXZWJDb3JlOjpIVE1MRnJhbWVP d25lckVsZW1lbnQ6OmdldFNWR0RvY3VtZW50IGNvbnN0KTogUmV2ZXJ0IHRvIG9yaWdpbmFsIGNv ZGUgaGVyZS4KKwogMjAxNy0xMS0xNyAgQWxleCBDaHJpc3RlbnNlbiAgPGFjaHJpc3RlbnNlbkB3 ZWJraXQub3JnPgogCiAgICAgICAgIENsZWFuIHVwIFVSTC5oCkluZGV4OiBTb3VyY2UvV2ViQ29y ZS9odG1sL0hUTUxGcmFtZU93bmVyRWxlbWVudC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dl YkNvcmUvaHRtbC9IVE1MRnJhbWVPd25lckVsZW1lbnQuY3BwCShyZXZpc2lvbiAyMjQ4OTQpCisr KyBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxGcmFtZU93bmVyRWxlbWVudC5jcHAJKHdvcmtpbmcg Y29weSkKQEAgLTkzLDEyICs5MywxMiBAQCBIVE1MRnJhbWVPd25lckVsZW1lbnQ6On5IVE1MRnJh bWVPd25lckVsCiAKIERvY3VtZW50KiBIVE1MRnJhbWVPd25lckVsZW1lbnQ6OmNvbnRlbnREb2N1 bWVudCgpIGNvbnN0CiB7Ci0gICAgcmV0dXJuIG1fY29udGVudEZyYW1lID8gbV9jb250ZW50RnJh bWUtPmRvY3VtZW50KCkgOiAwOworICAgIHJldHVybiBtX2NvbnRlbnRGcmFtZSA/IG1fY29udGVu dEZyYW1lLT5kb2N1bWVudCgpIDogbnVsbHB0cjsKIH0KIAogRE9NV2luZG93KiBIVE1MRnJhbWVP d25lckVsZW1lbnQ6OmNvbnRlbnRXaW5kb3coKSBjb25zdAogewotICAgIHJldHVybiBtX2NvbnRl bnRGcmFtZSA/IG1fY29udGVudEZyYW1lLT5kb2N1bWVudCgpLT5kb21XaW5kb3coKSA6IDA7Cisg ICAgcmV0dXJuIG1fY29udGVudEZyYW1lID8gbV9jb250ZW50RnJhbWUtPmRvY3VtZW50KCktPmRv bVdpbmRvdygpIDogbnVsbHB0cjsKIH0KIAogdm9pZCBIVE1MRnJhbWVPd25lckVsZW1lbnQ6OnNl dFNhbmRib3hGbGFncyhTYW5kYm94RmxhZ3MgZmxhZ3MpCkBAIC0xMTMsOSArMTEzLDkgQEAgYm9v bCBIVE1MRnJhbWVPd25lckVsZW1lbnQ6OmlzS2V5Ym9hcmRGbwogCiBFeGNlcHRpb25PcjxEb2N1 bWVudCY+IEhUTUxGcmFtZU93bmVyRWxlbWVudDo6Z2V0U1ZHRG9jdW1lbnQoKSBjb25zdAogewot ICAgIGF1dG8gZG9jdW1lbnQgPSBtYWtlUmVmKCpjb250ZW50RG9jdW1lbnQoKSk7CisgICAgYXV0 byogZG9jdW1lbnQgPSBjb250ZW50RG9jdW1lbnQoKTsKICAgICBpZiAoaXM8U1ZHRG9jdW1lbnQ+ KGRvY3VtZW50KSkKLSAgICAgICAgcmV0dXJuIGRvY3VtZW50LmdldCgpOworICAgICAgICByZXR1 cm4gKmRvY3VtZW50OwogICAgIC8vIFNwZWM6IGh0dHA6Ly93d3cudzMub3JnL1RSL1NWRy9zdHJ1 Y3QuaHRtbCNJbnRlcmZhY2VHZXRTVkdEb2N1bWVudAogICAgIHJldHVybiBFeGNlcHRpb24geyBO b3RTdXBwb3J0ZWRFcnJvciB9OwogfQo=