Allow CrossThreadPersistent in collections (issue 2007283002 by keishi@chromium.org)

[kei...@chromium.org]

Reviewers: oilpan-reviews, haraken
CL: https://codereview.chromium.org/2007283002/

Message:
PTAL

Part of:

1. Add checks for per thread heap
https://codereview.chromium.org/2012763002/

2. This. Allow CrossThreadPersistent in collections
https://codereview.chromium.org/2007283002/

3. Enable per thread heap for database thread
https://codereview.chromium.org/1909813002/

Description:
Allow CrossThreadPersistent in collections

BUG=591606

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+62, -5 lines):
M third_party/WebKit/Source/platform/heap/HeapAllocator.h
M third_party/WebKit/Source/platform/heap/Persistent.h
M third_party/WebKit/Source/platform/heap/PersistentNode.cpp


Index: third_party/WebKit/Source/platform/heap/HeapAllocator.h
diff --git a/third_party/WebKit/Source/platform/heap/HeapAllocator.h b/third_party/WebKit/Source/platform/heap/HeapAllocator.h
index 40e0d381d90225e8c4bdae2271e9e72bf48ae07b..d6204e7fe75fd5e9453d5b83d2d6b8988956c799 100644
--- a/third_party/WebKit/Source/platform/heap/HeapAllocator.h
+++ b/third_party/WebKit/Source/platform/heap/HeapAllocator.h
@@ -422,6 +422,14 @@ template <typename T> struct VectorTraits<blink::UntracedMember<T>> : VectorTrai
static const bool canMoveWithMemcpy = true;
};

+template <typename T> struct VectorTraits<blink::CrossThreadPersistent<T>> : VectorTraitsBase<blink::CrossThreadPersistent<T>> {
+ STATIC_ONLY(VectorTraits);
+ static const bool needsDestruction = true;
+ static const bool canInitializeWithMemset = false;
+ static const bool canClearUnusedSlotsWithMemset = false;
+ static const bool canMoveWithMemcpy = false;
+};
+
template <typename T> struct VectorTraits<blink::HeapVector<T, 0>> : VectorTraitsBase<blink::HeapVector<T, 0>> {
STATIC_ONLY(VectorTraits);
static const bool needsDestruction = false;
@@ -551,6 +559,33 @@ struct IsGarbageCollectedType<ListHashSetNode<T, blink::HeapListHashSetAllocator
static const bool value = true;
};

+template<typename T> struct HashTraits<blink::CrossThreadPersistent<T>> : SimpleClassHashTraits<blink::CrossThreadPersistent<T>> {
+ STATIC_ONLY(HashTraits);
+ // FIXME: The distinction between PeekInType and PassInType is there for
+ // the sake of the reference counting handles. When they are gone the two
+ // types can be merged into PassInType.
+ // FIXME: Implement proper const'ness for iterator types. Requires support
+ // in the marking Visitor.
+ using PeekInType = T*;
+ using PassInType = T*;
+ using IteratorGetType = blink::Member<T>*;
+ using IteratorConstGetType = const blink::CrossThreadPersistent<T>*;
+ using IteratorReferenceType = blink::CrossThreadPersistent<T>&;
+ using IteratorConstReferenceType = const blink::CrossThreadPersistent<T>&;
+ static IteratorReferenceType getToReferenceConversion(IteratorGetType x) { return *x; }
+ static IteratorConstReferenceType getToReferenceConstConversion(IteratorConstGetType x) { return *x; }
+ // FIXME: Similarly, there is no need for a distinction between PeekOutType
+ // and PassOutType without reference counting.
+ using PeekOutType = T*;
+ using PassOutType = T*;
+
+ template<typename U>
+ static void store(const U& value, blink::CrossThreadPersistent<T>& storage) { storage = value; }
+
+ static PeekOutType peek(const blink::CrossThreadPersistent<T>& value) { return value; }
+ static PassOutType passOut(const blink::CrossThreadPersistent<T>& value) { return value; }
+};
+
} // namespace WTF

#endif
Index: third_party/WebKit/Source/platform/heap/Persistent.h
diff --git a/third_party/WebKit/Source/platform/heap/Persistent.h b/third_party/WebKit/Source/platform/heap/Persistent.h
index 8aa3708af2df13521feaaa734413965cde3ac299..64b03d3a5a5976412dda1f316530dca8c83291c0 100644
--- a/third_party/WebKit/Source/platform/heap/Persistent.h
+++ b/third_party/WebKit/Source/platform/heap/Persistent.h
@@ -77,12 +77,20 @@ public:
checkPointer();
}

+ PersistentBase(WTF::HashTableDeletedValueType) : m_raw(reinterpret_cast<T*>(-1))
+ {
+ initialize();
+ checkPointer();
+ }
+
~PersistentBase()
{
uninitialize();
m_raw = nullptr;
}

+ bool isHashTableDeletedValue() const { return m_raw == reinterpret_cast<T*>(-1); }
+
template<typename VisitorDispatcher>
void trace(VisitorDispatcher visitor)
{
@@ -184,7 +192,7 @@ private:
void initialize()
{
ASSERT(!m_persistentNode);
- if (!m_raw)
+ if (!m_raw || isHashTableDeletedValue())
return;

TraceCallback traceCallback = TraceMethodDelegate<PersistentBase<T, weaknessConfiguration, crossThreadnessConfiguration>, &PersistentBase<T, weaknessConfiguration, crossThreadnessConfiguration>::trace>::trampoline;
@@ -202,14 +210,15 @@ private:

void uninitialize()
{
+ if (!m_persistentNode)
+ return;
+
if (crossThreadnessConfiguration == CrossThreadPersistentConfiguration) {
if (acquireLoad(reinterpret_cast<void* volatile*>(&m_persistentNode)))
ProcessHeap::crossThreadPersistentRegion().freePersistentNode(m_persistentNode);
return;
}

- if (!m_persistentNode)
- return;
ThreadState* state = ThreadStateFor<ThreadingTrait<T>::Affinity>::state();
ASSERT(state->checkThread());
// Persistent handle must be created and destructed in the same thread.
@@ -221,7 +230,7 @@ private:
void checkPointer()
{
#if ENABLE(ASSERT) && defined(ADDRESS_SANITIZER)
- if (!m_raw)
+ if (!m_raw || isHashTableDeletedValue())
return;

// ThreadHeap::isHeapObjectAlive(m_raw) checks that m_raw is a traceable
@@ -372,6 +381,7 @@ public:
CrossThreadPersistent(const CrossThreadPersistent<U>& other) : Parent(other) { }
template<typename U>
CrossThreadPersistent(const Member<U>& other) : Parent(other) { }
+ CrossThreadPersistent(WTF::HashTableDeletedValueType x) : Parent(x) { }

T* atomicGet() { return Parent::atomicGet(); }

@@ -682,6 +692,17 @@ template<typename T, typename U> inline bool operator!=(const Persistent<T>& a,

namespace WTF {

+template <typename T>
+struct CrossThreadPersistentHash : MemberHash<T> {
+ STATIC_ONLY(CrossThreadPersistentHash);
+};
+
+template <typename T>
+struct DefaultHash<blink::CrossThreadPersistent<T>> {
+ STATIC_ONLY(DefaultHash);
+ using Hash = CrossThreadPersistentHash<T>;
+};
+
template<typename T>
struct ParamStorageTraits<blink::WeakPersistentThisPointer<T>> {
STATIC_ONLY(ParamStorageTraits);
Index: third_party/WebKit/Source/platform/heap/PersistentNode.cpp
diff --git a/third_party/WebKit/Source/platform/heap/PersistentNode.cpp b/third_party/WebKit/Source/platform/heap/PersistentNode.cpp
index 2c512958fed0f56d3fa88e42fe2b6710c21e5b8d..615b6cb76a356fdcadac8dc0dd2a834005e16ddc 100644
--- a/third_party/WebKit/Source/platform/heap/PersistentNode.cpp
+++ b/third_party/WebKit/Source/platform/heap/PersistentNode.cpp
@@ -124,7 +124,8 @@ void PersistentRegion::tracePersistentNodes(Visitor* visitor, ShouldTraceCallbac
bool CrossThreadPersistentRegion::shouldTracePersistentNode(Visitor* visitor, PersistentNode* node)
{
CrossThreadPersistent<DummyGCBase>* persistent = reinterpret_cast<CrossThreadPersistent<DummyGCBase>*>(node->self());
- ASSERT(persistent);
+ DCHECK(persistent);
+ DCHECK(!persistent->isHashTableDeletedValue());
Address rawObject = reinterpret_cast<Address>(persistent->get());
if (!rawObject)
return false;

[sigb...@opera.com]

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/Persistent.h
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/Persistent.h#newcode213
third_party/WebKit/Source/platform/heap/Persistent.h:213: if
(!m_persistentNode)
Moving this up here (again) will lead to a race.

https://codereview.chromium.org/2007283002/

[sigb...@opera.com]

Some unit tests would be good.


https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h
File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right):

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode425
third_party/WebKit/Source/platform/heap/HeapAllocator.h:425: template

<typename T> struct VectorTraits<blink::CrossThreadPersistent<T>> :
VectorTraitsBase<blink::CrossThreadPersistent<T>> {

Why is this needed for CrossThreadPersistent, but not Persistent? Or to
put it differently, why not add support for both kinds at the same time?

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode428
third_party/WebKit/Source/platform/heap/HeapAllocator.h:428: static

const bool canInitializeWithMemset = false;

This can be |true|.

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode430
third_party/WebKit/Source/platform/heap/HeapAllocator.h:430: static

const bool canMoveWithMemcpy = false;

Wouldn't |true| work out?

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode564
third_party/WebKit/Source/platform/heap/HeapAllocator.h:564: // FIXME:

The distinction between PeekInType and PassInType is there for

nit: shouldn't be adding new FIXMEs by now.

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode571
third_party/WebKit/Source/platform/heap/HeapAllocator.h:571: using

IteratorGetType = blink::Member<T>*;

why Member<> ?

https://codereview.chromium.org/2007283002/

[kei...@chromium.org]

Added HeapTest.CrossThreadPersistent{Vector,Set}

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h
File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right):

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode425
third_party/WebKit/Source/platform/heap/HeapAllocator.h:425: template
<typename T> struct VectorTraits<blink::CrossThreadPersistent<T>> :
VectorTraitsBase<blink::CrossThreadPersistent<T>> {

On 2016/05/25 13:26:39, sof wrote:
> Why is this needed for CrossThreadPersistent, but not Persistent? Or
to put it
> differently, why not add support for both kinds at the same time?

I need this to do
typedef Deque<CrossThreadPersistent<SQLTransactionBackend>>
TransactionsQueue;

in SQLTransactionCoordinator.h

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode428
third_party/WebKit/Source/platform/heap/HeapAllocator.h:428: static
const bool canInitializeWithMemset = false;

On 2016/05/25 13:26:39, sof wrote:
> This can be |true|.

Done.

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode430
third_party/WebKit/Source/platform/heap/HeapAllocator.h:430: static
const bool canMoveWithMemcpy = false;

On 2016/05/25 13:26:39, sof wrote:
> Wouldn't |true| work out?

Done.

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode564
third_party/WebKit/Source/platform/heap/HeapAllocator.h:564: // FIXME:
The distinction between PeekInType and PassInType is there for

On 2016/05/25 13:26:39, sof wrote:
> nit: shouldn't be adding new FIXMEs by now.

Done.

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode571
third_party/WebKit/Source/platform/heap/HeapAllocator.h:571: using
IteratorGetType = blink::Member<T>*;

On 2016/05/25 13:26:39, sof wrote:
> why Member<> ?

Done.

https://codereview.chromium.org/2007283002/

[kei...@chromium.org]

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h
File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right):

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode425
third_party/WebKit/Source/platform/heap/HeapAllocator.h:425: template
<typename T> struct VectorTraits<blink::CrossThreadPersistent<T>> :
VectorTraitsBase<blink::CrossThreadPersistent<T>> {

On 2016/05/26 04:23:13, keishi wrote:
> On 2016/05/25 13:26:39, sof wrote:
> > Why is this needed for CrossThreadPersistent, but not Persistent? Or
to put it
> > differently, why not add support for both kinds at the same time?
>
> I need this to do
> typedef Deque<CrossThreadPersistent<SQLTransactionBackend>>
TransactionsQueue;
>
> in SQLTransactionCoordinator.h

So I don't need support for Persistent right now. Should I still add it?

https://codereview.chromium.org/2007283002/

[sigb...@opera.com]

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h
File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right):

https://codereview.chromium.org/2007283002/diff/1/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode425
third_party/WebKit/Source/platform/heap/HeapAllocator.h:425: template
<typename T> struct VectorTraits<blink::CrossThreadPersistent<T>> :
VectorTraitsBase<blink::CrossThreadPersistent<T>> {

On 2016/05/26 04:45:43, keishi wrote:
> On 2016/05/26 04:23:13, keishi wrote:
> > On 2016/05/25 13:26:39, sof wrote:
> > > Why is this needed for CrossThreadPersistent, but not Persistent?
Or to put
> it
> > > differently, why not add support for both kinds at the same time?
> >
> > I need this to do
> > typedef Deque<CrossThreadPersistent<SQLTransactionBackend>>
TransactionsQueue;
> >
> > in SQLTransactionCoordinator.h
>
> So I don't need support for Persistent right now. Should I still add
it?

That would my preference.

https://codereview.chromium.org/2007283002/

[kei...@chromium.org]

> That would my preference.
Done. Changed to PersistentBase.


https://codereview.chromium.org/2007283002/

[sigb...@opera.com]

https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Source/platform/heap/HeapAllocator.h
File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right):

https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode565
third_party/WebKit/Source/platform/heap/HeapAllocator.h:565:

template<typename T> struct HashTraits<blink::CrossThreadPersistent<T>>
: SimpleClassHashTraits<blink::CrossThreadPersistent<T>> {

Can we make this be over PersistentBase<> also, or do we need to split
it out into two specializations?

https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Source/platform/heap/Persistent.h
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Source/platform/heap/Persistent.h#newcode383
third_party/WebKit/Source/platform/heap/Persistent.h:383:
CrossThreadPersistent(WTF::HashTableDeletedValueType x) : Parent(x) { }
Isn't this ctor needed for Persistent<> also?

https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Source/platform/heap/Persistent.h#newcode695
third_party/WebKit/Source/platform/heap/Persistent.h:695: struct
CrossThreadPersistentHash : MemberHash<T> {
Add for Persistent<> also.

https://codereview.chromium.org/2007283002/

[kei...@chromium.org]

https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Source/platform/heap/HeapAllocator.h
File third_party/WebKit/Source/platform/heap/HeapAllocator.h (right):

https://codereview.chromium.org/2007283002/diff/80001/third_party/WebKit/Source/platform/heap/HeapAllocator.h#newcode565
third_party/WebKit/Source/platform/heap/HeapAllocator.h:565:
template<typename T> struct HashTraits<blink::CrossThreadPersistent<T>>
: SimpleClassHashTraits<blink::CrossThreadPersistent<T>> {

On 2016/05/26 09:41:44, sof wrote:
> Can we make this be over PersistentBase<> also, or do we need to split
it out
> into two specializations?

I tried PersistentBase but it didn't work so had to split it. But I
created HandleHashTrait to share the code.

https://codereview.chromium.org/2007283002/

[sigb...@opera.com]

getting close.


https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/Heap.h
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/Heap.h#newcode206
third_party/WebKit/Source/platform/heap/Heap.h:206: if
(!ThreadState::current())
What's going wrong without this bail out? I thought the predicate would
just inspect the mark bit in the header preceding |object|.

https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/HeapTest.cpp
File third_party/WebKit/Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/HeapTest.cpp#newcode3034
third_party/WebKit/Source/platform/heap/HeapTest.cpp:3034:
TEST(HeapTest, CrossThreadPersistentSet)
Could you create the Persistent edition of this test also?

https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/Persistent.h
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/Persistent.h#newcode383

third_party/WebKit/Source/platform/heap/Persistent.h:383:
CrossThreadPersistent(WTF::HashTableDeletedValueType x) : Parent(x) { }

The Persistent<> version of this?

https://codereview.chromium.org/2007283002/

[kei...@chromium.org]

https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/Heap.h
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/Heap.h#newcode206
third_party/WebKit/Source/platform/heap/Heap.h:206: if
(!ThreadState::current())

On 2016/05/26 12:09:41, sof wrote:
> What's going wrong without this bail out? I thought the predicate
would just
> inspect the mark bit in the header preceding |object|.

Sorry this is part of https://codereview.chromium.org/2012763002/ .
I used the wrong diff base.

I explain in the comments there, but the tests below seem to be creating
CrossThreadPersistents on non-oilpan attached threads.
> ExtensionApiTest.TabAudible
> ScriptStreamingTest.EncodingFromBOM
> ScriptStreamingTest.EncodingChanges
> ScriptStreamingTest.SuppressingStreaming
> ScriptStreamingTest.CompilingStreamedScriptWithParseError
> ScriptStreamingTest.ScriptsWithSmallFirstChunk
> ScriptStreamingTest.CancellingStreaming
> ScriptStreamingTest.CompilingStreamedScript

https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/Persistent.h
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2007283002/diff/100001/third_party/WebKit/Source/platform/heap/Persistent.h#newcode383
third_party/WebKit/Source/platform/heap/Persistent.h:383:
CrossThreadPersistent(WTF::HashTableDeletedValueType x) : Parent(x) { }

On 2016/05/26 12:09:41, sof wrote:
> The Persistent<> version of this?

Done.

https://codereview.chromium.org/2007283002/

[sigb...@opera.com]

lgtm, thanks for the iterations.

https://codereview.chromium.org/2007283002/

[har...@chromium.org]

LGTM


https://codereview.chromium.org/2007283002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/2007283002/120001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/2007283002/120001


https://codereview.chromium.org/2007283002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #7 (id:120001)

https://codereview.chromium.org/2007283002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 7 (id:??) landed as
https://crrev.com/5adc4454691bffcbfdc7cd60866c145a4e67de0d
Cr-Commit-Position: refs/heads/master@{#396410}

https://codereview.chromium.org/2007283002/

[kei...@chromium.org]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2028433003/ by kei...@chromium.org.

The reason for reverting is: blink_perf.bindings regression on android.webview
crbug.com/615832.

https://codereview.chromium.org/2007283002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/patch-status/2007283002/160001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/2007283002/160001


https://codereview.chromium.org/2007283002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #9 (id:160001)

https://codereview.chromium.org/2007283002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 9 (id:??) landed as
https://crrev.com/365962a919dbc711a23bc388c214778367522ba3
Cr-Commit-Position: refs/heads/master@{#397076}

https://codereview.chromium.org/2007283002/

[sigb...@opera.com]

what did the perf regression uncover?

https://codereview.chromium.org/2007283002/

[kei...@chromium.org]

On 2016/06/01 08:00:01, sof wrote:
> what did the perf regression uncover?

I'm not sure about the cause since it was only observed on one test on one bot
(android-webview-nexus5X). But because the test is doesn't use threads I guessed
that the change to HashTraits<blink::Member<T>> somehow effected it. PS9 reverts
that part and I'm seeing if it fixes it.

https://codereview.chromium.org/2007283002/

[kei...@chromium.org]

On 2016/06/01 08:09:05, keishi wrote:
> On 2016/06/01 08:00:01, sof wrote:
> > what did the perf regression uncover?
>
> I'm not sure about the cause since it was only observed on one test on one bot
> (android-webview-nexus5X). But because the test is doesn't use threads I
guessed
> that the change to HashTraits<blink::Member<T>> somehow effected it. PS9
reverts
> that part and I'm seeing if it fixes it.

Looks like it worked. Regression not seen after relanded.
https://chromeperf.appspot.com/report?sid=c9da549580958add3d6b6fb5f1a3ad6bc51330824ec03f8c0ceed5158fb82968&rev=396420

https://codereview.chromium.org/2007283002/