Allow null other.m_parent in TraceWrapperMember::operator= if other is nullptr.

Allow null other.m_parent in TraceWrapperMember::operator= if other is nullptr.

This CL fixes the DCHECK condition in TraceWrapperMember::operator= to allow
assignment of null TraceWrapperMember.

This CL allows TraceWrapperMember to be used in HeapHash{Set,Map}, as
HashTable::rehash operations involve assignment of empty==null TraceWrapperMembers.

BUG=594639, 707123
Review-Url: https://codereview.chromium.org/2802593002
Cr-Commit-Position: refs/heads/master@{#462044}
Committed: https://chromium.googlesource.com/chromium/src/+/e0828110c362763556738df32990abbc38370ebc

[kouhei (in TOK), 2017-04-05 07:25:27]

Description was changed from

==========
Allow null other.m_parent in TraceWrapperMember::operator= if other is nullptr.

This CL fixes the DCHECK condition in TraceWrapperMember::operator= to allow
assignment of null TraceWrapperMember.

This CL allows TraceWrapperMember to be used in HeapHash{Set,Map}, as
HashTable::rehash operations involve assignment of empty==null
TraceWrapperMembers.

BUG=594639
==========

to

==========
Allow null other.m_parent in TraceWrapperMember::operator= if other is nullptr.

This CL fixes the DCHECK condition in TraceWrapperMember::operator= to allow
assignment of null TraceWrapperMember.

This CL allows TraceWrapperMember to be used in HeapHash{Set,Map}, as
HashTable::rehash operations involve assignment of empty==null
TraceWrapperMembers.

BUG=594639, 707123
==========

[kouhei (in TOK), 2017-04-05 07:25:27]

kouhei@chromium.org changed reviewers:
+ haraken@chromium.org, mlippautz@chromium.org, tzik@chromium.org,
yutak@chromium.org

[kouhei (in TOK), 2017-04-05 07:25:56]

mlippautz, haraken: PTAL
tzik, yutak: for C++/WTF expertise

[kouhei (in TOK), 2017-04-05 07:26:56]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 07:27:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 07:37:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 07:37:33]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Michael Lippautz, 2017-04-05 07:52:19]

Can you elaborate on this change a bit? From a GC perspective this is incorrect
as we may sneak in nullptr as backpointer which invalidates the write barrier.

E.g.

Member<SomeType> b = ...;
TraceWrapperMember<SomeType> a(backpointer, value);
a = TraceWrapperMember<SomeType>(nullptr, nullptr);  // Enabled by this CL.
// GC Starts and marks the backpointer object
a = b;  // We don't emit the write barrier as 'a' has no back pointer.
// We loose 'b' in the GC.

In fact, we should have this case as a test in ScriptWrappableVisitorTest.cpp
with the requirement of 'b' being marked after assigning upon landing such a
change. The tests added in this CL are good as they test the map/set behavior
but not enough as we also require the GC-side of things here.

We solved this issue for now by implementing specific 'swap' methods for
HeapVector that are unfortunately slower than the regular ones but at least
preserve GC semantics. I would assume that you need to do the same for
HeapHash{Set,Map}.

haraken: This would all become obsolete if we would have a bitmap for object
starts and could thus get rid of the back pointer.

[Michael Lippautz, 2017-04-05 07:58:49]

On 2017/04/05 07:52:19, Michael Lippautz wrote:
> Can you elaborate on this change a bit? From a GC perspective this is
incorrect
> as we may sneak in nullptr as backpointer which invalidates the write barrier.
> 
> E.g.
> 
> Member<SomeType> b = ...;
> TraceWrapperMember<SomeType> a(backpointer, value);
> a = TraceWrapperMember<SomeType>(nullptr, nullptr);  // Enabled by this CL.
> // GC Starts and marks the backpointer object
> a = b;  // We don't emit the write barrier as 'a' has no back pointer.
> // We loose 'b' in the GC.
> 
> In fact, we should have this case as a test in ScriptWrappableVisitorTest.cpp
> with the requirement of 'b' being marked after assigning upon landing such a
> change. The tests added in this CL are good as they test the map/set behavior
> but not enough as we also require the GC-side of things here.
> 
> We solved this issue for now by implementing specific 'swap' methods for
> HeapVector that are unfortunately slower than the regular ones but at least
> preserve GC semantics. I would assume that you need to do the same for
> HeapHash{Set,Map}.
> 
> haraken: This would all become obsolete if we would have a bitmap for object
> starts and could thus get rid of the back pointer.

Hah, I just realized that we disallow assigning from Member<T> without back
pointer through a DCHECK. So the change in TraceWrapperMember should be fine as
is.

Please add a test case that checks for that the parent pointers are properly
swapped though.

[kouhei (in TOK), 2017-04-05 08:01:50]

On 2017/04/05 07:58:49, Michael Lippautz wrote:
> On 2017/04/05 07:52:19, Michael Lippautz wrote:
> > Can you elaborate on this change a bit? From a GC perspective this is
> incorrect
> > as we may sneak in nullptr as backpointer which invalidates the write
barrier.
> > 
> > E.g.
> > 
> > Member<SomeType> b = ...;
> > TraceWrapperMember<SomeType> a(backpointer, value);
> > a = TraceWrapperMember<SomeType>(nullptr, nullptr);  // Enabled by this CL.
> > // GC Starts and marks the backpointer object
> > a = b;  // We don't emit the write barrier as 'a' has no back pointer.
> > // We loose 'b' in the GC.
> > 
> > In fact, we should have this case as a test in
ScriptWrappableVisitorTest.cpp
> > with the requirement of 'b' being marked after assigning upon landing such a
> > change. The tests added in this CL are good as they test the map/set
behavior
> > but not enough as we also require the GC-side of things here.
> > 
> > We solved this issue for now by implementing specific 'swap' methods for
> > HeapVector that are unfortunately slower than the regular ones but at least
> > preserve GC semantics. I would assume that you need to do the same for
> > HeapHash{Set,Map}.
> > 
> > haraken: This would all become obsolete if we would have a bitmap for object
> > starts and could thus get rid of the back pointer.
> 
> Hah, I just realized that we disallow assigning from Member<T> without back
> pointer through a DCHECK. So the change in TraceWrapperMember should be fine
as
> is.
Yes. :)

> Please add a test case that checks for that the parent pointers are properly
> swapped though.
Would you elaborate on this?

[Michael Lippautz, 2017-04-05 08:29:05]

On 2017/04/05 08:01:50, kouhei wrote:
> On 2017/04/05 07:58:49, Michael Lippautz wrote:
> > On 2017/04/05 07:52:19, Michael Lippautz wrote:
> > > Can you elaborate on this change a bit? From a GC perspective this is
> > incorrect
> > > as we may sneak in nullptr as backpointer which invalidates the write
> barrier.
> > > 
> > > E.g.
> > > 
> > > Member<SomeType> b = ...;
> > > TraceWrapperMember<SomeType> a(backpointer, value);
> > > a = TraceWrapperMember<SomeType>(nullptr, nullptr);  // Enabled by this
CL.
> > > // GC Starts and marks the backpointer object
> > > a = b;  // We don't emit the write barrier as 'a' has no back pointer.
> > > // We loose 'b' in the GC.
> > > 
> > > In fact, we should have this case as a test in
> ScriptWrappableVisitorTest.cpp
> > > with the requirement of 'b' being marked after assigning upon landing such
a
> > > change. The tests added in this CL are good as they test the map/set
> behavior
> > > but not enough as we also require the GC-side of things here.
> > > 
> > > We solved this issue for now by implementing specific 'swap' methods for
> > > HeapVector that are unfortunately slower than the regular ones but at
least
> > > preserve GC semantics. I would assume that you need to do the same for
> > > HeapHash{Set,Map}.
> > > 
> > > haraken: This would all become obsolete if we would have a bitmap for
object
> > > starts and could thus get rid of the back pointer.
> > 
> > Hah, I just realized that we disallow assigning from Member<T> without back
> > pointer through a DCHECK. So the change in TraceWrapperMember should be fine
> as
> > is.
> Yes. :)
> 
> > Please add a test case that checks for that the parent pointers are properly
> > swapped though.
> Would you elaborate on this?

At the top of my head I was thinking about something along the lines of

typedef ... Wrapper;
DeathAwareScriptWrappable* parent1 = ...;
....* child1 = ....;
HeapHashSet<Wrapper> a;
a.insert(Wrapper(parent1, child1));
HeapHashSet<Wrapper> b;
swap(a,b);
// a.size() == 0
// b.size() == 1
// find element in b as 'e'
// *e == child1
// e.parent() == parent1

[kouhei (in TOK), 2017-04-05 09:52:24]

On 2017/04/05 08:29:05, Michael Lippautz wrote:
> On 2017/04/05 08:01:50, kouhei wrote:
> > On 2017/04/05 07:58:49, Michael Lippautz wrote:
> > > On 2017/04/05 07:52:19, Michael Lippautz wrote:
> > > > Can you elaborate on this change a bit? From a GC perspective this is
> > > incorrect
> > > > as we may sneak in nullptr as backpointer which invalidates the write
> > barrier.
> > > > 
> > > > E.g.
> > > > 
> > > > Member<SomeType> b = ...;
> > > > TraceWrapperMember<SomeType> a(backpointer, value);
> > > > a = TraceWrapperMember<SomeType>(nullptr, nullptr);  // Enabled by this
> CL.
> > > > // GC Starts and marks the backpointer object
> > > > a = b;  // We don't emit the write barrier as 'a' has no back pointer.
> > > > // We loose 'b' in the GC.
> > > > 
> > > > In fact, we should have this case as a test in
> > ScriptWrappableVisitorTest.cpp
> > > > with the requirement of 'b' being marked after assigning upon landing
such
> a
> > > > change. The tests added in this CL are good as they test the map/set
> > behavior
> > > > but not enough as we also require the GC-side of things here.
> > > > 
> > > > We solved this issue for now by implementing specific 'swap' methods for
> > > > HeapVector that are unfortunately slower than the regular ones but at
> least
> > > > preserve GC semantics. I would assume that you need to do the same for
> > > > HeapHash{Set,Map}.
> > > > 
> > > > haraken: This would all become obsolete if we would have a bitmap for
> object
> > > > starts and could thus get rid of the back pointer.
> > > 
> > > Hah, I just realized that we disallow assigning from Member<T> without
back
> > > pointer through a DCHECK. So the change in TraceWrapperMember should be
fine
> > as
> > > is.
> > Yes. :)
> > 
> > > Please add a test case that checks for that the parent pointers are
properly
> > > swapped though.
> > Would you elaborate on this?
> 
> At the top of my head I was thinking about something along the lines of
> 
> typedef ... Wrapper;
> DeathAwareScriptWrappable* parent1 = ...;
> ....* child1 = ....;
> HeapHashSet<Wrapper> a;
> a.insert(Wrapper(parent1, child1));
> HeapHashSet<Wrapper> b;
> swap(a,b);
> // a.size() == 0
> // b.size() == 1
> // find element in b as 'e'
> // *e == child1
> // e.parent() == parent1

Done

[Yuta Kitamura, 2017-04-05 10:01:19]

WTF-ism in the tests LGTM.

[kouhei (in TOK), 2017-04-05 10:02:13]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 10:02:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 10:51:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 10:51:35]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[haraken, 2017-04-05 11:37:51]

LGTM

[Michael Lippautz, 2017-04-05 11:41:03]

Thanks for adding the tests, lgtm

[kouhei (in TOK), 2017-04-05 12:04:43]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2017-04-05 12:04:51]

Thanks!

[commit-bot: I haz the power, 2017-04-05 12:04:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 12:53:46]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491393883643150,
"parent_rev": "08c0ab4e786593dd8d91407165da85d923d0179b", "commit_rev":
"e0828110c362763556738df32990abbc38370ebc"}

[commit-bot: I haz the power, 2017-04-05 12:54:28]

Description was changed from

==========
Allow null other.m_parent in TraceWrapperMember::operator= if other is nullptr.

This CL fixes the DCHECK condition in TraceWrapperMember::operator= to allow
assignment of null TraceWrapperMember.

This CL allows TraceWrapperMember to be used in HeapHash{Set,Map}, as
HashTable::rehash operations involve assignment of empty==null
TraceWrapperMembers.

BUG=594639, 707123
==========

to

==========
Allow null other.m_parent in TraceWrapperMember::operator= if other is nullptr.

This CL fixes the DCHECK condition in TraceWrapperMember::operator= to allow
assignment of null TraceWrapperMember.

This CL allows TraceWrapperMember to be used in HeapHash{Set,Map}, as
HashTable::rehash operations involve assignment of empty==null
TraceWrapperMembers.

BUG=594639, 707123

Review-Url: https://codereview.chromium.org/2802593002
Cr-Commit-Position: refs/heads/master@{#462044}
Committed:
https://chromium.googlesource.com/chromium/src/+/e0828110c362763556738df32990...
==========

[commit-bot: I haz the power, 2017-04-05 12:54:29]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/e0828110c362763556738df32990...