Bug 41727 - More OwnPtr work, including making clear set the pointer to 0 before deletion
Summary: More OwnPtr work, including making clear set the pointer to 0 before deletion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-06 18:09 PDT by Darin Adler
Modified: 2010-07-07 10:05 PDT (History)
0 users

See Also:


Attachments
Patch (48.27 KB, patch)
2010-07-06 18:16 PDT, Darin Adler
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2010-07-06 18:09:01 PDT
More OwnPtr work, including making clear set the pointer to 0 before deltion
Comment 1 Darin Adler 2010-07-06 18:16:51 PDT
Created attachment 60669 [details]
Patch
Comment 2 Adam Barth 2010-07-07 02:57:58 PDT
Comment on attachment 60669 [details]
Patch

Glad to see this work continue.  Please consider landing this patch in two pieces as described below.

JavaScriptCore/wtf/OwnArrayPtr.h:72
 +      template<typename T> inline void OwnArrayPtr<T>::clear()
This change to clear() seems independent of deploying adoptPtr.  Why are we doing them in the same patch?  It's a bit hard to foresee the consequences of this change.  It seems like we'd be better off doing that in a separate patch.

WebCore/css/CSSSelector.h:311
 +              m_data.m_rareData = new RareData(adoptPtr(m_data.m_tagHistory));
This adoptPtr is unfortunately far from the new...  Why doesn't the "new RareData" have an adoptPtr?
Comment 3 Darin Adler 2010-07-07 09:03:46 PDT
(In reply to comment #2)
> JavaScriptCore/wtf/OwnArrayPtr.h:72
>  +      template<typename T> inline void OwnArrayPtr<T>::clear()
> This change to clear() seems independent of deploying adoptPtr.  Why are we doing them in the same patch?  It's a bit hard to foresee the consequences of this change.  It seems like we'd be better off doing that in a separate patch.

Yes, OK. I’ll land these separately.

> WebCore/css/CSSSelector.h:311
>  +              m_data.m_rareData = new RareData(adoptPtr(m_data.m_tagHistory));
> This adoptPtr is unfortunately far from the new...  Why doesn't the "new RareData" have an adoptPtr?

The answer to both questions is the same. The m_data object is a union and so has raw pointers in it rather than smart pointers. Thus, we need to leak the pointers on the way in, and adopt then pointers on the way out. There are probably multiple ways to refactor this so it's cleaner, but none immediately come to mind.
Comment 4 Darin Adler 2010-07-07 09:54:08 PDT
Committed r62677: <http://trac.webkit.org/changeset/62677>
Comment 5 Darin Adler 2010-07-07 10:05:46 PDT
The clear change is in <http://trac.webkit.org/changeset/62674>.