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.
<rdar://problem/35624718>
Created attachment 327248 [details] Patch
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.
Comment on attachment 327248 [details] 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.
Comment on attachment 327248 [details] 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 };
Comment on attachment 327248 [details] 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.
Comment on attachment 327248 [details] 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.
Comment on attachment 327248 [details] 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.
Comment on attachment 327248 [details] 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.
Created attachment 327253 [details] Patch
Comment on attachment 327253 [details] Patch r=me
Committed r224995: <https://trac.webkit.org/changeset/224995>
*** Bug 179854 has been marked as a duplicate of this bug. ***
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?
(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