[oilpan] Move IDBDatabase, IDBDatabaseCallbacks, IDBDatabaseBackendInterface and other related clas…

[oilpan] Move IDBDatabase, IDBDatabaseCallbacks, IDBDatabaseBackendInterface and other related classes

R=ager@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163072

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163086

[haraken, 2013-11-21 17:28:35]

Now it compiles, but I don't yet fully understand lifetime relationships.
Probably Persistents that I introduced in this CL might cause leaks. I'll look
into the issue next week.

Not yet for review.

[haraken, 2013-11-26 07:26:02]

https://codereview.chromium.org/78053006/diff/70001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/IDBCallbacksProxy.h (right):

https://codereview.chromium.org/78053006/diff/70001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/IDBCallbacksProxy.h:67:
WebCore::Persistent<IDBDatabaseCallbacksProxy> m_databaseCallbacks;

Help me debug when you have time.

I'm hitting the following crash when creating an IDBCallbacksProxy object. This
looks strange because I already added  REQUIRE_THREADSAFE_HANDLES for
IDBDatabaseCallbacksProxy and IDBDatabaseCallbacks. Am I missing something?


crash log for DumpRenderTree (pid 17547):
STDOUT: <empty>
STDERR: ASSERTION FAILED: isAnyThreadInGC() || (m_thread == currentThread())
STDERR: ../../third_party/WebKit/Source/heap/ThreadState.h(200) : void
WebCore::ThreadState::checkThread() const
STDERR: 1   0x4496ae
STDERR: 2   0x4492e9
STDERR: 3   0x2b63abf
STDERR: 4   0x2b63325
STDERR: 5   0x2b626fc
STDERR: 6   0x2b62683
STDERR: 7   0x2b5ab41
STDERR: 8   0x1208f95
STDERR: 9   0x5418b3
STDERR: 10  0x1f1d24b
STDERR: 11  0x19d506e
STDERR: 12  0x19d4635
STDERR: 13  0x99fc31
STDERR: 14  0x99f803
STDERR: 15  0x998f5c
STDERR: 16  0x3dd9aa00616e
STDERR: Received signal 11 SEGV_MAPERR 0000bbadbeef
STDERR:  [0x000002def7de] base::debug::StackTrace::StackTrace()
STDERR:  [0x000002def317] base::debug::(anonymous
namespace)::StackDumpSignalHandler()
STDERR:  [0x7f30b0012cb0] <unknown>
STDERR:  [0x0000004496bd] WebCore::ThreadState::checkThread()
STDERR:  [0x0000004492e9] WebCore::PersistentBase<>::PersistentBase()
STDERR:  [0x000002b63abf] WebCore::Persistent<>::Persistent()
STDERR:  [0x000002b63325] WebCore::Persistent<>::Persistent()
STDERR:  [0x000002b626fc] WebKit::IDBCallbacksProxy::IDBCallbacksProxy()
STDERR:  [0x000002b62683] WebKit::IDBCallbacksProxy::create()
STDERR:  [0x000002b5ab41] WebKit::WebIDBFactoryImpl::deleteDatabase()
STDERR:  [0x000001208f95] TestWebIDBFactory::deleteDatabase()
STDERR:  [0x0000005418b3] WebKit::IDBFactoryBackendProxy::deleteDatabase()
STDERR:  [0x000001f1d24b] WebCore::IDBFactory::deleteDatabase()
STDERR:  [0x0000019d506e] WebCore::IDBFactoryV8Internal::deleteDatabaseMethod()
STDERR:  [0x0000019d4635]
WebCore::IDBFactoryV8Internal::deleteDatabaseMethodCallback()
STDERR:  [0x00000099fc31] v8::internal::HandleApiCallHelper<>()
STDERR:  [0x00000099f803] v8::internal::Builtin_Impl_HandleApiCall()
STDERR:  [0x000000998f5c] v8::internal::Builtin_HandleApiCall()
STDERR:  [0x3dd9aa00616e] <unknown>
STDERR:   r8: 00007f30a4ccc700  r9: 00007f30b511e0d0 r10: 0000000000000001 r11:
0000000000000000
STDERR:  r12: 0000000100000000 r13: 00000fe59b94b0b8 r14: 00000000041e2ace r15:
00007f30a4ccafd0
STDERR:   di: 0000000000000000  si: 00000000efcdab90  bp: 00007f30a4cca790  bx:
0000000000000008
STDERR:   dx: 00007f30ad9cfaa0  ax: 0000000000000010  cx: 00000000bbadbeef  sp:
00007f30a4cca770
STDERR:   ip: 00000000004496bd efl: 0000000000010246 cgf: 0000000000000033 erf:
0000000000000006
STDERR:  trp: 000000000000000e msk: 0000000000000000 cr2: 00000000bbadbeef

[haraken, 2013-11-26 07:44:45]

On 2013/11/26 07:26:02, haraken wrote:
>
https://codereview.chromium.org/78053006/diff/70001/Source/WebKit/chromium/sr...
> File Source/WebKit/chromium/src/IDBCallbacksProxy.h (right):
> 
>
https://codereview.chromium.org/78053006/diff/70001/Source/WebKit/chromium/sr...
> Source/WebKit/chromium/src/IDBCallbacksProxy.h:67:
> WebCore::Persistent<IDBDatabaseCallbacksProxy> m_databaseCallbacks;
> 
> Help me debug when you have time.
> 
> I'm hitting the following crash when creating an IDBCallbacksProxy object.
This
> looks strange because I already added  REQUIRE_THREADSAFE_HANDLES for
> IDBDatabaseCallbacksProxy and IDBDatabaseCallbacks. Am I missing something?
> 
> 
> crash log for DumpRenderTree (pid 17547):
> STDOUT: <empty>
> STDERR: ASSERTION FAILED: isAnyThreadInGC() || (m_thread == currentThread())
> STDERR: ../../third_party/WebKit/Source/heap/ThreadState.h(200) : void
> WebCore::ThreadState::checkThread() const
> STDERR: 1   0x4496ae
> STDERR: 2   0x4492e9
> STDERR: 3   0x2b63abf
> STDERR: 4   0x2b63325
> STDERR: 5   0x2b626fc
> STDERR: 6   0x2b62683
> STDERR: 7   0x2b5ab41
> STDERR: 8   0x1208f95
> STDERR: 9   0x5418b3
> STDERR: 10  0x1f1d24b
> STDERR: 11  0x19d506e
> STDERR: 12  0x19d4635
> STDERR: 13  0x99fc31
> STDERR: 14  0x99f803
> STDERR: 15  0x998f5c
> STDERR: 16  0x3dd9aa00616e
> STDERR: Received signal 11 SEGV_MAPERR 0000bbadbeef
> STDERR:  [0x000002def7de] base::debug::StackTrace::StackTrace()
> STDERR:  [0x000002def317] base::debug::(anonymous
> namespace)::StackDumpSignalHandler()
> STDERR:  [0x7f30b0012cb0] <unknown>
> STDERR:  [0x0000004496bd] WebCore::ThreadState::checkThread()
> STDERR:  [0x0000004492e9] WebCore::PersistentBase<>::PersistentBase()
> STDERR:  [0x000002b63abf] WebCore::Persistent<>::Persistent()
> STDERR:  [0x000002b63325] WebCore::Persistent<>::Persistent()
> STDERR:  [0x000002b626fc] WebKit::IDBCallbacksProxy::IDBCallbacksProxy()
> STDERR:  [0x000002b62683] WebKit::IDBCallbacksProxy::create()
> STDERR:  [0x000002b5ab41] WebKit::WebIDBFactoryImpl::deleteDatabase()
> STDERR:  [0x000001208f95] TestWebIDBFactory::deleteDatabase()
> STDERR:  [0x0000005418b3] WebKit::IDBFactoryBackendProxy::deleteDatabase()
> STDERR:  [0x000001f1d24b] WebCore::IDBFactory::deleteDatabase()
> STDERR:  [0x0000019d506e]
WebCore::IDBFactoryV8Internal::deleteDatabaseMethod()
> STDERR:  [0x0000019d4635]
> WebCore::IDBFactoryV8Internal::deleteDatabaseMethodCallback()
> STDERR:  [0x00000099fc31] v8::internal::HandleApiCallHelper<>()
> STDERR:  [0x00000099f803] v8::internal::Builtin_Impl_HandleApiCall()
> STDERR:  [0x000000998f5c] v8::internal::Builtin_HandleApiCall()
> STDERR:  [0x3dd9aa00616e] <unknown>
> STDERR:   r8: 00007f30a4ccc700  r9: 00007f30b511e0d0 r10: 0000000000000001
r11:
> 0000000000000000
> STDERR:  r12: 0000000100000000 r13: 00000fe59b94b0b8 r14: 00000000041e2ace
r15:
> 00007f30a4ccafd0
> STDERR:   di: 0000000000000000  si: 00000000efcdab90  bp: 00007f30a4cca790 
bx:
> 0000000000000008
> STDERR:   dx: 00007f30ad9cfaa0  ax: 0000000000000010  cx: 00000000bbadbeef 
sp:
> 00007f30a4cca770
> STDERR:   ip: 00000000004496bd efl: 0000000000010246 cgf: 0000000000000033
erf:
> 0000000000000006
> STDERR:  trp: 000000000000000e msk: 0000000000000000 cr2: 00000000bbadbeef

Note: I haven't yet looked into the issue in detail. If you cannot come up with
a reason in 5 mins, please just say so :)

[Vyacheslav Egorov (Google), 2013-11-26 08:44:26]

What happens here is that REQUIRE_THREADSAFE_HANDLES(T); declares T in the
current namespace as thread-safe. 

In our case IDBDatabaseCallbacksProxy is actually in WebKit namespace (not
webcore).

So you need something like:

namespace WebKit {
REQUIRE_THREADSAFE_HANDLES(IDBDatabaseCallbacksProxy);
} 

instead of putting REQUIRE_THREADSAFE_HANDLES(IDBDatabaseCallbacksProxy); into
WebCore namespace.

[haraken, 2013-11-26 10:45:07]

On 2013/11/26 08:44:26, Vyacheslav Egorov (Google) wrote:
> What happens here is that REQUIRE_THREADSAFE_HANDLES(T); declares T in the
> current namespace as thread-safe. 
> 
> In our case IDBDatabaseCallbacksProxy is actually in WebKit namespace (not
> webcore).
> 
> So you need something like:
> 
> namespace WebKit {
> REQUIRE_THREADSAFE_HANDLES(IDBDatabaseCallbacksProxy);
> } 
> 
> instead of putting REQUIRE_THREADSAFE_HANDLES(IDBDatabaseCallbacksProxy); into
> WebCore namespace.

Thanks Slava! But it doesn't compile due to cross-namespace template
specialization...

namespace WebCore {
template<typename T>
struct TreadingTrait {
  static const ThreadAffinity Affinity = MainThreadOnly;
};
}

namespace WebKit {
template<>
struct TreadingTrait<class IDBDatabaseCallbacksProxy> {  // This doesn't compile
because we're trying to specialize templates defined in a different namespace.
  static const ThreadAffinity Affinity = AnyThread;
};
}

Would you give me a hint?

[Vyacheslav Egorov (Google), 2013-11-26 12:05:49]

Indeed, I forgot about this limitation.

I think this should work:

#define REQUIRE_THREADSAFE_HANDLES_NS(Namespace, Class)  \
  namespace Namespace { class Class; }                   \
  namespace WebCore   {                                  \
    template<> struct ThreadingTrait<Namespace::Class> { \
      static const ThreadAffinity Affinity = AnyThread;  \
    };                                                   \
  }

And use it like this outside of WebCore namespace:

REQUIRE_THREADSAFE_HANDLES_NS(WebKit, IDBDatabaseCallbacksProxy);

[haraken, 2013-11-26 13:09:13]

Thanks Slava! It worked.

PTAL.

[haraken, 2013-11-28 01:44:19]

ping?

[Mads Ager (chromium), 2013-11-28 10:24:45]

There is one place where we replace a raw pointer with a Member. Have you drawn
the diagram of the pointer structure here to make sure that this will not create
a cycle involving persistent handles? It is not obvious to me. Replacing RefPtr
with Persistent should be safe, but as soon as we make a raw pointer a Member as
well it is very easy to get leaks.

https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/IDBCallbacksProxy.h (right):

https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/IDBCallbacksProxy.h:67:
WebCore::Persistent<IDBDatabaseCallbacksProxy> m_databaseCallbacks;
This will put a Persistent in our heap I think. This inherits from
WebCore::IDBCallbacks which is now in our heap. Can we implement the trace
method call chain and make this a Member?

https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebIDBCallbacksImpl.h (right):

https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebIDBCallbacksImpl.h:62:
RefPtr<WebCore::IDBCallbacks> m_callbacks;
This should be a persistent as well since we have moved IDBCallbacks to the
heap?

https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp (right):

https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:91: IDBDatabaseCallbacksProxy*
callbacks = m_databaseCallbacks;
Shouldn't this be m_databaseCallbacks.clear()?

https://codereview.chromium.org/78053006/diff/90001/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/78053006/diff/90001/Source/heap/ThreadState.h...
Source/heap/ThreadState.h:392: REQUIRE_THREADSAFE_HANDLES_WITH_NAMESPACE(WebKit,
IDBCallbacksProxy);
Everything above is just sorting, right?

https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBDatabaseBackendImpl.cpp (right):

https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBDatabaseBackendImpl.cpp:1389:
visitor->visit(m_factory);
These are now called |trace|.

https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBDatabaseBackendImpl.cpp:1390: for
(DatabaseCallbacksSet::const_iterator it = m_databaseCallbacksSet.begin(); it !=
m_databaseCallbacksSet.end(); ++it) {
We should extend the visitor to understand ListHashSet<Member<T> > in the same
way it understand HashSet<Member<T> >.

https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBDatabaseCallbacksImpl.h (right):

https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBDatabaseCallbacksImpl.h:57: Member<IDBDatabase>
m_database;
Here we are turning a raw (and therefore weak) pointer into a strong pointer
that keeps the database alive. I wonder if that can create a cycle involving a
persistents. In this changelist we have heap allocated objects that have ownptrs
to objects that contain persistents. So far I have only seen persistents used
where we used to have RefPtr, so that should be safe. However, this change from
raw to strong pointer might change that and create a cycle with Persistents in
it that cannot be reclaimed. We should draw a diagram of everything that points
to IDBDatabaseCallbacksImpl objects to see if such a cycle can occur.

[haraken, 2013-12-02 09:13:26]

Thanks for review. I noticed that I have to move IDBRequest to the heap at the
same time (for some reason). Let me reupload the CL after moving the IDLRequest
hierarchy.

https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/IDBCallbacksProxy.h (right):

https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/IDBCallbacksProxy.h:67:
WebCore::Persistent<IDBDatabaseCallbacksProxy> m_databaseCallbacks;
On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> This will put a Persistent in our heap I think. This inherits from
> WebCore::IDBCallbacks which is now in our heap. Can we implement the trace
> method call chain and make this a Member?

Done.

https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebIDBCallbacksImpl.h (right):

https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebIDBCallbacksImpl.h:62:
RefPtr<WebCore::IDBCallbacks> m_callbacks;
On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> This should be a persistent as well since we have moved IDBCallbacks to the
> heap?

Done.

https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp (right):

https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:91: IDBDatabaseCallbacksProxy*
callbacks = m_databaseCallbacks;
On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> Shouldn't this be m_databaseCallbacks.clear()?

Nice catch. Done.

https://codereview.chromium.org/78053006/diff/90001/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/78053006/diff/90001/Source/heap/ThreadState.h...
Source/heap/ThreadState.h:392: REQUIRE_THREADSAFE_HANDLES_WITH_NAMESPACE(WebKit,
IDBCallbacksProxy);
On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> Everything above is just sorting, right?

Yes.

https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBDatabaseBackendImpl.cpp (right):

https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBDatabaseBackendImpl.cpp:1389:
visitor->visit(m_factory);
On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> These are now called |trace|.

Done.

https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBDatabaseBackendImpl.cpp:1390: for
(DatabaseCallbacksSet::const_iterator it = m_databaseCallbacksSet.begin(); it !=
m_databaseCallbacksSet.end(); ++it) {
On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> We should extend the visitor to understand ListHashSet<Member<T> > in the same
> way it understand HashSet<Member<T> >.

Done. Please double-check a change to Visitor.h :)

https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBDatabaseCallbacksImpl.h (right):

https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBDatabaseCallbacksImpl.h:57: Member<IDBDatabase>
m_database;
On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> Here we are turning a raw (and therefore weak) pointer into a strong pointer
> that keeps the database alive. I wonder if that can create a cycle involving a
> persistents. In this changelist we have heap allocated objects that have
ownptrs
> to objects that contain persistents. So far I have only seen persistents used
> where we used to have RefPtr, so that should be safe. However, this change
from
> raw to strong pointer might change that and create a cycle with Persistents in
> it that cannot be reclaimed. We should draw a diagram of everything that
points
> to IDBDatabaseCallbacksImpl objects to see if such a cycle can occur.

Good point. I scanned the code base but it's unclear to me if this causes a
cycle or not. It's even harder to make sure that this won't cause a cycle in
future CLs as a side effect.

Probably would the following strategy be safe for our incremental replacement?

(1) Keep raw pointers as is.
(2) Once we move the entire hierarchy to the heap, we change the raw pointers
with Members.

[haraken, 2013-12-02 09:13:55]

On 2013/12/02 09:13:26, haraken wrote:
> Thanks for review. I noticed that I have to move IDBRequest to the heap at the
> same time (for some reason). Let me reupload the CL after moving the
IDLRequest
> hierarchy.

I mean, the CL is not yet for review.

[Mads Ager (chromium), 2013-12-02 09:16:59]

On 2013/12/02 09:13:26, haraken wrote:
> Thanks for review. I noticed that I have to move IDBRequest to the heap at the
> same time (for some reason). Let me reupload the CL after moving the
IDLRequest
> hierarchy.
> 
>
https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
> File Source/WebKit/chromium/src/IDBCallbacksProxy.h (right):
> 
>
https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
> Source/WebKit/chromium/src/IDBCallbacksProxy.h:67:
> WebCore::Persistent<IDBDatabaseCallbacksProxy> m_databaseCallbacks;
> On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> > This will put a Persistent in our heap I think. This inherits from
> > WebCore::IDBCallbacks which is now in our heap. Can we implement the trace
> > method call chain and make this a Member?
> 
> Done.
> 
>
https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
> File Source/WebKit/chromium/src/WebIDBCallbacksImpl.h (right):
> 
>
https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
> Source/WebKit/chromium/src/WebIDBCallbacksImpl.h:62:
> RefPtr<WebCore::IDBCallbacks> m_callbacks;
> On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> > This should be a persistent as well since we have moved IDBCallbacks to the
> > heap?
> 
> Done.
> 
>
https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
> File Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp (right):
> 
>
https://codereview.chromium.org/78053006/diff/90001/Source/WebKit/chromium/sr...
> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:91:
IDBDatabaseCallbacksProxy*
> callbacks = m_databaseCallbacks;
> On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> > Shouldn't this be m_databaseCallbacks.clear()?
> 
> Nice catch. Done.
> 
> https://codereview.chromium.org/78053006/diff/90001/Source/heap/ThreadState.h
> File Source/heap/ThreadState.h (right):
> 
>
https://codereview.chromium.org/78053006/diff/90001/Source/heap/ThreadState.h...
> Source/heap/ThreadState.h:392:
REQUIRE_THREADSAFE_HANDLES_WITH_NAMESPACE(WebKit,
> IDBCallbacksProxy);
> On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> > Everything above is just sorting, right?
> 
> Yes.
> 
>
https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
> File Source/modules/indexeddb/IDBDatabaseBackendImpl.cpp (right):
> 
>
https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
> Source/modules/indexeddb/IDBDatabaseBackendImpl.cpp:1389:
> visitor->visit(m_factory);
> On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> > These are now called |trace|.
> 
> Done.
> 
>
https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
> Source/modules/indexeddb/IDBDatabaseBackendImpl.cpp:1390: for
> (DatabaseCallbacksSet::const_iterator it = m_databaseCallbacksSet.begin(); it
!=
> m_databaseCallbacksSet.end(); ++it) {
> On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> > We should extend the visitor to understand ListHashSet<Member<T> > in the
same
> > way it understand HashSet<Member<T> >.
> 
> Done. Please double-check a change to Visitor.h :)
> 
>
https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
> File Source/modules/indexeddb/IDBDatabaseCallbacksImpl.h (right):
> 
>
https://codereview.chromium.org/78053006/diff/90001/Source/modules/indexeddb/...
> Source/modules/indexeddb/IDBDatabaseCallbacksImpl.h:57: Member<IDBDatabase>
> m_database;
> On 2013/11/28 10:24:46, Mads Ager (chromium) wrote:
> > Here we are turning a raw (and therefore weak) pointer into a strong pointer
> > that keeps the database alive. I wonder if that can create a cycle involving
a
> > persistents. In this changelist we have heap allocated objects that have
> ownptrs
> > to objects that contain persistents. So far I have only seen persistents
used
> > where we used to have RefPtr, so that should be safe. However, this change
> from
> > raw to strong pointer might change that and create a cycle with Persistents
in
> > it that cannot be reclaimed. We should draw a diagram of everything that
> points
> > to IDBDatabaseCallbacksImpl objects to see if such a cycle can occur.
> 
> Good point. I scanned the code base but it's unclear to me if this causes a
> cycle or not. It's even harder to make sure that this won't cause a cycle in
> future CLs as a side effect.
> 
> Probably would the following strategy be safe for our incremental replacement?
> 
> (1) Keep raw pointers as is.
> (2) Once we move the entire hierarchy to the heap, we change the raw pointers
> with Members.

Yes, that should work. We just need to remember to revisit all the raw pointers
at the end so that we get the actual benefits of oilpan in terms of avoiding
dangling references.

[haraken, 2013-12-03 05:29:48]

I moved IDBRequest and IDBOpenDBRequest at the same time since they inherit from
IDBCallbacks.

PTAL. I'm sorry about the big CL.

[Mads Ager (chromium), 2013-12-03 08:43:32]

LGTM

https://codereview.chromium.org/78053006/diff/110001/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/78053006/diff/110001/Source/heap/ThreadState....
Source/heap/ThreadState.h:68: namespace Namespace {                             
       \
Could you align the '\'s at the end of the line.

[haraken, 2013-12-03 08:48:54]

Thanks for review!

https://codereview.chromium.org/78053006/diff/110001/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/78053006/diff/110001/Source/heap/ThreadState....
Source/heap/ThreadState.h:68: namespace Namespace {                             
       \
On 2013/12/03 08:43:33, Mads Ager (chromium) wrote:
> Could you align the '\'s at the end of the line.

Done.

[haraken, 2013-12-03 08:49:13]

Committed patchset #6 manually as r163072.

[haraken, 2013-12-03 13:40:59]

Sorry about the build breakage for webkit_unit_tests. Fixed the build.
Relanding.

[haraken, 2013-12-03 13:42:28]

Committed patchset #7 manually as r163086.

[haraken, 2013-12-04 04:48:48]

This CL caused a crash of storage/indexeddb/objectstore-basics.html and
storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange.html.
Looking.