Bug 153401 – Destructor for ResourceHandleCFURLConnectionDelegate should clear m_handle

• Component: Page Loading
• Status: RESOLVED WONTFIX

[David Kilzer (:ddkilzer)]

The destructor for ResourceHandleCFURLConnectionDelegate should clear m_handle by calling releaseHandle().

[David Kilzer (:ddkilzer)]

Created attachment 269675 [details]
Patch v1

[Alexey Proskuryakov]

Why is this necessary, does this change fix something?

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> Why is this necessary, does this change fix something?

I guess this only matters if ResourceHandleCFURLConnectionDelegate (or its derived classes) are used as bare pointers.  The only place I found this happening was in QuickLook.h with SynchronousResourceHandleCFURLConnectionDelegate:

    static std::unique_ptr<QuickLookHandle> create(ResourceHandle*, SynchronousResourceHandleCFURLConnectionDelegate*, CFURLResponseRef);

The pointer is later assigned to a RefPtr<> in the WebQuickLookHandleAsDelegate class.

This was a defensive change; it doesn't fix any known issue at this time.

[Darin Adler]

Comment on attachment 269675 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=269675&action=review

> Source/WebCore/ChangeLog:3
> +        Destructor for ResourceHandleCFURLConnectionDelegate should clear m_handle

Why? This should not be necessary under normal circumstances. I presume there is some reason you want to do this, and the patch should state the reason.

[Darin Adler]

(In reply to comment #3)
> (In reply to comment #2)
> > Why is this necessary, does this change fix something?
> 
> I guess this only matters if ResourceHandleCFURLConnectionDelegate (or its
> derived classes) are used as bare pointers.  The only place I found this
> happening was in QuickLook.h with
> SynchronousResourceHandleCFURLConnectionDelegate:
> 
>     static std::unique_ptr<QuickLookHandle> create(ResourceHandle*,
> SynchronousResourceHandleCFURLConnectionDelegate*, CFURLResponseRef);
> 
> The pointer is later assigned to a RefPtr<> in the
> WebQuickLookHandleAsDelegate class.
> 
> This was a defensive change; it doesn't fix any known issue at this time.

I still don’t understand. You say you found “this” happening, but I don’t know what “this” is.

[Darin Adler]

Comment on attachment 269675 [details]
Patch v1

review- for now; we may decide there is some value in doing this, but it’s normally not helpful to change the values of data members in an object that is being destroyed, since code can’t rely on looking at the data members values after the object is destroyed

[David Kilzer (:ddkilzer)]

Closing.

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> Comment on attachment 269675 [details]
> Patch v1
> 
> review- for now; we may decide there is some value in doing this, but it’s
> normally not helpful to change the values of data members in an object that
> is being destroyed, since code can’t rely on looking at the data members
> values after the object is destroyed

The general idea was to clear out pointer member variables on destruction in case other code was holding a pointer to the object (in the case where the memory for the current object was still reference-able, and got the old pointer value for the pointer member variable).  But because this object is RefCounted, there is little chance we'll ever hit this case, so there's no point in making this change.