Bug 179844 – REGRESSION(r224390): Revert unneeded Ref use

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/35624718>

[Brent Fulgham]

Created attachment 327248 [details]
Patch

[Brent Fulgham]

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.

[Chris Dumez]

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.

[Chris Dumez]

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 };

[Chris Dumez]

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.

[Jiewen Tan]

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.

[Jiewen Tan]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 327253 [details]
Patch

[Chris Dumez]

Comment on attachment 327253 [details]
Patch

r=me

[Brent Fulgham]

Committed r224995: <https://trac.webkit.org/changeset/224995>

[alan]

*** Bug 179854 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

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?

[alan]

(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