Bug 59656 – Fix OwnPtr issues in IndexedDB

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Fix OwnPtr issues in IndexedDB

[Adam Barth]

Created attachment 91378 [details]
Patch

[David Levin]

Comment on attachment 91378 [details]
Patch

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

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:102
> +    OwnPtr<LevelDBDatabase> result = adoptPtr(new LevelDBDatabase());

Not required (cq+ is ok with me) but might want to remove the ().

[Eric Seidel (no email)]

Comment on attachment 91378 [details]
Patch

Yay!  I'm kinda surprised it got landed as it was.

[Eric Seidel (no email)]

In fareness to Steve, I think Darin's document is due for a refresh: http://www.webkit.org/coding/RefPtr.html.  Some of what seemed "obviously" wrong in the previous commit, it not actually that obvious of patterns of WebCore.  But we do need to be careful to get our memory management (and header includes) correct.

[Adam Barth]

Committed r85122: <http://trac.webkit.org/changeset/85122>

[WebKit Review Bot]

http://trac.webkit.org/changeset/85122 might have broken SnowLeopard Intel Release (Tests)

[David Grogan]

This busticated the clang build:

http://build.chromium.org/p/chromium.webkit/builders/Mac%20Clang%20Builder%20%28dbg%29/builds/6490/steps/compile/logs/stdio

In file included from /b/build/slave/Mac_Clang_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../platform/leveldb/LevelDBDatabase.cpp:27:
In file included from ../platform/leveldb/LevelDBDatabase.h:31:
../../JavaScriptCore/wtf/OwnPtr.h:57:9: error: function 'WTF::OwnPtr<WebCore::<anonymous>::ComparatorAdapter>::OwnPtr' has internal linkage but is not defined [-Werror]
        OwnPtr(const OwnPtr<ValueType>&);
        ^
/b/build/slave/Mac_Clang_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../platform/leveldb/LevelDBDatabase.cpp:100:31: note: used here
    OwnPtr<ComparatorAdapter> comparatorAdapter = adoptPtr(new ComparatorAdapter(comparator));
                              ^
1 error generated.

[Adam Barth]

> This busticated the clang build:

I have no idea what that error means.  That line of code is one of the most common patterns in WebKit.  Maybe it's a compiler bug?

[Adam Barth]

Maciej has a theory for how to fix this error.

[Adam Barth]

Created attachment 91436 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 91436 [details]
Patch

r=me

[Adam Barth]

> r=me

Thanks!

[Adam Barth]

Committed r85169: <http://trac.webkit.org/changeset/85169>

[Steve Block]

Thanks for fixing this and apologies for missing the include problems in the original patch.

I wasn't aware that we should always use (Pass)OwnPtr every time ownership of an object is being transferred. What exactly is the policy? It would be great if the document could be updated for future reference.

[Adam Barth]

> I wasn't aware that we should always use (Pass)OwnPtr every time ownership of an object is being transferred. What exactly is the policy? It would be great if the document could be updated for future reference.

The policy is that we should try to avoid ever having a "naked new," which means every call to new should be immediately followed by either adoptRef (for ref-counted objects) or adoptPtr (for objects with a single owner).  Everything else follows from the C++ type system.

Turning on strict mode tightens the type system to better enforce this style.  Once we've got that all working, we'll probably add a check-webkit-style nag about "naked new" so that these problems can be flagged automagically.  (There's already an ASSERT that enforces this behavior for ref-counted objects.)