Oilpan: move custom element objects to the heap.

Oilpan: move custom element objects to the heap.

R=ager@chromium.org,haraken@chromium.org,dominicc@chromium.org
BUG=357163
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174948

[sof, 2014-05-22 07:49:14]

Please take a look.

Points of interest:

 - The mixed use of CustomElementObserver as a part object and as a pointer
value in hash map was a bit cumbersome to work with. Simplified by moving it
away from being a part object.
 - Minimal changes to HTML Imports made here; the use of
   mutually-recursive WeakPtrs (and whether or not to have HTMLImport be a GCed
class) could be considered later.
 - The chief motivating reason for doing this right now is to be able to move to
using traceable Element members.

NOTE: this will have to be rebased on top of
https://codereview.chromium.org/290333008/ once that lands.

[haraken, 2014-05-22 08:24:09]

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:626: m_registrationContext.clear();

Do we still need to clear m_registrationContext in oilpan builds?

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/DocumentInit.h
File Source/core/dom/DocumentInit.h (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/DocumentInit...
Source/core/dom/DocumentInit.h:84: WeakPtr<Document> m_contextDocument;

Not related to your CL, but this looks strange to me. Why do we want to use a
WeakPtr in an on-stack object?

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementDescriptor.h (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementDescriptor.h:45:
ALLOW_ONLY_INLINE_ALLOCATION();

- I think this should be DISALLOW_ALLOCATION, since CustomElementDescriptor is
used as a part of CustomElementMicrotaskResolutionStep.

- I wonder why this compiles with ALLOW_ONLY_INLINE_ALLOCATION() :)

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementRegistrationContext.cpp (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementRegistrationContext.cpp:115:
context->m_candidates = CustomElementUpgradeCandidateMap::create();

We normally do this kind of initialization in the constructor.

CustomElementRegistrationContext()
    : m_candidates(CustomElementUpgradeCandidateMap::create())
{
}

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.h (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.h:70: typedef
WillBeHeapHashMap<RawPtrWillBeMember<Element>, CustomElementDescriptor>
UpgradeCandidateMap;

Shouldn't the key Element be a weak member?

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.h:73: typedef
WillBeHeapHashMap<CustomElementDescriptor, ElementSet> UnresolvedDefinitionMap;

This looks OK. The ElementSet can be a hash set of strong members, because the
entries of the hash map are explicitly cleared.

[Mads Ager (chromium), 2014-05-22 08:33:27]

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:46: // With Oilpan
enabled, the observer table keeps a weak reference to the
The problem is if we have a strong pointer to the element in the upgrade
candidate map. Then the element will not die while we are alive and we will not
die because the element does not die and therefore there is a strong pointer to
us in a global table?

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.h (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.h:70: typedef
WillBeHeapHashMap<RawPtrWillBeMember<Element>, CustomElementDescriptor>
UpgradeCandidateMap;
Will the m_upgradeCandidates map always end up empty eventually? Otherwise this
looks like it could leak.

The global map from element to custom element observer is:

HashMap<WeakMember<Element>, Member<CustomElementObserver> >

which means that the observer is alive as long as the Element stays alive. Now,
if the CustomElementUpgradeCandidateMap which is a CustomElementObserver keeps
the element alive, we will never collect these.

[haraken, 2014-05-22 08:34:39]

+dominicc

[wibling-chromium, 2014-05-22 09:06:04]

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.h (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.h:73: typedef
WillBeHeapHashMap<CustomElementDescriptor, ElementSet> UnresolvedDefinitionMap;
On 2014/05/22 08:24:10, haraken wrote:
> 
> This looks OK. The ElementSet can be a hash set of strong members, because the
> entries of the hash map are explicitly cleared.

I cannot find where this is explicitly cleared? The elementWasDestroyed method
is only called when the observed element is destroyed which will not happen as
long as we have this hash map with a member pointing to the element.
Generally speaking shouldn't we have purely weak members in observers to ensure
they don't keep the observee alive?

[sof, 2014-05-25 16:30:01]

Sorry about the delay, got busy elsewhere. An interesting issue here wrt weak
hash tables, which several reviewers have picked up on.

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:626: m_registrationContext.clear();
On 2014/05/22 08:24:10, haraken wrote:
> 
> Do we still need to clear m_registrationContext in oilpan builds?

As long as we have a dispose() step, I don't think it hurts. But I've moved in
under the #if !ENABLE(OILPAN) block.

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementDescriptor.h (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementDescriptor.h:45:
ALLOW_ONLY_INLINE_ALLOCATION();
On 2014/05/22 08:24:10, haraken wrote:
> 
> - I think this should be DISALLOW_ALLOCATION, since CustomElementDescriptor is
> used as a part of CustomElementMicrotaskResolutionStep.
> 
> - I wonder why this compiles with ALLOW_ONLY_INLINE_ALLOCATION() :)

I don't understand this comment. A part object doesn't require a call to 'new',
so why would not declaring DISALLOW_ALLOCATION() cause a compilation error? And
as CustomElementDescriptor is used as a value object in the
CustomElementUpgradeCandidateMap::UnresolvedDefinitionMap hash map, it seems
appropriate to use ALLOW_ONLY_INLINE_ALLOCATION().

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementRegistrationContext.cpp (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementRegistrationContext.cpp:115:
context->m_candidates = CustomElementUpgradeCandidateMap::create();
On 2014/05/22 08:24:10, haraken wrote:
> 
> We normally do this kind of initialization in the constructor.
> 
> CustomElementRegistrationContext()
>     : m_candidates(CustomElementUpgradeCandidateMap::create())
> {
> }

Thanks, shifted over.

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:46: // With Oilpan
enabled, the observer table keeps a weak reference to the
On 2014/05/22 08:33:27, Mads Ager (chromium) wrote:
> The problem is if we have a strong pointer to the element in the upgrade
> candidate map. Then the element will not die while we are alive and we will
not
> die because the element does not die and therefore there is a strong pointer
to
> us in a global table?

The registration context (a member on the Document) keeps an upgrade candidate
map. Once the Document is GCed, the candidate map is presumably no longer
root-reachable and it will also be. This seems no different from non-Oilpan, I
think.

What is the semantics of weak hash tables as implemented here? If the key is
weak, does its value keep the key alive if it also references the key? The
validity of _only_ keeping an Element weak pointer on the ElementObserverMap
hinges on it not keeping it alive. If it does, then we ought to have a weak
reference on the UpgradeCandidateMap. If it doesn't, then possibly.

IOW, it'd be good to know the weak hash table semantics here before weak'ifying
Element references in CustomElementUpgradeCandidateMap.

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.h (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.h:73: typedef
WillBeHeapHashMap<CustomElementDescriptor, ElementSet> UnresolvedDefinitionMap;
On 2014/05/22 09:06:05, wibling-chromium wrote:
> On 2014/05/22 08:24:10, haraken wrote:
> > 
> > This looks OK. The ElementSet can be a hash set of strong members, because
the
> > entries of the hash map are explicitly cleared.
> 
> I cannot find where this is explicitly cleared? The elementWasDestroyed method
> is only called when the observed element is destroyed which will not happen as
> long as we have this hash map with a member pointing to the element.
> Generally speaking shouldn't we have purely weak members in observers to
ensure
> they don't keep the observee alive?

Whether or not to use weak references in ElementSet is the same issue as the use
of a weak or strong Element reference for the key in UpgradeCandidateMap.

If we do have to, then we'll have to work out what to use instead of
ListHashSets of weak members :)

[haraken, 2014-05-26 01:21:39]

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementDescriptor.h (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementDescriptor.h:45:
ALLOW_ONLY_INLINE_ALLOCATION();
On 2014/05/25 16:30:01, sof wrote:
> On 2014/05/22 08:24:10, haraken wrote:
> > 
> > - I think this should be DISALLOW_ALLOCATION, since CustomElementDescriptor
is
> > used as a part of CustomElementMicrotaskResolutionStep.
> > 
> > - I wonder why this compiles with ALLOW_ONLY_INLINE_ALLOCATION() :)
> 
> I don't understand this comment. A part object doesn't require a call to
'new',
> so why would not declaring DISALLOW_ALLOCATION() cause a compilation error?
And
> as CustomElementDescriptor is used as a value object in the
> CustomElementUpgradeCandidateMap::UnresolvedDefinitionMap hash map, it seems
> appropriate to use ALLOW_ONLY_INLINE_ALLOCATION().

Sorry, you're right. (I was misunderstanding that ALLOW_ONLY_INLINE_ALLOCATION()
is a stronger restriction than DISALLOW_ALLOCATION().)

[dominicc (has gone to gerrit), 2014-05-26 02:28:58]

I think I have template blindness... and one question inline.

https://codereview.chromium.org/296703009/diff/20001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/296703009/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.h:1371:
RefPtrWillBeMember<CustomElementRegistrationContext> m_registrationContext;
If it helps, one registration context may be associated with many documents.

The registration context doesn't have direct references back to its related
documents and in should not keep documents alive.

https://codereview.chromium.org/296703009/diff/20001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.h (right):

https://codereview.chromium.org/296703009/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.h:53: typedef
WillBeHeapListHashSet<RawPtrWillBeMember<Element> > ElementSet;
Hmm... the idea of the CustomElementUpgradeCandidateMap is that it effectively
has a weak reference to the element. That is what the raw ptr and the
observation is about.

If this becomes a member, it looks like the CEUCM will keep the upgrade
candidates alive, which is not what we want. (It is allowable, but it could
regress performance. In particular, if an element never gets a definition, we're
going to hold onto a whole document etc. as a result, right?)

[Mads Ager (chromium), 2014-05-26 10:08:25]

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:46: // With Oilpan
enabled, the observer table keeps a weak reference to the
On 2014/05/25 16:30:01, sof wrote:
> On 2014/05/22 08:33:27, Mads Ager (chromium) wrote:
> > The problem is if we have a strong pointer to the element in the upgrade
> > candidate map. Then the element will not die while we are alive and we will
> not
> > die because the element does not die and therefore there is a strong pointer
> to
> > us in a global table?
> 
> The registration context (a member on the Document) keeps an upgrade candidate
> map. Once the Document is GCed, the candidate map is presumably no longer
> root-reachable and it will also be. This seems no different from non-Oilpan, I
> think.
> 
> What is the semantics of weak hash tables as implemented here? If the key is
> weak, does its value keep the key alive if it also references the key? The
> validity of _only_ keeping an Element weak pointer on the ElementObserverMap
> hinges on it not keeping it alive. If it does, then we ought to have a weak
> reference on the UpgradeCandidateMap. If it doesn't, then possibly.
> 
> IOW, it'd be good to know the weak hash table semantics here before
weak'ifying
> Element references in CustomElementUpgradeCandidateMap.

That is exactly the right question to ask. :-)

And yes, the semantics of weak maps is that we always mark from Members and if
we end up marking the WeakMember part of the map in that marking we will leave
the mapping in there.

The other style of weak map (where you only mark the value if the key is
reachable) require a fix point algorithm and we don't have them. We could add
them but would prefer not to.

[zerny-chromium, 2014-05-26 11:01:00]

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementDescriptor.h (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementDescriptor.h:45:
ALLOW_ONLY_INLINE_ALLOCATION();
On 2014/05/26 01:21:40, haraken wrote:
> On 2014/05/25 16:30:01, sof wrote:
> > On 2014/05/22 08:24:10, haraken wrote:
> > > 
> > > - I think this should be DISALLOW_ALLOCATION, since
CustomElementDescriptor
> is
> > > used as a part of CustomElementMicrotaskResolutionStep.
> > > 
> > > - I wonder why this compiles with ALLOW_ONLY_INLINE_ALLOCATION() :)
> > 
> > I don't understand this comment. A part object doesn't require a call to
> 'new',
> > so why would not declaring DISALLOW_ALLOCATION() cause a compilation error?
> And
> > as CustomElementDescriptor is used as a value object in the
> > CustomElementUpgradeCandidateMap::UnresolvedDefinitionMap hash map, it seems
> > appropriate to use ALLOW_ONLY_INLINE_ALLOCATION().
> 
> Sorry, you're right. (I was misunderstanding that
ALLOW_ONLY_INLINE_ALLOCATION()
> is a stronger restriction than DISALLOW_ALLOCATION().)

Just to clarify, DISALLOW_ALLOCATION is the stronger requirement. In this case
we want to use CustomeElementDescriptor inline/as-a-value in a collection, so we
need to use ALLOW_ONLY_INLINE_ALLOCATION.

[sof, 2014-05-26 14:21:24]

Switching to a 'weaker' CustomElementUpgradeCandidateMap required changing the
ElementSet representation to a (Heap)LinkedHashSet (of weak Element refs) as
HeapListHashSet over weaks isn't supported. Hope that's an acceptable change in
representation.

The leak counts for the fast/dom/custom tests now stay the same before & after
this CL (with Oilpan enabled.)

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/custom/Custo...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:46: // With Oilpan
enabled, the observer table keeps a weak reference to the
On 2014/05/26 10:08:26, Mads Ager (chromium) wrote:
> On 2014/05/25 16:30:01, sof wrote:
> > On 2014/05/22 08:33:27, Mads Ager (chromium) wrote:
> > > The problem is if we have a strong pointer to the element in the upgrade
> > > candidate map. Then the element will not die while we are alive and we
will
> > not
> > > die because the element does not die and therefore there is a strong
pointer
> > to
> > > us in a global table?
> > 
> > The registration context (a member on the Document) keeps an upgrade
candidate
> > map. Once the Document is GCed, the candidate map is presumably no longer
> > root-reachable and it will also be. This seems no different from non-Oilpan,
I
> > think.
> > 
> > What is the semantics of weak hash tables as implemented here? If the key is
> > weak, does its value keep the key alive if it also references the key? The
> > validity of _only_ keeping an Element weak pointer on the ElementObserverMap
> > hinges on it not keeping it alive. If it does, then we ought to have a weak
> > reference on the UpgradeCandidateMap. If it doesn't, then possibly.
> > 
> > IOW, it'd be good to know the weak hash table semantics here before
> weak'ifying
> > Element references in CustomElementUpgradeCandidateMap.
> 
> That is exactly the right question to ask. :-)
> 
> And yes, the semantics of weak maps is that we always mark from Members and if
> we end up marking the WeakMember part of the map in that marking we will leave
> the mapping in there.
> 
> The other style of weak map (where you only mark the value if the key is
> reachable) require a fix point algorithm and we don't have them. We could add
> them but would prefer not to.

Thanks, that's really useful information. So all the Element references from
CustomElementUpgradeCandidateMap need to be weak to prevent Element being
retained when they shouldn't be.

https://codereview.chromium.org/296703009/diff/20001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.h (right):

https://codereview.chromium.org/296703009/diff/20001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.h:53: typedef
WillBeHeapListHashSet<RawPtrWillBeMember<Element> > ElementSet;
On 2014/05/26 02:28:58, dominicc wrote:
> Hmm... the idea of the CustomElementUpgradeCandidateMap is that it effectively
> has a weak reference to the element. That is what the raw ptr and the
> observation is about.
> 

Thanks, the weakness of that reference must stay the same with Oilpan. There was
a question whether or not it was effectively weak already, but not so.

> If this becomes a member, it looks like the CEUCM will keep the upgrade
> candidates alive, which is not what we want. (It is allowable, but it could
> regress performance. In particular, if an element never gets a definition,
we're
> going to hold onto a whole document etc. as a result, right?)

I've added a test to verify non-leaking for that scenario (and verfied that it
leaked if the unresolved Element references were kept "strong".)

[Mads Ager (chromium), 2014-05-26 14:30:38]

LGTM

HeapLinkedHashSet and HeapListHashSet should perform equally well so this should
be fine.

HeapLinkedHashSet supports weakness but does not support modification during
iteration.

HeapListHashSet supports modification during iteration but does not support
weakness.

So, the only thing you have to consider here is whether this set is currently
change during iteration. :)

[sof, 2014-05-26 14:36:52]

On 2014/05/26 14:30:38, Mads Ager (chromium) wrote:
> LGTM
> 
> HeapLinkedHashSet and HeapListHashSet should perform equally well so this
should
> be fine.
> 
> HeapLinkedHashSet supports weakness but does not support modification during
> iteration.
> 
> HeapListHashSet supports modification during iteration but does not support
> weakness.
> 
> So, the only thing you have to consider here is whether this set is currently
> change during iteration. :)

Double-checked; the iteration that happens in registerElement() doesn't modify
the set, nor do the iterations that happen internal to
CustomElementUpgradeCandidateMap.

[haraken, 2014-05-26 15:40:23]

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:92: for
(WillBeHeapVector<RawPtrWillBeMember<CustomElementCallbackQueue> >::iterator it
= m_elements.begin();it != m_elements.end(); ++it) {

Nit: One space needed after "begin();"

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskResolutionStep.h (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskResolutionStep.h:67:
CustomElementDescriptor m_descriptor;

You need to trace this one, or you can simply drop ALLOW_ONLY_INLINE_ALLOCATION
from  CustomElementDescriptor since CustomElementDescriptor has no members to be
traced.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementObserver.cpp (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementObserver.cpp:40: typedef
WillBeHeapHashMap<RawPtrWillBeWeakMember<Element>,
RawPtrWillBeMember<CustomElementObserver> > ElementObserverMap;

I think you need to remove the following lines from Element's destructor:

Element::~Element() {
  ...;
  if (isCustomElement())
    CustomElement::wasDestroyed(this);
  ...;
}

CustomElement::wasDestroyed calls
CustomElementUpgradeCandidateMap::elementWasDestroyed and thus touches on-heap
objects. I guess this will cause an ASan issue.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:50: unobserve(*it);

Just help me understand: As far as I see the code, it looks like we want to
remove CustomElementObserver from ElementObserverMap (in
CustomElementObserver.cpp) when the CustomElementObserver dies. Then, should the
ElementObserverMap be a map from a WeakMember<Element> to a
WeakMember<CustomElementObserver> ? Currently it's a map from a
WeakMember<Element> to a Member<CustomElementObserver>.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:70: void
CustomElementUpgradeCandidateMap::remove(Element* element)

Who calls this method? As far as I see the code search, I couldn't find any
caller. If so, we can remove this method.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:90:
m_upgradeCandidates.remove(candidate);

Can this be candidate->remove() ? We won't want to look up the hash table twice.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:115: return 0;

Can this happen? If no, we might want to keep ElementSet&.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.h (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.h:73: typedef
WillBeHeapHashMap<CustomElementDescriptor, OwnPtrWillBeMember<ElementSet> >
UnresolvedDefinitionMap;

I'm curious why you didn't use WillBeHeapHashMap<CustomElementDescriptor,
ElementSet>. Then you can keep using ElementSet& in several places in this CL.

[sof, 2014-05-26 15:46:17]

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/DocumentInit.h
File Source/core/dom/DocumentInit.h (right):

https://codereview.chromium.org/296703009/diff/1/Source/core/dom/DocumentInit...
Source/core/dom/DocumentInit.h:84: WeakPtr<Document> m_contextDocument;
On 2014/05/22 08:24:10, haraken wrote:
> 
> Not related to your CL, but this looks strange to me. Why do we want to use a
> WeakPtr in an on-stack object?

Will follow up on this separately (along with the other occurrences of
Document.)

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:115: return 0;
On 2014/05/26 15:40:23, haraken wrote:
> 
> Can this happen? If no, we might want to keep ElementSet&.

It will happen if you call registerElement() before there have been any uses of
the custom element.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.h (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.h:73: typedef
WillBeHeapHashMap<CustomElementDescriptor, OwnPtrWillBeMember<ElementSet> >
UnresolvedDefinitionMap;
On 2014/05/26 15:40:23, haraken wrote:
> 
> I'm curious why you didn't use WillBeHeapHashMap<CustomElementDescriptor,
> ElementSet>. Then you can keep using ElementSet& in several places in this CL.

LinkedHashSet<>s are not allowed used as inline value objects. 4 pages of
template type errors told me that :)

[sof, 2014-05-26 16:41:07]

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:92: for
(WillBeHeapVector<RawPtrWillBeMember<CustomElementCallbackQueue> >::iterator it
= m_elements.begin();it != m_elements.end(); ++it) {
On 2014/05/26 15:40:23, haraken wrote:
> 
> Nit: One space needed after "begin();"

Tidied.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskResolutionStep.h (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskResolutionStep.h:67:
CustomElementDescriptor m_descriptor;
On 2014/05/26 15:40:23, haraken wrote:
> 
> You need to trace this one, or you can simply drop
ALLOW_ONLY_INLINE_ALLOCATION
> from  CustomElementDescriptor since CustomElementDescriptor has no members to
be
> traced.

If the object doesn't have any Members (CustomElementDescriptor doesn't), it
doesn't need a trace() method. Separately adding ALLOW_ONLY_INLINE_ALOCATION()
is useful to constrain use, I think.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementObserver.cpp (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementObserver.cpp:40: typedef
WillBeHeapHashMap<RawPtrWillBeWeakMember<Element>,
RawPtrWillBeMember<CustomElementObserver> > ElementObserverMap;
On 2014/05/26 15:40:23, haraken wrote:
> 
> I think you need to remove the following lines from Element's destructor:
> 
> Element::~Element() {
>   ...;
>   if (isCustomElement())
>     CustomElement::wasDestroyed(this);
>   ...;
> }
> 
> CustomElement::wasDestroyed calls
> CustomElementUpgradeCandidateMap::elementWasDestroyed and thus touches on-heap
> objects. I guess this will cause an ASan issue.

Very good catch; made it conditional on !ENABLE(OILPAN).

The weak reference on the other side will remove/unregister the object during
the same GC.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:50: unobserve(*it);
On 2014/05/26 15:40:23, haraken wrote:
> 
> Just help me understand: As far as I see the code, it looks like we want to
> remove CustomElementObserver from ElementObserverMap (in
> CustomElementObserver.cpp) when the CustomElementObserver dies. Then, should
the
> ElementObserverMap be a map from a WeakMember<Element> to a
> WeakMember<CustomElementObserver> ? Currently it's a map from a
> WeakMember<Element> to a Member<CustomElementObserver>.

The weakly held Elements keep the CustomElementObserver objects alive. In
non-Oilpan builds, this is triggered from ~Element, releasing the corresponding
*Observer and this dtor gets to run.

Why do we want to independently make that weak, when we already have the key as
being weak? I don't understand what you are asking/suggesting, I think.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:70: void
CustomElementUpgradeCandidateMap::remove(Element* element)
On 2014/05/26 15:40:23, haraken wrote:
> 
> Who calls this method? As far as I see the code search, I couldn't find any
> caller. If so, we can remove this method.

You're quite right, unused. Nice, removed and simplified.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:90:
m_upgradeCandidates.remove(candidate);
On 2014/05/26 15:40:23, haraken wrote:
> 
> Can this be candidate->remove() ? We won't want to look up the hash table
twice.
> 

Can't be - this is removing it from another table.

[sof, 2014-05-26 19:25:36]

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementObserver.cpp (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementObserver.cpp:40: typedef
WillBeHeapHashMap<RawPtrWillBeWeakMember<Element>,
RawPtrWillBeMember<CustomElementObserver> > ElementObserverMap;
On 2014/05/26 16:41:08, sof wrote:
> On 2014/05/26 15:40:23, haraken wrote:
> > 
> > I think you need to remove the following lines from Element's destructor:
> > 
> > Element::~Element() {
> >   ...;
> >   if (isCustomElement())
> >     CustomElement::wasDestroyed(this);
> >   ...;
> > }
> > 
> > CustomElement::wasDestroyed calls
> > CustomElementUpgradeCandidateMap::elementWasDestroyed and thus touches
on-heap
> > objects. I guess this will cause an ASan issue.
> 
> Very good catch; made it conditional on !ENABLE(OILPAN).
> 
> The weak reference on the other side will remove/unregister the object during
> the same GC.

Just to continue that thought & looking at details some more.. the weak
reference to the Element is cleared out before the sweep and Element
finalization, so CustomElementObserver::notifyElementWasDestroyed() will return
early and no possibly-finalized heap objects will be touched here.

So, while clearly wrong to trigger the wasDestroyed() call in ~Element, it is
not unsafe, just redundant (with the current implementation.)

[haraken, 2014-05-27 01:08:44]

Thanks for the update! Here is a final round of comments.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:50: unobserve(*it);
On 2014/05/26 16:41:08, sof wrote:
> On 2014/05/26 15:40:23, haraken wrote:
> > 
> > Just help me understand: As far as I see the code, it looks like we want to
> > remove CustomElementObserver from ElementObserverMap (in
> > CustomElementObserver.cpp) when the CustomElementObserver dies. Then, should
> the
> > ElementObserverMap be a map from a WeakMember<Element> to a
> > WeakMember<CustomElementObserver> ? Currently it's a map from a
> > WeakMember<Element> to a Member<CustomElementObserver>.
> 
> The weakly held Elements keep the CustomElementObserver objects alive. In
> non-Oilpan builds, this is triggered from ~Element, releasing the
corresponding
> *Observer and this dtor gets to run.
> 
> Why do we want to independently make that weak, when we already have the key
as
> being weak? I don't understand what you are asking/suggesting, I think.

Correct me if I'm wrong:

> The weakly held Elements keep the CustomElementObserver objects alive. In
> non-Oilpan builds, this is triggered from ~Element, releasing the
corresponding
> *Observer and this dtor gets to run.

This code is not triggered from ~Element.

(a) When an Element dies, CustomElementUpgradeCandidateMap::remove is triggered
and the Element is unregistered from the ElementObserverMap.

(b) When an CustomElementObserver dies, ~CustomElementUpgradeCandidateMap is
triggered and the Elements observed by the CustomElementObserver are
unregistered from the ElementObserverMap.

Making the key of the ElementObserverMap weak takes care of the weakness for
(a), and making the value of the ElementObserverMap weak takes care of the
weakness for (b). Thus I think both key and value of the ElementObserverMap
needs to be weak.

What do you think?

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:115: return 0;
On 2014/05/26 15:46:18, sof wrote:
> On 2014/05/26 15:40:23, haraken wrote:
> > 
> > Can this happen? If no, we might want to keep ElementSet&.
> 
> It will happen if you call registerElement() before there have been any uses
of
> the custom element.

Just help me understand: Before this CL, we were using ListHashSet&, which means
that it can't be 0. After this CL, it can be 0. Where does the difference come
from?

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp (right):

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:36: #endif

Ditto. I'd prefer DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent...)

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskDispatcher.h (right):

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.h:21: class
CustomElementMicrotaskDispatcher FINAL : public
NoBaseWillBeGarbageCollectedFinalized<CustomElementMicrotaskDispatcher> {

This can be NoBaseWillBeGarbageCollected.

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskQueue.h (right):

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskQueue.h:45: class
CustomElementMicrotaskQueue FINAL : public
RefCountedWillBeGarbageCollectedFinalized<CustomElementMicrotaskQueue> {

This can be RefCountedWillBeGarbageCollected.

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementObserver.cpp (right):

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementObserver.cpp:50: #endif

I'd prefer:

DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<ElementObserverMap>, map, (new
ElementObserverMap));
return *map;

to avoid the #if branch.

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementScheduler.cpp (right):

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementScheduler.cpp:105: #endif

Ditto. I'd prefer DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent...)

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementScheduler.h (right):

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementScheduler.h:49: class CustomElementScheduler
FINAL : public NoBaseWillBeGarbageCollectedFinalized<CustomElementScheduler> {

This can be NoBaseWillBeGarbageCollected.

[sof, 2014-05-27 05:29:50]

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:50: unobserve(*it);
On 2014/05/27 01:08:45, haraken wrote:
> On 2014/05/26 16:41:08, sof wrote:
> > On 2014/05/26 15:40:23, haraken wrote:
> > > 
> > > Just help me understand: As far as I see the code, it looks like we want
to
> > > remove CustomElementObserver from ElementObserverMap (in
> > > CustomElementObserver.cpp) when the CustomElementObserver dies. Then,
should
> > the
> > > ElementObserverMap be a map from a WeakMember<Element> to a
> > > WeakMember<CustomElementObserver> ? Currently it's a map from a
> > > WeakMember<Element> to a Member<CustomElementObserver>.
> > 
> > The weakly held Elements keep the CustomElementObserver objects alive. In
> > non-Oilpan builds, this is triggered from ~Element, releasing the
> corresponding
> > *Observer and this dtor gets to run.
> > 
> > Why do we want to independently make that weak, when we already have the key
> as
> > being weak? I don't understand what you are asking/suggesting, I think.
> 
> Correct me if I'm wrong:
> 
> > The weakly held Elements keep the CustomElementObserver objects alive. In
> > non-Oilpan builds, this is triggered from ~Element, releasing the
> corresponding
> > *Observer and this dtor gets to run.
> 
> This code is not triggered from ~Element.
> 

It is triggered by the call to wasDestroyed() in ~Element, which brings about
deletion

> (a) When an Element dies, CustomElementUpgradeCandidateMap::remove is
triggered
> and the Element is unregistered from the ElementObserverMap.
> 
> (b) When an CustomElementObserver dies, ~CustomElementUpgradeCandidateMap is
> triggered and the Elements observed by the CustomElementObserver are
> unregistered from the ElementObserverMap.
> 
> Making the key of the ElementObserverMap weak takes care of the weakness for
> (a), and making the value of the ElementObserverMap weak takes care of the
> weakness for (b). Thus I think both key and value of the ElementObserverMap
> needs to be weak.
> 
> What do you think?

- Who holds onto the CustomElementObservers? ElementObserverMap.
- What is keeping it alive? The weak Element reference. 

Consequently, when the Element dies, the CustomElementObserver (which can be a
CustomElementUpgradeCandidateMap) is retired. i.e., a weak key in a hash table
will also remove its value.

I don't understand what it means to have weak value here or why you think it is
needed.

(This is triggered from ~Element in the non-Oilpan case.)

[haraken, 2014-05-27 05:47:29]

LGTM

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:50: unobserve(*it);
On 2014/05/27 05:29:51, sof wrote:
> On 2014/05/27 01:08:45, haraken wrote:
> > On 2014/05/26 16:41:08, sof wrote:
> > > On 2014/05/26 15:40:23, haraken wrote:
> > > > 
> > > > Just help me understand: As far as I see the code, it looks like we want
> to
> > > > remove CustomElementObserver from ElementObserverMap (in
> > > > CustomElementObserver.cpp) when the CustomElementObserver dies. Then,
> should
> > > the
> > > > ElementObserverMap be a map from a WeakMember<Element> to a
> > > > WeakMember<CustomElementObserver> ? Currently it's a map from a
> > > > WeakMember<Element> to a Member<CustomElementObserver>.
> > > 
> > > The weakly held Elements keep the CustomElementObserver objects alive. In
> > > non-Oilpan builds, this is triggered from ~Element, releasing the
> > corresponding
> > > *Observer and this dtor gets to run.
> > > 
> > > Why do we want to independently make that weak, when we already have the
key
> > as
> > > being weak? I don't understand what you are asking/suggesting, I think.
> > 
> > Correct me if I'm wrong:
> > 
> > > The weakly held Elements keep the CustomElementObserver objects alive. In
> > > non-Oilpan builds, this is triggered from ~Element, releasing the
> > corresponding
> > > *Observer and this dtor gets to run.
> > 
> > This code is not triggered from ~Element.
> > 
> 
> It is triggered by the call to wasDestroyed() in ~Element, which brings about
> deletion

Sorry, I was misreading the call path to CustomElementUpgradeCandidateMap's
destructor. You're right; CustomElementUpgradeCandidateMap's destructor is
called in ~Element(), and then making the key of ElementObserverMap weak is
enough. Makes sense, and I'm sorry about the noise.

[sof, 2014-05-27 11:59:13]

dominicc: some representation changes made in the end to
CustomElementUpgradeCandidateMap (non Oilpan), are they acceptable?

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp (right):

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:50: unobserve(*it);
On 2014/05/27 05:47:30, haraken wrote:
> On 2014/05/27 05:29:51, sof wrote:
> > On 2014/05/27 01:08:45, haraken wrote:
> > > On 2014/05/26 16:41:08, sof wrote:
> > > > On 2014/05/26 15:40:23, haraken wrote:
> > > > > 
> > > > > Just help me understand: As far as I see the code, it looks like we
want
> > to
> > > > > remove CustomElementObserver from ElementObserverMap (in
> > > > > CustomElementObserver.cpp) when the CustomElementObserver dies. Then,
> > should
> > > > the
> > > > > ElementObserverMap be a map from a WeakMember<Element> to a
> > > > > WeakMember<CustomElementObserver> ? Currently it's a map from a
> > > > > WeakMember<Element> to a Member<CustomElementObserver>.
> > > > 
> > > > The weakly held Elements keep the CustomElementObserver objects alive.
In
> > > > non-Oilpan builds, this is triggered from ~Element, releasing the
> > > corresponding
> > > > *Observer and this dtor gets to run.
> > > > 
> > > > Why do we want to independently make that weak, when we already have the
> key
> > > as
> > > > being weak? I don't understand what you are asking/suggesting, I think.
> > > 
> > > Correct me if I'm wrong:
> > > 
> > > > The weakly held Elements keep the CustomElementObserver objects alive.
In
> > > > non-Oilpan builds, this is triggered from ~Element, releasing the
> > > corresponding
> > > > *Observer and this dtor gets to run.
> > > 
> > > This code is not triggered from ~Element.
> > > 
> > 
> > It is triggered by the call to wasDestroyed() in ~Element, which brings
about
> > deletion
> 
> Sorry, I was misreading the call path to CustomElementUpgradeCandidateMap's
> destructor. You're right; CustomElementUpgradeCandidateMap's destructor is
> called in ~Element(), and then making the key of ElementObserverMap weak is
> enough. Makes sense, and I'm sorry about the noise.

That matches my understanding also. I've been comparing wrt ToT in terms of
leaks when doing this CL, and see no difference.

https://codereview.chromium.org/296703009/diff/50001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:115: return 0;
On 2014/05/27 01:08:45, haraken wrote:
> On 2014/05/26 15:46:18, sof wrote:
> > On 2014/05/26 15:40:23, haraken wrote:
> > > 
> > > Can this happen? If no, we might want to keep ElementSet&.
> > 
> > It will happen if you call registerElement() before there have been any uses
> of
> > the custom element.
> 
> Just help me understand: Before this CL, we were using ListHashSet&, which
means
> that it can't be 0. After this CL, it can be 0. Where does the difference come
> from?

hashSet.take()'s return type is HashTraits<ValueType>::PassOutType. If the
element is not present, it returns the value
HashTraits<ValueType>::emptyValue().

With HashMap<CustomElementDescriptor, ElementSet> that type is equal to
ElementSet and the value returned is ElementSet() (i.e, an empty hash set.)

With HashMap<CustomElementDescriptor, OwnPtrWillBeMember<ElementSet> > that type
is equal to either PassOwnPtr<ElementSet> or ElementSet*, returning nullptr.

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp (right):

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:36: #endif
On 2014/05/27 01:08:45, haraken wrote:
> 
> Ditto. I'd prefer DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent...)

Done, here & elsewhere.

(Where does this style come from? Seems like a recent change in preference..and
it can be tidied up a bit further by way of a macro, I think. But for later.)

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementMicrotaskDispatcher.h (right):

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementMicrotaskDispatcher.h:21: class
CustomElementMicrotaskDispatcher FINAL : public
NoBaseWillBeGarbageCollectedFinalized<CustomElementMicrotaskDispatcher> {
On 2014/05/27 01:08:45, haraken wrote:
> 
> This can be NoBaseWillBeGarbageCollected.

Done.

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
File Source/core/dom/custom/CustomElementScheduler.h (right):

https://codereview.chromium.org/296703009/diff/90001/Source/core/dom/custom/C...
Source/core/dom/custom/CustomElementScheduler.h:49: class CustomElementScheduler
FINAL : public NoBaseWillBeGarbageCollectedFinalized<CustomElementScheduler> {
On 2014/05/27 01:08:45, haraken wrote:
> 
> This can be NoBaseWillBeGarbageCollected.

Done.

[dominicc (has gone to gerrit), 2014-05-28 07:57:44]

Sure. I didn't follow all the twists and turns of why these are necessary
but where you have ended up LGTM.


On Tue, May 27, 2014 at 8:59 PM, <sigbjornf@opera.com> wrote:

> dominicc: some representation changes made in the end to
> CustomElementUpgradeCandidateMap (non Oilpan), are they acceptable?
>
>
>
> https://codereview.chromium.org/296703009/diff/50001/
> Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp
> File Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp
> (right):
>
> https://codereview.chromium.org/296703009/diff/50001/
> Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp#newcode50
> Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:50:
> unobserve(*it);
> On 2014/05/27 05:47:30, haraken wrote:
>
>> On 2014/05/27 05:29:51, sof wrote:
>> > On 2014/05/27 01:08:45, haraken wrote:
>> > > On 2014/05/26 16:41:08, sof wrote:
>> > > > On 2014/05/26 15:40:23, haraken wrote:
>> > > > >
>> > > > > Just help me understand: As far as I see the code, it looks
>>
> like we want
>
>> > to
>> > > > > remove CustomElementObserver from ElementObserverMap (in
>> > > > > CustomElementObserver.cpp) when the CustomElementObserver
>>
> dies. Then,
>
>> > should
>> > > > the
>> > > > > ElementObserverMap be a map from a WeakMember<Element> to a
>> > > > > WeakMember<CustomElementObserver> ? Currently it's a map from
>>
> a
>
>> > > > > WeakMember<Element> to a Member<CustomElementObserver>.
>> > > >
>> > > > The weakly held Elements keep the CustomElementObserver objects
>>
> alive. In
>
>> > > > non-Oilpan builds, this is triggered from ~Element, releasing
>>
> the
>
>> > > corresponding
>> > > > *Observer and this dtor gets to run.
>> > > >
>> > > > Why do we want to independently make that weak, when we already
>>
> have the
>
>> key
>> > > as
>> > > > being weak? I don't understand what you are asking/suggesting, I
>>
> think.
>
>> > >
>> > > Correct me if I'm wrong:
>> > >
>> > > > The weakly held Elements keep the CustomElementObserver objects
>>
> alive. In
>
>> > > > non-Oilpan builds, this is triggered from ~Element, releasing
>>
> the
>
>> > > corresponding
>> > > > *Observer and this dtor gets to run.
>> > >
>> > > This code is not triggered from ~Element.
>> > >
>> >
>> > It is triggered by the call to wasDestroyed() in ~Element, which
>>
> brings about
>
>> > deletion
>>
>
>  Sorry, I was misreading the call path to
>>
> CustomElementUpgradeCandidateMap's
>
>> destructor. You're right; CustomElementUpgradeCandidateMap's
>>
> destructor is
>
>> called in ~Element(), and then making the key of ElementObserverMap
>>
> weak is
>
>> enough. Makes sense, and I'm sorry about the noise.
>>
>
> That matches my understanding also. I've been comparing wrt ToT in terms
> of leaks when doing this CL, and see no difference.
>
>
> https://codereview.chromium.org/296703009/diff/50001/
> Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp#newcode115
> Source/core/dom/custom/CustomElementUpgradeCandidateMap.cpp:115: return
> 0;
> On 2014/05/27 01:08:45, haraken wrote:
>
>> On 2014/05/26 15:46:18, sof wrote:
>> > On 2014/05/26 15:40:23, haraken wrote:
>> > >
>> > > Can this happen? If no, we might want to keep ElementSet&.
>> >
>> > It will happen if you call registerElement() before there have been
>>
> any uses
>
>> of
>> > the custom element.
>>
>
>  Just help me understand: Before this CL, we were using ListHashSet&,
>>
> which means
>
>> that it can't be 0. After this CL, it can be 0. Where does the
>>
> difference come
>
>> from?
>>
>
> hashSet.take()'s return type is HashTraits<ValueType>::PassOutType. If
> the element is not present, it returns the value
> HashTraits<ValueType>::emptyValue().
>
> With HashMap<CustomElementDescriptor, ElementSet> that type is equal to
> ElementSet and the value returned is ElementSet() (i.e, an empty hash
> set.)
>
> With HashMap<CustomElementDescriptor, OwnPtrWillBeMember<ElementSet> >
> that type is equal to either PassOwnPtr<ElementSet> or ElementSet*,
> returning nullptr.
>
>
> https://codereview.chromium.org/296703009/diff/90001/
> Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp
> File Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp
> (right):
>
> https://codereview.chromium.org/296703009/diff/90001/
> Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp#newcode36
> Source/core/dom/custom/CustomElementMicrotaskDispatcher.cpp:36: #endif
> On 2014/05/27 01:08:45, haraken wrote:
>
>  Ditto. I'd prefer DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent...)
>>
>
> Done, here & elsewhere.
>
> (Where does this style come from? Seems like a recent change in
> preference..and it can be tidied up a bit further by way of a macro, I
> think. But for later.)
>
>
> https://codereview.chromium.org/296703009/diff/90001/
> Source/core/dom/custom/CustomElementMicrotaskDispatcher.h
> File Source/core/dom/custom/CustomElementMicrotaskDispatcher.h (right):
>
> https://codereview.chromium.org/296703009/diff/90001/
> Source/core/dom/custom/CustomElementMicrotaskDispatcher.h#newcode21
> Source/core/dom/custom/CustomElementMicrotaskDispatcher.h:21: class
> CustomElementMicrotaskDispatcher FINAL : public
> NoBaseWillBeGarbageCollectedFinalized<CustomElementMicrotaskDispatcher>
> {
> On 2014/05/27 01:08:45, haraken wrote:
>
>  This can be NoBaseWillBeGarbageCollected.
>>
>
> Done.
>
>
> https://codereview.chromium.org/296703009/diff/90001/
> Source/core/dom/custom/CustomElementScheduler.h
> File Source/core/dom/custom/CustomElementScheduler.h (right):
>
> https://codereview.chromium.org/296703009/diff/90001/
> Source/core/dom/custom/CustomElementScheduler.h#newcode49
> Source/core/dom/custom/CustomElementScheduler.h:49: class
> CustomElementScheduler FINAL : public
> NoBaseWillBeGarbageCollectedFinalized<CustomElementScheduler> {
> On 2014/05/27 01:08:45, haraken wrote:
>
>  This can be NoBaseWillBeGarbageCollected.
>>
>
> Done.
>
> https://codereview.chromium.org/296703009/
>

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

[sof, 2014-05-28 08:09:31]

On 2014/05/28 07:57:44, dominicc wrote:
> Sure. I didn't follow all the twists and turns of why these are necessary
> but where you have ended up LGTM.
> 

Thanks; I'll watch the perf bots.

[sof, 2014-05-28 08:09:59]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-05-28 08:10:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/296703009/140001

[dominicc (has gone to gerrit), 2014-05-28 08:11:43]

lgtm

[dominicc (has gone to gerrit), 2014-05-28 08:11:46]

lgtm

[commit-bot: I haz the power, 2014-05-28 09:03:53]

Change committed as 174948