Bug 153401

Summary: Destructor for ResourceHandleCFURLConnectionDelegate should clear m_handle
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: Page LoadingAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED WONTFIX    
Severity: Normal CC: aestes, ap, beidson, darin, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch v1 darin: review-, darin: commit-queue-

Description David Kilzer (:ddkilzer) 2016-01-23 10:38:53 PST
The destructor for ResourceHandleCFURLConnectionDelegate should clear m_handle by calling releaseHandle().
Comment 1 David Kilzer (:ddkilzer) 2016-01-23 10:54:42 PST
Created attachment 269675 [details]
Patch v1
Comment 2 Alexey Proskuryakov 2016-01-23 11:54:34 PST
Why is this necessary, does this change fix something?
Comment 3 David Kilzer (:ddkilzer) 2016-01-23 12:14:19 PST
(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.
Comment 4 Darin Adler 2016-01-24 12:20:11 PST
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.
Comment 5 Darin Adler 2016-01-24 12:21:31 PST
(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.
Comment 6 Darin Adler 2016-01-24 12:22:56 PST
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
Comment 7 David Kilzer (:ddkilzer) 2016-01-25 11:05:45 PST
Closing.
Comment 8 David Kilzer (:ddkilzer) 2016-01-25 11:25:52 PST
(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.