WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
153401
Destructor for ResourceHandleCFURLConnectionDelegate should clear m_handle
https://bugs.webkit.org/show_bug.cgi?id=153401
Summary
Destructor for ResourceHandleCFURLConnectionDelegate should clear m_handle
David Kilzer (:ddkilzer)
Reported
2016-01-23 10:38:53 PST
The destructor for ResourceHandleCFURLConnectionDelegate should clear m_handle by calling releaseHandle().
Attachments
Patch v1
(1.54 KB, patch)
2016-01-23 10:54 PST
,
David Kilzer (:ddkilzer)
darin
: review-
darin
: commit-queue-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
David Kilzer (:ddkilzer)
Comment 1
2016-01-23 10:54:42 PST
Created
attachment 269675
[details]
Patch v1
Alexey Proskuryakov
Comment 2
2016-01-23 11:54:34 PST
Why is this necessary, does this change fix something?
David Kilzer (:ddkilzer)
Comment 3
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.
Darin Adler
Comment 4
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.
Darin Adler
Comment 5
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.
Darin Adler
Comment 6
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
David Kilzer (:ddkilzer)
Comment 7
2016-01-25 11:05:45 PST
Closing.
David Kilzer (:ddkilzer)
Comment 8
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug