[Oilpan]: Moving the FontSelector/FontCacheClient, CSSSegmentedFontFace, and FontFaceCache to the o…

[Oilpan]: Moving the FontSelector/FontCacheClient, CSSSegmentedFontFace, and FontFaceCache to the oilpan heap.

Had to add two tests to the OilpanExpectations file since this triggers a change in the order in which scrollbars are layed out.

R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org
BUG=341815
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171119

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

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

[sof, 2014-04-07 13:37:58]

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCache.h
File Source/core/css/FontFaceCache.h (right):

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
Source/core/css/FontFaceCache.h:44: class FontFaceCache :
NoBaseWillBeGarbageCollectedFinalized<FontFaceCache> {
A FINAL would be in order here?

[haraken, 2014-04-07 14:10:11]

https://codereview.chromium.org/227083006/diff/1/Source/core/css/CSSFontFace.h
File Source/core/css/CSSFontFace.h (right):

https://codereview.chromium.org/227083006/diff/1/Source/core/css/CSSFontFace....
Source/core/css/CSSFontFace.h:117: CSSSegmentedFontFace* m_segmentedFontFace;

I'm just curious: Is it safe to not replace "a raw pointer from an off-heap
object to an on-heap object" with a Member? I think it's safe as long as the raw
pointer is safe in the current reference-counted world.

https://codereview.chromium.org/227083006/diff/1/Source/core/css/CSSFontSelec...
File Source/core/css/CSSFontSelector.cpp (right):

https://codereview.chromium.org/227083006/diff/1/Source/core/css/CSSFontSelec...
Source/core/css/CSSFontSelector.cpp:232: visitor->trace(m_fontFaceCache);

You need to trace m_fontLoader. I hope the plugin catches the error :)

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
File Source/core/css/FontFaceCache.cpp (right):

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
Source/core/css/FontFaceCache.cpp:63: familyFontFaces = adoptPtrWillBeNoop(new
TraitsMap);

In our experimental branch, I think we were using a get() & set() pattern
instead of an add() & new() pattern.

  Item* item = map.get(name);
  if (!item)
    map.set(name, new Item);

In my understanding, previously we had to use the get() & set() pattern because
we didn't have conservative scanning. Now that we have the conservative
scanning, we can just use the add() & new() pattern. Am I understanding
correctly?

https://codereview.chromium.org/227083006/diff/1/Source/platform/fonts/FontCa...
File Source/platform/fonts/FontCache.cpp (right):

https://codereview.chromium.org/227083006/diff/1/Source/platform/fonts/FontCa...
Source/platform/fonts/FontCache.cpp:246: static bool invalidateFontCache =
false;

invalidateFontCache => fontCacheMapIsInitialized ?

I don't fully understand what "invalidate" means here.

https://codereview.chromium.org/227083006/diff/1/Source/platform/fonts/FontCa...
Source/platform/fonts/FontCache.cpp:251:
DEFINE_STATIC_LOCAL(Persistent<HeapHashSet<Member<FontCacheClient> > >, clients,
(new HeapHashSet<Member<FontCacheClient> >()));

Shouldn't this be a hash set of weak members? I guess the pointers to
FontCacheClients are weak.

https://codereview.chromium.org/227083006/diff/1/Source/platform/fonts/FontCa...
Source/platform/fonts/FontCache.cpp:265: void
FontCache::removeClient(FontCacheClient* client)

If the FontCacheClients are weak, this method should be in #if !ENABLE(OILPAN).

[wibling-chromium, 2014-04-08 09:35:59]

Thanks for the reviews!

https://codereview.chromium.org/227083006/diff/1/Source/core/css/CSSFontFace.h
File Source/core/css/CSSFontFace.h (right):

https://codereview.chromium.org/227083006/diff/1/Source/core/css/CSSFontFace....
Source/core/css/CSSFontFace.h:117: CSSSegmentedFontFace* m_segmentedFontFace;
On 2014/04/07 14:10:12, haraken wrote:
> 
> I'm just curious: Is it safe to not replace "a raw pointer from an off-heap
> object to an on-heap object" with a Member? I think it's safe as long as the
raw
> pointer is safe in the current reference-counted world.

Yes, the requirement is that we check whether it should be replaced with a
WeakMember or a Member. In most cases it should just be a member because the
original raw pointer was only there to break a refcount cycle.

https://codereview.chromium.org/227083006/diff/1/Source/core/css/CSSFontSelec...
File Source/core/css/CSSFontSelector.cpp (right):

https://codereview.chromium.org/227083006/diff/1/Source/core/css/CSSFontSelec...
Source/core/css/CSSFontSelector.cpp:232: visitor->trace(m_fontFaceCache);
On 2014/04/07 14:10:12, haraken wrote:
> 
> You need to trace m_fontLoader. I hope the plugin catches the error :)

It didn't. Thanks for catching this.
I just talked to Ian and the plugin will complain only if there is a member
field or superclass in the object that needs tracing which is not the case here.
The reason to not complain in this case was that it became close to impossible
since an object could have a trace method without it being called in e.g. the
non-oilpan build.

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
File Source/core/css/FontFaceCache.cpp (right):

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
Source/core/css/FontFaceCache.cpp:63: familyFontFaces = adoptPtrWillBeNoop(new
TraitsMap);
On 2014/04/07 14:10:12, haraken wrote:
> 
> In our experimental branch, I think we were using a get() & set() pattern
> instead of an add() & new() pattern.
> 
>   Item* item = map.get(name);
>   if (!item)
>     map.set(name, new Item);
> 
> In my understanding, previously we had to use the get() & set() pattern
because
> we didn't have conservative scanning. Now that we have the conservative
> scanning, we can just use the add() & new() pattern. Am I understanding
> correctly?

I am not sure we need conservative scanning to keep the object alive since it
would have been kept alive from the Member anyway if it was already part of the
map, but maybe I am missing a corner case.
Either way I decided to use the add/new pattern since it was the one already
being used in the code.

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCache.h
File Source/core/css/FontFaceCache.h (right):

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
Source/core/css/FontFaceCache.h:44: class FontFaceCache :
NoBaseWillBeGarbageCollectedFinalized<FontFaceCache> {
On 2014/04/07 13:37:59, sof wrote:
> A FINAL would be in order here?

Done.

https://codereview.chromium.org/227083006/diff/1/Source/platform/fonts/FontCa...
File Source/platform/fonts/FontCache.cpp (right):

https://codereview.chromium.org/227083006/diff/1/Source/platform/fonts/FontCa...
Source/platform/fonts/FontCache.cpp:246: static bool invalidateFontCache =
false;
On 2014/04/07 14:10:12, haraken wrote:
> 
> invalidateFontCache => fontCacheMapIsInitialized ?
> 
> I don't fully understand what "invalidate" means here.

It guards whether we need to invalidate the cache, hence the name, by default we
don't need to invalidate the cache only when an object has been added.

https://codereview.chromium.org/227083006/diff/1/Source/platform/fonts/FontCa...
Source/platform/fonts/FontCache.cpp:251:
DEFINE_STATIC_LOCAL(Persistent<HeapHashSet<Member<FontCacheClient> > >, clients,
(new HeapHashSet<Member<FontCacheClient> >()));
On 2014/04/07 14:10:12, haraken wrote:
> 
> Shouldn't this be a hash set of weak members? I guess the pointers to
> FontCacheClients are weak.

It is not strictly needed since removeClient is always called. That said it is
definitely a good idea so I will make that change now.

https://codereview.chromium.org/227083006/diff/1/Source/platform/fonts/FontCa...
Source/platform/fonts/FontCache.cpp:265: void
FontCache::removeClient(FontCacheClient* client)
On 2014/04/07 14:10:12, haraken wrote:
> 
> If the FontCacheClients are weak, this method should be in #if
!ENABLE(OILPAN).

Done.

[wibling-chromium, 2014-04-08 10:52:32]

PTAL

Updated the diff.

[haraken, 2014-04-08 11:14:39]

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
File Source/core/css/FontFaceCache.cpp (right):

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
Source/core/css/FontFaceCache.cpp:63: familyFontFaces = adoptPtrWillBeNoop(new
TraitsMap);
On 2014/04/08 09:35:59, wibling-chromium wrote:
> On 2014/04/07 14:10:12, haraken wrote:
> > 
> > In our experimental branch, I think we were using a get() & set() pattern
> > instead of an add() & new() pattern.
> > 
> >   Item* item = map.get(name);
> >   if (!item)
> >     map.set(name, new Item);
> > 
> > In my understanding, previously we had to use the get() & set() pattern
> because
> > we didn't have conservative scanning. Now that we have the conservative
> > scanning, we can just use the add() & new() pattern. Am I understanding
> > correctly?
> 
> I am not sure we need conservative scanning to keep the object alive since it
> would have been kept alive from the Member anyway if it was already part of
the
> map, but maybe I am missing a corner case.
> Either way I decided to use the add/new pattern since it was the one already
> being used in the code.

Sorry, this issue is not related to conservative scanning.

The reason why we replaced add/new with get/set in the experiment branch is this
one:
https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel...

> I don't think we should be using this pattern. The problem is that the create
> call below can allocate. That allocation can cause a GC which can call
> finalizers. If one of those finalizers changes the segmentedFontFaceCache we
can
> get a memory corruption here. Instead, we should do something like:
>
> Handle<CSSSegmentedFontFace> face = segmentedFontFaceCache->add(traitsMask,
nullptr).iterator->value;
> if (value)
>     return value;
> face = CSSSegmentedFontFace::create(this);
> segmentedFontFaceCache->set(traitsMask, face);

In my understanding, this issue still exists in the current oilpan, so we need
to use the get/set pattern instead of the add/new pattern.

Would you ask it to mads@ offline (and tell me the answer:-)?

[zerny-chromium, 2014-04-08 12:10:20]

QQ:

https://codereview.chromium.org/227083006/diff/20001/Source/core/css/CSSSegme...
File Source/core/css/CSSSegmentedFontFace.h (right):

https://codereview.chromium.org/227083006/diff/20001/Source/core/css/CSSSegme...
Source/core/css/CSSSegmentedFontFace.h:83: RawPtrWillBeMember<CSSFontSelector>
m_fontSelector;
This could potentially change lifetime of the font selector. Is there a specific
reason to trace this raw pointer? Or can we wait as done with other raw ptrs.

[wibling-chromium, 2014-04-08 12:47:23]

Thanks again for the reviews. I have updated the diff.

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
File Source/core/css/FontFaceCache.cpp (right):

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
Source/core/css/FontFaceCache.cpp:63: familyFontFaces = adoptPtrWillBeNoop(new
TraitsMap);
On 2014/04/08 11:14:40, haraken wrote:
> On 2014/04/08 09:35:59, wibling-chromium wrote:
> > On 2014/04/07 14:10:12, haraken wrote:
> > > 
> > > In our experimental branch, I think we were using a get() & set() pattern
> > > instead of an add() & new() pattern.
> > > 
> > >   Item* item = map.get(name);
> > >   if (!item)
> > >     map.set(name, new Item);
> > > 
> > > In my understanding, previously we had to use the get() & set() pattern
> > because
> > > we didn't have conservative scanning. Now that we have the conservative
> > > scanning, we can just use the add() & new() pattern. Am I understanding
> > > correctly?
> > 
> > I am not sure we need conservative scanning to keep the object alive since
it
> > would have been kept alive from the Member anyway if it was already part of
> the
> > map, but maybe I am missing a corner case.
> > Either way I decided to use the add/new pattern since it was the one already
> > being used in the code.
> 
> Sorry, this issue is not related to conservative scanning.
> 
> The reason why we replaced add/new with get/set in the experiment branch is
this
> one:
>
https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel...
> 
> > I don't think we should be using this pattern. The problem is that the
create
> > call below can allocate. That allocation can cause a GC which can call
> > finalizers. If one of those finalizers changes the segmentedFontFaceCache we
> can
> > get a memory corruption here. Instead, we should do something like:
> >
> > Handle<CSSSegmentedFontFace> face = segmentedFontFaceCache->add(traitsMask,
> nullptr).iterator->value;
> > if (value)
> >     return value;
> > face = CSSSegmentedFontFace::create(this);
> > segmentedFontFaceCache->set(traitsMask, face);
> 
> In my understanding, this issue still exists in the current oilpan, so we need
> to use the get/set pattern instead of the add/new pattern.
> 
> Would you ask it to mads@ offline (and tell me the answer:-)?

Just talked to Mads about this:)
As he also mentioned in the review you referred above, the reason is that
anything could potentially happen when a GC is doing finalization since a
destructor could add/remove to the same set which could cause a rehashing or
even a move of the backing making the reference that I hold above pointing into
invalid memory.
If we can guarantee that no resizing of the collection happens from destructors
the add/new pattern is okay, but for now we might as well use the get/set
pattern since that is trivially safe.

https://codereview.chromium.org/227083006/diff/20001/Source/core/css/CSSSegme...
File Source/core/css/CSSSegmentedFontFace.h (right):

https://codereview.chromium.org/227083006/diff/20001/Source/core/css/CSSSegme...
Source/core/css/CSSSegmentedFontFace.h:83: RawPtrWillBeMember<CSSFontSelector>
m_fontSelector;
On 2014/04/08 12:10:20, zerny-chromium wrote:
> This could potentially change lifetime of the font selector. Is there a
specific
> reason to trace this raw pointer? Or can we wait as done with other raw ptrs.

The code already assumes the m_fontSelector is present so it seemed safer to
actually ensure it is alive than rely on the calling code to make it so.

[haraken, 2014-04-08 12:59:58]

Thanks, now I understand the get/set or add/new issue. I agree that the get/set
pattern is better since it's clear that it's safe :)

LGTM.

https://codereview.chromium.org/227083006/diff/40001/Source/core/css/FontFace...
File Source/core/css/FontFaceCache.cpp (right):

https://codereview.chromium.org/227083006/diff/40001/Source/core/css/FontFace...
Source/core/css/FontFaceCache.cpp:67: segmentedFontFace =
familyFontFaces->add(fontFace->traits().mask(),
CSSSegmentedFontFace::create(cssFontSelector,
fontFace->traits())).storedValue->value;

You can use set() instead of add().

https://codereview.chromium.org/227083006/diff/40001/Source/core/css/FontFace...
Source/core/css/FontFaceCache.cpp:198: segmentedFontFaceCache =
adoptPtrWillBeNoop(new TraitsMap);

Let's use the get/set pattern for safety here too.

https://codereview.chromium.org/227083006/diff/40001/Source/core/css/FontFace...
Source/core/css/FontFaceCache.cpp:201: RefPtrWillBeMember<CSSSegmentedFontFace>&
face = segmentedFontFaceCache->add(traits.mask(), nullptr).storedValue->value;

Let's use the get/set pattern for safety.

[haraken, 2014-04-08 13:00:22]

tkent-san: Would you take a look at platform/ ?

[tkent, 2014-04-08 13:45:37]

lgtm

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
File Source/core/css/FontFaceCache.cpp (right):

https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
Source/core/css/FontFaceCache.cpp:63: familyFontFaces = adoptPtrWillBeNoop(new
TraitsMap);
On 2014/04/08 12:47:23, wibling-chromium wrote:
> On 2014/04/08 11:14:40, haraken wrote:
> > On 2014/04/08 09:35:59, wibling-chromium wrote:
> > > On 2014/04/07 14:10:12, haraken wrote:
> > > > 
> > > > In our experimental branch, I think we were using a get() & set()
pattern
> > > > instead of an add() & new() pattern.
> > > > 
> > > >   Item* item = map.get(name);
> > > >   if (!item)
> > > >     map.set(name, new Item);
> > > > 
> > > > In my understanding, previously we had to use the get() & set() pattern
> > > because
> > > > we didn't have conservative scanning. Now that we have the conservative
> > > > scanning, we can just use the add() & new() pattern. Am I understanding
> > > > correctly?
> > > 
> > > I am not sure we need conservative scanning to keep the object alive since
> it
> > > would have been kept alive from the Member anyway if it was already part
of
> > the
> > > map, but maybe I am missing a corner case.
> > > Either way I decided to use the add/new pattern since it was the one
already
> > > being used in the code.
> > 
> > Sorry, this issue is not related to conservative scanning.
> > 
> > The reason why we replaced add/new with get/set in the experiment branch is
> this
> > one:
> >
>
https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel...
> > 
> > > I don't think we should be using this pattern. The problem is that the
> create
> > > call below can allocate. That allocation can cause a GC which can call
> > > finalizers. If one of those finalizers changes the segmentedFontFaceCache
we
> > can
> > > get a memory corruption here. Instead, we should do something like:
> > >
> > > Handle<CSSSegmentedFontFace> face =
segmentedFontFaceCache->add(traitsMask,
> > nullptr).iterator->value;
> > > if (value)
> > >     return value;
> > > face = CSSSegmentedFontFace::create(this);
> > > segmentedFontFaceCache->set(traitsMask, face);
> > 
> > In my understanding, this issue still exists in the current oilpan, so we
need
> > to use the get/set pattern instead of the add/new pattern.
> > 
> > Would you ask it to mads@ offline (and tell me the answer:-)?
> 
> Just talked to Mads about this:)
> As he also mentioned in the review you referred above, the reason is that
> anything could potentially happen when a GC is doing finalization since a
> destructor could add/remove to the same set which could cause a rehashing or
> even a move of the backing making the reference that I hold above pointing
into
> invalid memory.
> If we can guarantee that no resizing of the collection happens from
destructors
> the add/new pattern is okay, but for now we might as well use the get/set
> pattern since that is trivially safe.

Shouldn't we apply the add-new idiom to HeapHashMap with WeakMember in general?

[wibling-chromium, 2014-04-08 14:03:09]

On 2014/04/08 13:45:37, tkent wrote:
> lgtm
> 
>
https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
> File Source/core/css/FontFaceCache.cpp (right):
> 
>
https://codereview.chromium.org/227083006/diff/1/Source/core/css/FontFaceCach...
> Source/core/css/FontFaceCache.cpp:63: familyFontFaces = adoptPtrWillBeNoop(new
> TraitsMap);
> On 2014/04/08 12:47:23, wibling-chromium wrote:
> > On 2014/04/08 11:14:40, haraken wrote:
> > > On 2014/04/08 09:35:59, wibling-chromium wrote:
> > > > On 2014/04/07 14:10:12, haraken wrote:
> > > > > 
> > > > > In our experimental branch, I think we were using a get() & set()
> pattern
> > > > > instead of an add() & new() pattern.
> > > > > 
> > > > >   Item* item = map.get(name);
> > > > >   if (!item)
> > > > >     map.set(name, new Item);
> > > > > 
> > > > > In my understanding, previously we had to use the get() & set()
pattern
> > > > because
> > > > > we didn't have conservative scanning. Now that we have the
conservative
> > > > > scanning, we can just use the add() & new() pattern. Am I
understanding
> > > > > correctly?
> > > > 
> > > > I am not sure we need conservative scanning to keep the object alive
since
> > it
> > > > would have been kept alive from the Member anyway if it was already part
> of
> > > the
> > > > map, but maybe I am missing a corner case.
> > > > Either way I decided to use the add/new pattern since it was the one
> already
> > > > being used in the code.
> > > 
> > > Sorry, this issue is not related to conservative scanning.
> > > 
> > > The reason why we replaced add/new with get/set in the experiment branch
is
> > this
> > > one:
> > >
> >
>
https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel...
> > > 
> > > > I don't think we should be using this pattern. The problem is that the
> > create
> > > > call below can allocate. That allocation can cause a GC which can call
> > > > finalizers. If one of those finalizers changes the
segmentedFontFaceCache
> we
> > > can
> > > > get a memory corruption here. Instead, we should do something like:
> > > >
> > > > Handle<CSSSegmentedFontFace> face =
> segmentedFontFaceCache->add(traitsMask,
> > > nullptr).iterator->value;
> > > > if (value)
> > > >     return value;
> > > > face = CSSSegmentedFontFace::create(this);
> > > > segmentedFontFaceCache->set(traitsMask, face);
> > > 
> > > In my understanding, this issue still exists in the current oilpan, so we
> need
> > > to use the get/set pattern instead of the add/new pattern.
> > > 
> > > Would you ask it to mads@ offline (and tell me the answer:-)?
> > 
> > Just talked to Mads about this:)
> > As he also mentioned in the review you referred above, the reason is that
> > anything could potentially happen when a GC is doing finalization since a
> > destructor could add/remove to the same set which could cause a rehashing or
> > even a move of the backing making the reference that I hold above pointing
> into
> > invalid memory.
> > If we can guarantee that no resizing of the collection happens from
> destructors
> > the add/new pattern is okay, but for now we might as well use the get/set
> > pattern since that is trivially safe.
> 
> Shouldn't we apply the add-new idiom to HeapHashMap with WeakMember in
general?

Do you mean we can use the add-new idiom when we have weak members? Or are you
referring to that we have to update all occurrences where we currently have used
the add-new, but should be using get-set?

[wibling-chromium, 2014-04-08 14:03:18]

https://codereview.chromium.org/227083006/diff/40001/Source/core/css/FontFace...
File Source/core/css/FontFaceCache.cpp (right):

https://codereview.chromium.org/227083006/diff/40001/Source/core/css/FontFace...
Source/core/css/FontFaceCache.cpp:67: segmentedFontFace =
familyFontFaces->add(fontFace->traits().mask(),
CSSSegmentedFontFace::create(cssFontSelector,
fontFace->traits())).storedValue->value;
On 2014/04/08 12:59:59, haraken wrote:
> 
> You can use set() instead of add().

Done.

https://codereview.chromium.org/227083006/diff/40001/Source/core/css/FontFace...
Source/core/css/FontFaceCache.cpp:198: segmentedFontFaceCache =
adoptPtrWillBeNoop(new TraitsMap);
On 2014/04/08 12:59:59, haraken wrote:
> 
> Let's use the get/set pattern for safety here too.

Done.

https://codereview.chromium.org/227083006/diff/40001/Source/core/css/FontFace...
Source/core/css/FontFaceCache.cpp:201: RefPtrWillBeMember<CSSSegmentedFontFace>&
face = segmentedFontFaceCache->add(traits.mask(), nullptr).storedValue->value;
On 2014/04/08 12:59:59, haraken wrote:
> 
> Let's use the get/set pattern for safety.

Done.

[tkent, 2014-04-09 01:20:39]

On 2014/04/08 14:03:09, wibling-chromium wrote:
> > Shouldn't we apply the add-new idiom to HeapHashMap with WeakMember in
> general?
> 
> Do you mean we can use the add-new idiom when we have weak members? Or are you
> referring to that we have to update all occurrences where we currently have
used
> the add-new, but should be using get-set?

I just wanted to clarify a general rule for this issue.

- If the key type of a HeapHashMap is not WeakMember, we can continue to use the
add-new pattern.
  (Except extremely strange code like removing a HeapHashMap entry in a
constructor.  It's unsafe in non-Oilpan too.)
- If the key type of a HeapHashMap is WeakMember, we can't use the add-new
pattern.
  Are they right?

If so, I'm negative to replace existing add-new patterns with the get-set
pattern, which is slower than add-new, and a compiler/plugin should warn a
dangerous usage of add-new.

[haraken, 2014-04-09 01:31:15]

On 2014/04/09 01:20:39, tkent wrote:
> On 2014/04/08 14:03:09, wibling-chromium wrote:
> > > Shouldn't we apply the add-new idiom to HeapHashMap with WeakMember in
> > general?
> > 
> > Do you mean we can use the add-new idiom when we have weak members? Or are
you
> > referring to that we have to update all occurrences where we currently have
> used
> > the add-new, but should be using get-set?
> 
> I just wanted to clarify a general rule for this issue.
> 
> - If the key type of a HeapHashMap is not WeakMember, we can continue to use
the
> add-new pattern.
>   (Except extremely strange code like removing a HeapHashMap entry in a
> constructor.  It's unsafe in non-Oilpan too.)
> - If the key type of a HeapHashMap is WeakMember, we can't use the add-new
> pattern.
>   Are they right?

Yes, but it's still true that the get/set pattern is safer than the add/new
pattern. For example, the following scenario is possible (although this is too
hypothetical):

(1) Member<T>& item = add();
(2) Create something.
(3) A GC is triggered.
(4) The GC calls a destructor which allocates some object. The constructor of
the object can insert an item to the hash set and trigger resizing.
(5) |item| becomes stale.

So I think that it is better to always use the get/set pattern unless there is
any particular reason we need to use add/new (e.g., performance). Also "always
use the get/set pattern" would be easier than "use the get/set pattern for weak
members and the add/new pattern for non-weak members". What do you think?

> If so, I'm negative to replace existing add-new patterns with the get-set
> pattern, which is slower than add-new, and a compiler/plugin should warn a
> dangerous usage of add-new.

I'd guess it's hard to do the verification statically.

[Mads Ager (chromium), 2014-04-09 05:37:12]

On 2014/04/09 01:20:39, tkent wrote:
> On 2014/04/08 14:03:09, wibling-chromium wrote:
> > > Shouldn't we apply the add-new idiom to HeapHashMap with WeakMember in
> > general?
> > 
> > Do you mean we can use the add-new idiom when we have weak members? Or are
you
> > referring to that we have to update all occurrences where we currently have
> used
> > the add-new, but should be using get-set?
> 
> I just wanted to clarify a general rule for this issue.
> 
> - If the key type of a HeapHashMap is not WeakMember, we can continue to use
the
> add-new pattern.
>   (Except extremely strange code like removing a HeapHashMap entry in a
> constructor.  It's unsafe in non-Oilpan too.)
> - If the key type of a HeapHashMap is WeakMember, we can't use the add-new
> pattern.
>   Are they right?

WeakMember/Member have the same issues here. WeakMembers in collections behave
like Members if there is a pointer from the stack to the backing store. For
these patterns there is a pointer to the backing store on the stack and
therefore the collection will not change as part of weak processing. So, there
is no difference between WeakMember and Member here.

The potential, rare, problem is the one Haraken mentions. An allocation can now
run arbitrary *destructor* code that is not related to the type of object
allocated. Therefore, it is not the case that this is unsafe if and only if it
was unsafe for non-Oilpan too. If a destructor for another object can modify the
collection you are working with this pattern is unsafe. That is hard to reason
about statically. However, as we move further along with Oilpan and get rid of
more destructor code, this will be less of a problem. In particular, this is not
a problem at all if we do not add elements to collections in destructors and use
weak processing to remove elements from collections when they are dead instead
of doing it in the destructor of the object.

So, the bottom-line is that the add-new pattern is safe *if* we know that the
collection we are dealing with is not explicitly modified in the constructor we
are calling (that is already the case) and is not explicitly modified in *any*
destructors. That is probably the case for the one here, but I haven't
explicitly checked.

> If so, I'm negative to replace existing add-new patterns with the get-set
> pattern, which is slower than add-new, and a compiler/plugin should warn a
> dangerous usage of add-new.

[wibling-chromium, 2014-04-09 06:46:46]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-04-09 06:46:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wibling@chromium.org/227083006/80001

[commit-bot: I haz the power, 2014-04-09 06:48:17]

Change committed as 171119

[tkent, 2014-04-09 06:53:55]

On 2014/04/09 05:37:12, Mads Ager (chromium) wrote:
> On 2014/04/09 01:20:39, tkent wrote:
> > On 2014/04/08 14:03:09, wibling-chromium wrote:
> > > > Shouldn't we apply the add-new idiom to HeapHashMap with WeakMember in
> > > general?
> > > 
> > > Do you mean we can use the add-new idiom when we have weak members? Or are
> you
> > > referring to that we have to update all occurrences where we currently
have
> > used
> > > the add-new, but should be using get-set?
> > 
> > I just wanted to clarify a general rule for this issue.
> > 
> > - If the key type of a HeapHashMap is not WeakMember, we can continue to use
> the
> > add-new pattern.
> >   (Except extremely strange code like removing a HeapHashMap entry in a
> > constructor.  It's unsafe in non-Oilpan too.)
> > - If the key type of a HeapHashMap is WeakMember, we can't use the add-new
> > pattern.
> >   Are they right?
> 
> WeakMember/Member have the same issues here. WeakMembers in collections behave
> like Members if there is a pointer from the stack to the backing store. For
> these patterns there is a pointer to the backing store on the stack and
> therefore the collection will not change as part of weak processing. So, there
> is no difference between WeakMember and Member here.
> 
> The potential, rare, problem is the one Haraken mentions. An allocation can
now
> run arbitrary *destructor* code that is not related to the type of object
> allocated. Therefore, it is not the case that this is unsafe if and only if it
> was unsafe for non-Oilpan too. If a destructor for another object can modify
the
> collection you are working with this pattern is unsafe. That is hard to reason
> about statically. However, as we move further along with Oilpan and get rid of
> more destructor code, this will be less of a problem. In particular, this is
not
> a problem at all if we do not add elements to collections in destructors and
use
> weak processing to remove elements from collections when they are dead instead
> of doing it in the destructor of the object.

Yeah, I understand it.  The WeakMember case is more problematic because
GC-in-allocate can update the HeapHashMap directly.


> So, the bottom-line is that the add-new pattern is safe *if* we know that the
> collection we are dealing with is not explicitly modified in the constructor
we
> are calling (that is already the case) and is not explicitly modified in *any*
> destructors. That is probably the case for the one here, but I haven't
> explicitly checked.
> 
> > If so, I'm negative to replace existing add-new patterns with the get-set
> > pattern, which is slower than add-new, and a compiler/plugin should warn a
> > dangerous usage of add-new.

The get-set pattern also has a defect. It is possible for any destructors to add
new entry to a HashMap between get and set, and the map will be in unexpected
state.

Hmm, this issue looks unresolvable.  What we can do is only "do not update
HashMaps in destructors"

[Mads Ager (chromium), 2014-04-09 07:28:16]

On Wed, Apr 9, 2014 at 8:53 AM, <tkent@chromium.org> wrote:

> On 2014/04/09 05:37:12, Mads Ager (chromium) wrote:
>
>> On 2014/04/09 01:20:39, tkent wrote:
>> > On 2014/04/08 14:03:09, wibling-chromium wrote:
>> > > > Shouldn't we apply the add-new idiom to HeapHashMap with WeakMember
>> in
>> > > general?
>> > >
>> > > Do you mean we can use the add-new idiom when we have weak members?
>> Or are
>> you
>> > > referring to that we have to update all occurrences where we currently
>>
> have
>
>> > used
>> > > the add-new, but should be using get-set?
>> >
>> > I just wanted to clarify a general rule for this issue.
>> >
>> > - If the key type of a HeapHashMap is not WeakMember, we can continue
>> to use
>> the
>> > add-new pattern.
>> >   (Except extremely strange code like removing a HeapHashMap entry in a
>> > constructor.  It's unsafe in non-Oilpan too.)
>> > - If the key type of a HeapHashMap is WeakMember, we can't use the
>> add-new
>> > pattern.
>> >   Are they right?
>>
>
>  WeakMember/Member have the same issues here. WeakMembers in collections
>> behave
>> like Members if there is a pointer from the stack to the backing store.
>> For
>> these patterns there is a pointer to the backing store on the stack and
>> therefore the collection will not change as part of weak processing. So,
>> there
>> is no difference between WeakMember and Member here.
>>
>
>  The potential, rare, problem is the one Haraken mentions. An allocation
>> can
>>
> now
>
>> run arbitrary *destructor* code that is not related to the type of object
>> allocated. Therefore, it is not the case that this is unsafe if and only
>> if it
>> was unsafe for non-Oilpan too. If a destructor for another object can
>> modify
>>
> the
>
>> collection you are working with this pattern is unsafe. That is hard to
>> reason
>> about statically. However, as we move further along with Oilpan and get
>> rid of
>> more destructor code, this will be less of a problem. In particular, this
>> is
>>
> not
>
>> a problem at all if we do not add elements to collections in destructors
>> and
>>
> use
>
>> weak processing to remove elements from collections when they are dead
>> instead
>> of doing it in the destructor of the object.
>>
>
> Yeah, I understand it.  The WeakMember case is more problematic because
> GC-in-allocate can update the HeapHashMap directly.


The WeakMember case is actually no more complicated than the Member case.
For these patterns WeakMember will be exactly the same as Member because
there is a pointer on the stack into the collection backing store. When the
garbage collector finds a pointer into the backing store of a collection
with WeakMembers it will treat those WeakMembers as strong pointers and
they will not die. Therefore, weak processing will not change the
collection. So, weakness doesn't add any extra complications. The two cases
are exactly the same. The issue is only with the add/new pattern *if* a
destructor can mess with that collection.


>  So, the bottom-line is that the add-new pattern is safe *if* we know that
>> the
>> collection we are dealing with is not explicitly modified in the
>> constructor
>>
> we
>
>> are calling (that is already the case) and is not explicitly modified in
>> *any*
>> destructors. That is probably the case for the one here, but I haven't
>> explicitly checked.
>>
>
>  > If so, I'm negative to replace existing add-new patterns with the
>> get-set
>> > pattern, which is slower than add-new, and a compiler/plugin should
>> warn a
>> > dangerous usage of add-new.
>>
>
> The get-set pattern also has a defect. It is possible for any destructors
> to add
> new entry to a HashMap between get and set, and the map will be in
> unexpected
> state.
>

Right, in principle it is possible that a destructor could have added the
member that you have just checked is not there. That defect seems more
unlikely than the "this collection was touched" defect of the add-assign
pattern, but it could happen.


> Hmm, this issue looks unresolvable.  What we can do is only "do not update
> HashMaps in destructors.
>

Yes, the end goal in any case is to get rid of as many destructors as
possible. In particular, we should use weak processing to remove dead
objects from collections instead of doing it in destructors and we should
not add to collections in destructors. If we do that consistently both the
add-assign and the get-set patterns would be safe. The add-assign pattern
would be safe because no weak processing will happen on the collection when
we have a pointer into its backing store on the stack. get-set will be safe
because destructors don't add anything.

The same considerations apply to iterators. If you are iterating over a
collection and performing allocations during iteration, you have to make
sure that destructors will not explicitly modify the collection you are
iterating over. The same solution applies: don't modify the set in
destructors. Use weak processing if you have to. Iteration over weak
collections is safe because the iterator is a pointer into the backing
store and therefore no weak processing will happen.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[wibling-chromium, 2014-04-09 07:38:31]

A revert of this CL has been created in
https://codereview.chromium.org/230303002/ by wibling@chromium.org.

The reason for reverting is: Broke the build....

[tkent, 2014-04-09 07:48:34]

On 2014/04/09 07:28:16, Mads Ager (chromium) wrote:
> The WeakMember case is actually no more complicated than the Member case.
> For these patterns WeakMember will be exactly the same as Member because
> there is a pointer on the stack into the collection backing store. When the
> garbage collector finds a pointer into the backing store of a collection
> with WeakMembers it will treat those WeakMembers as strong pointers and
> they will not die. Therefore, weak processing will not change the
> collection. So, weakness doesn't add any extra complications. The two cases
> are exactly the same. The issue is only with the add/new pattern *if* a
> destructor can mess with that collection.

Oh, I see.  I noticed deleting a weak-referred object didn't cause rehash.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[wibling-chromium, 2014-04-10 06:44:03]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-04-10 06:44:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wibling@chromium.org/227083006/100001

[commit-bot: I haz the power, 2014-04-10 06:44:45]

Change committed as 171236

[wibling-chromium, 2014-04-10 08:46:05]

A revert of this CL has been created in
https://codereview.chromium.org/232723002/ by wibling@chromium.org.

The reason for reverting is: Breaking webkit unit tests....

[wibling-chromium, 2014-04-10 11:33:54]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-04-10 11:34:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wibling@chromium.org/227083006/140001

[commit-bot: I haz the power, 2014-04-10 12:01:13]

Change committed as 171265