Oilpan: Prepare moving AXObject to heap

Oilpan: Prepare moving AXObject to heap

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

[keishi, 2015-04-28 06:20:37]

Depends on https://codereview.chromium.org/1081673003/

[keishi, 2015-04-28 06:20:54]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org

[dmazzoni, 2015-04-28 06:53:24]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2015-04-28 06:53:25]

Maybe you've already got this figured out and I don't fully understand the new
logic, but here are the first questions I have.

https://codereview.chromium.org/1072273006/diff/60001/Source/core/dom/AXObjec...
File Source/core/dom/AXObjectCache.h (right):

https://codereview.chromium.org/1072273006/diff/60001/Source/core/dom/AXObjec...
Source/core/dom/AXObjectCache.h:161: OwnPtr<AXObjectCache> m_ownedCache;
any reason not to just use m_isOwned for the non-oilpan case too, for fewer
ifdefs?

https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXNodeObject.h (right):

https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.h:207: RawPtrWillBeWeakMember<Node>
m_node;
So this is safe from use-after-free, but I think it will leak AXNodeObjects.
Currently we rely on knowing when the Node is removed from the document to
actually call detach() on AXNodeObject. We need some sort of equivalent - a
callback so that we can detach an AXNodeObject when it no longer points to a
valid node, and eventually delete it from the HashMap.

https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXObjectCacheImpl.h (right):

https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObjectCacheImpl.h:142: void remove(AXObject*);
Can we make this change for the non-oilpan compile just so there aren't two code
paths?

https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObjectCacheImpl.h:198:
HeapHashMap<WeakMember<Node>, Member<AXObject>> m_nodeObjectMapping;
I'm worried about WeakMember<Node> here. We rely on getting a call to
AXObjectCache::remove(Node*) from Node::willBeDeletedFromDocument currently.
With this change, it looks like that call won't happen, so we'll end up with an
entry in the HashMap with a null key and the AXObject won't detach itself.

[haraken, 2015-04-28 06:54:28]

On 2015/04/28 06:53:25, dmazzoni wrote:
> Maybe you've already got this figured out and I don't fully understand the new
> logic, but here are the first questions I have.
> 
>
https://codereview.chromium.org/1072273006/diff/60001/Source/core/dom/AXObjec...
> File Source/core/dom/AXObjectCache.h (right):
> 
>
https://codereview.chromium.org/1072273006/diff/60001/Source/core/dom/AXObjec...
> Source/core/dom/AXObjectCache.h:161: OwnPtr<AXObjectCache> m_ownedCache;
> any reason not to just use m_isOwned for the non-oilpan case too, for fewer
> ifdefs?
> 
>
https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
> File Source/modules/accessibility/AXNodeObject.h (right):
> 
>
https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
> Source/modules/accessibility/AXNodeObject.h:207: RawPtrWillBeWeakMember<Node>
> m_node;
> So this is safe from use-after-free, but I think it will leak AXNodeObjects.
> Currently we rely on knowing when the Node is removed from the document to
> actually call detach() on AXNodeObject. We need some sort of equivalent - a
> callback so that we can detach an AXNodeObject when it no longer points to a
> valid node, and eventually delete it from the HashMap.
> 
>
https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
> File Source/modules/accessibility/AXObjectCacheImpl.h (right):
> 
>
https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
> Source/modules/accessibility/AXObjectCacheImpl.h:142: void remove(AXObject*);
> Can we make this change for the non-oilpan compile just so there aren't two
code
> paths?
> 
>
https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
> Source/modules/accessibility/AXObjectCacheImpl.h:198:
> HeapHashMap<WeakMember<Node>, Member<AXObject>> m_nodeObjectMapping;
> I'm worried about WeakMember<Node> here. We rely on getting a call to
> AXObjectCache::remove(Node*) from Node::willBeDeletedFromDocument currently.
> With this change, it looks like that call won't happen, so we'll end up with
an
> entry in the HashMap with a null key and the AXObject won't detach itself.

(BTW, thanks for reviewing the complicated CL, dominic!)

[keishi, 2015-05-28 06:49:19]

https://codereview.chromium.org/1072273006/diff/60001/Source/core/dom/AXObjec...
File Source/core/dom/AXObjectCache.h (right):

https://codereview.chromium.org/1072273006/diff/60001/Source/core/dom/AXObjec...
Source/core/dom/AXObjectCache.h:161: OwnPtr<AXObjectCache> m_ownedCache;
On 2015/04/28 06:53:25, dmazzoni wrote:
> any reason not to just use m_isOwned for the non-oilpan case too, for fewer
> ifdefs?
> 

Nope removed.

https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXNodeObject.h (right):

https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.h:207: RawPtrWillBeWeakMember<Node>
m_node;
On 2015/04/28 06:53:25, dmazzoni wrote:
> So this is safe from use-after-free, but I think it will leak AXNodeObjects.
> Currently we rely on knowing when the Node is removed from the document to
> actually call detach() on AXNodeObject. We need some sort of equivalent - a
> callback so that we can detach an AXNodeObject when it no longer points to a
> valid node, and eventually delete it from the HashMap.

Like I answered in my reply for the m_nodeObjectMapping part, I think
AXNodeObject::detach is called when AXObjectCacheImpl::remove is called from
AXObjectCacheImpl::clearWeakMembers.

https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXObjectCacheImpl.h (right):

https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObjectCacheImpl.h:142: void remove(AXObject*);
On 2015/04/28 06:53:25, dmazzoni wrote:
> Can we make this change for the non-oilpan compile just so there aren't two
code
> paths?

Done.

https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObjectCacheImpl.h:198:
HeapHashMap<WeakMember<Node>, Member<AXObject>> m_nodeObjectMapping;
On 2015/04/28 06:53:25, dmazzoni wrote:
> I'm worried about WeakMember<Node> here. We rely on getting a call to
> AXObjectCache::remove(Node*) from Node::willBeDeletedFromDocument currently.
> With this change, it looks like that call won't happen, so we'll end up with
an
> entry in the HashMap with a null key and the AXObject won't detach itself.

HeapHashMap with WeakMember entries will automatically be removed by weak
processing.
However, in this case we need to call AXNodeObject::detach for the removed nodes
so I am doing custom weak processing.
I've registered AXObjectCacheImpl::clearWeakMembers as a weak processing
callback using registerWeakMembers.
AXObjectCacheImpl::clearWeakMembers scans the map for unmarked Nodes, and calls
remove(Node*) so they are removed from the map and so AXNodeObject::detach is
called.

[haraken, 2015-05-28 10:48:42]

Thanks for working on the complicated hierarchy!

This is complicated.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXImageMapLink.cpp (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXImageMapLink.cpp:146:
AXMockObject::trace(visitor);

Make the ::trace call a tail call. The same comment for other traces.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXNodeObject.h (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.h:210: RawPtrWillBeWeakMember<Node>
m_node;

Why does this need to be weak?

What confuses me is that you sometimes use WeakMembers in AXObjects and
sometimes Members. What's the difference? I think all AXObjects should hold a
Member to the object they are observing, because the AXObject and the
corresponding element should live and die together.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.cpp:394: ASSERT(isDetached());

Just help me understand: In what scenario is it possible that we reach here
without calling detach()?

Just to confirm: detach() of AXObjects are not doing anything than just clearing
some pointers? Otherwise, not calling detach() in some cases would cause a
problem.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:216: class AccessibilityText : public
NoBaseWillBeGarbageCollectedFinalized<AccessibilityText> {

Add final.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:218: static
PassOwnPtrWillBeRawPtr<AccessibilityText> create(const String& t, const
AccessibilityTextSource& s)

t => text
s => source

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:220: return
adoptPtrWillBeNoop<AccessibilityText>(new AccessibilityText(t, s, nullptr));

adoptPtrWillBeNoop<AccessibilityText> => adoptPtrWillBeNoop

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:224: return
adoptPtrWillBeNoop<AccessibilityText>(new AccessibilityText(t, s, nullptr));

Ditto.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:227: String text() { return m_text; }

Add const.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:228: AccessibilityTextSource
textSource() { return m_textSource; }

Ditto.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:229: AXObject* textElement() { return
m_textElement.get(); }

Ditto.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:237: AccessibilityText(const String& t,
const AccessibilityTextSource& s, const RefPtrWillBeRawPtr<AXObject> element)

This looks strange...

RefPtr => PassRefPtr ?

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:803:
RawPtrWillBeWeakMember<AXObjectCacheImpl> m_axObjectCache;

I think this should be a Member. AXObjectCache owns the AXObject and they die
together, don't they?

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:113: for (auto& entry :
m_layoutObjectMapping) {

Don't we need to call detach() for AbstractInlineTextBoxs as well?

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:122: }

You'll need to call m_objects.clear(). Otherwise, the detach state and the
contents of the hashmap will be inconsistent until the next GC is triggered.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:568: obj->detach();

I think you still need to explicitly remove the object from m_objects, even
though m_objects is a hashmap of weak members. Otherwise, the detach state and
the contents of the hashmap will be inconsistent until the next GC is triggered.

(But as commented in the header file, I don't fully understand why m_objects is
a hashmap of weak members, not members.)

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:663: for (auto& entry :
m_nodeObjectMapping) {

Why is this needed? The weak processing should have been completed when you
reach here, because m_nodeObjectMapping is a hashmap of weak members. This
manual weak processing was needed before this CL because it was a hashmap of raw
pointers.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:671: for (auto& entry :
m_widgetObjectMapping) {

Ditto. This weak processing won't be needed.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:678: #if ENABLE(OILPAN)

Remove the #if flag. AXObjectCacheImpl::clearWeakMembers is called only in
oilpan builds. Instead you can add the #if flag on clearWeakMembers.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:681: if (entry.value)

This should be !visitor->isHeapObjectAlive(entry.value).

However, I don't fully understand why m_objects needs to be a hashmap of weak
members, not members.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:688: }

If I'm not missing something, I guess we can remove the clearWeakMembers
completely.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.h (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.h:192: Document& m_document;

This should be Member<Document>.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.h:195: HeapHashMap<AXID,
WeakMember<AXObject>> m_objects;

Why isn't this a HeapHashMap<AXID, Member<AXObject>>? It seems strange that you
change RefPtr to WeakMember.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.h:205:
HashMap<AbstractInlineTextBox*, AXID> m_inlineTextBoxObjectMapping;

It is a bit unfortunate (and makes it harder to verify correctness) that we have
to use different types of collections between oilpan and non-oilpan. Would it be
possible to refactor non-oilpan code so that it uses the same types of
collections as oilpan (before landing this CL)? Or would it be hard for some
reason?

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXScrollView.h (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXScrollView.h:89:
RawPtrWillBeWeakMember<FrameView> m_scrollView;

Would you elaborate on why this needs to be weak?

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXSpinButton.cpp (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXSpinButton.cpp:69: void
AXSpinButton::detachFromParent()

Some AXObjects have detachFromParent() but some AXObjects don't. What's the
difference? I guess all AXObjects that have raw pointers (on non-oilpan builds)
need to implement detachFromParent() and clear the raw pointers.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXSpinButton.h (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXSpinButton.h:57:
RawPtrWillBeWeakMember<SpinButtonElement> m_spinButtonElement;

Would you elaborate on why this needs to be weak?

[dmazzoni, 2015-05-28 14:57:52]

>
>
https://codereview.chromium.org/1072273006/diff/60001/Source/modules/accessib...
> Source/modules/accessibility/AXObjectCacheImpl.h:198:
> HeapHashMap<WeakMember<Node>, Member<AXObject>> m_nodeObjectMapping;
> On 2015/04/28 06:53:25, dmazzoni wrote:
>
>> I'm worried about WeakMember<Node> here. We rely on getting a call to
>> AXObjectCache::remove(Node*) from Node::willBeDeletedFromDocument
>>
> currently.
>
>> With this change, it looks like that call won't happen, so we'll end
>>
> up with an
>
>> entry in the HashMap with a null key and the AXObject won't detach
>>
> itself.
>
> HeapHashMap with WeakMember entries will automatically be removed by
> weak processing.
> However, in this case we need to call AXNodeObject::detach for the
> removed nodes so I am doing custom weak processing.
> I've registered AXObjectCacheImpl::clearWeakMembers as a weak processing
> callback using registerWeakMembers.
> AXObjectCacheImpl::clearWeakMembers scans the map for unmarked Nodes,
> and calls remove(Node*) so they are removed from the map and so
> AXNodeObject::detach is called.
>

That sounds good, but I don't see the call to AXObjectCacheImpl::remove
from AXObjectCacheImpl::clearWeakMembers, just m_objects.take(axID), which
isn't sufficient.

Also, I see you did nodes and widgets, but not LayoutObjects - doesn't the
same need to be done there?

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

[dmazzoni, 2015-06-03 20:55:48]

Just curious, is this still in progress? Are you waiting on anything from me?

[keishi, 2015-06-03 22:57:47]

On 2015/06/03 20:55:48, dmazzoni wrote:
> Just curious, is this still in progress? Are you waiting on anything from me?

Sorry, no, I'll update soon.

[keishi, 2015-06-08 06:27:21]

My intent was to simplify the reference structure by reducing reliance on AXID,
and use weak processing to clean up, but I give up.

Right now AXObjectCache::remove(Node*) is called from
Node::willBeDeletedFromDocument.
In my new patch AXObjectCache::remove(Node*) is called from Node::removedFrom().
However when a iframe loads another page, Node::removedFrom() is not called for
the nodes in the iframe, but the AXObjectCache is owned by the root document, so
the strong reference from AXObjectCache to the AXNodeObject persists and the
AXNodeObjects are retained untile the AXObjectCache is destroyed. I got around
it by calling AXObjectCache::remove for all descendent nodes in
Document::detach().

Right now, in the oilpan builds, AXObjectCache::remove(Widget*) is called from
weak processing.
My new patch does the same thing. AXScrollbar::m_scrollbar and
AXScrollView::m_scrollView need to be weak Members so that they get collected
before the AXObjectCache does.

AXInlineTextBox::~AXInlineTextBox touches m_axObjectCache, which is not allowed
in Oilpan. I couldn't figure out why this was dealying removal from
AXObjectCacheImpl, so I changed it so we call
AXObjectCache::remove(AbstractInlineTextBox*) from
AbstractInlineTextBox::detach().

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXImageMapLink.cpp (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXImageMapLink.cpp:146:
AXMockObject::trace(visitor);
On 2015/05/28 10:48:40, haraken wrote:
> 
> Make the ::trace call a tail call. The same comment for other traces.
> 

Done.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.cpp:394: ASSERT(isDetached());
On 2015/05/28 10:48:40, haraken wrote:
> 
> Just help me understand: In what scenario is it possible that we reach here
> without calling detach()?
> 
> Just to confirm: detach() of AXObjects are not doing anything than just
clearing
> some pointers? Otherwise, not calling detach() in some cases would cause a
> problem.

Reverted.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:216: class AccessibilityText : public
NoBaseWillBeGarbageCollectedFinalized<AccessibilityText> {
On 2015/05/28 10:48:40, haraken wrote:
> 
> Add final.

Done.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:218: static
PassOwnPtrWillBeRawPtr<AccessibilityText> create(const String& t, const
AccessibilityTextSource& s)
On 2015/05/28 10:48:40, haraken wrote:
> 
> t => text
> s => source

Done.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:220: return
adoptPtrWillBeNoop<AccessibilityText>(new AccessibilityText(t, s, nullptr));
On 2015/05/28 10:48:41, haraken wrote:
> 
> adoptPtrWillBeNoop<AccessibilityText> => adoptPtrWillBeNoop

Done.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:224: return
adoptPtrWillBeNoop<AccessibilityText>(new AccessibilityText(t, s, nullptr));
On 2015/05/28 10:48:40, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:227: String text() { return m_text; }
On 2015/05/28 10:48:41, haraken wrote:
> 
> Add const.

Done.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:228: AccessibilityTextSource
textSource() { return m_textSource; }
On 2015/05/28 10:48:41, haraken wrote:
> 
> Ditto.
> 

Done.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:229: AXObject* textElement() { return
m_textElement.get(); }
On 2015/05/28 10:48:41, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:237: AccessibilityText(const String& t,
const AccessibilityTextSource& s, const RefPtrWillBeRawPtr<AXObject> element)
On 2015/05/28 10:48:40, haraken wrote:
> 
> This looks strange...
> 
> RefPtr => PassRefPtr ?

Done.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:803:
RawPtrWillBeWeakMember<AXObjectCacheImpl> m_axObjectCache;
On 2015/05/28 10:48:41, haraken wrote:
> 
> I think this should be a Member. AXObjectCache owns the AXObject and they die
> together, don't they?

Done.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.h (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.h:192: Document& m_document;
On 2015/05/28 10:48:41, haraken wrote:
> 
> This should be Member<Document>.

Done.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.h:195: HeapHashMap<AXID,
WeakMember<AXObject>> m_objects;
On 2015/05/28 10:48:41, haraken wrote:
> 
> Why isn't this a HeapHashMap<AXID, Member<AXObject>>? It seems strange that
you
> change RefPtr to WeakMember.
> 

Done.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.h:205:
HashMap<AbstractInlineTextBox*, AXID> m_inlineTextBoxObjectMapping;
On 2015/05/28 10:48:41, haraken wrote:
> 
> It is a bit unfortunate (and makes it harder to verify correctness) that we
have
> to use different types of collections between oilpan and non-oilpan. Would it
be
> possible to refactor non-oilpan code so that it uses the same types of
> collections as oilpan (before landing this CL)? Or would it be hard for some
> reason?
> 

Done.

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
File Source/modules/accessibility/AXSpinButton.h (right):

https://codereview.chromium.org/1072273006/diff/120001/Source/modules/accessi...
Source/modules/accessibility/AXSpinButton.h:57:
RawPtrWillBeWeakMember<SpinButtonElement> m_spinButtonElement;
On 2015/05/28 10:48:42, haraken wrote:
> 
> Would you elaborate on why this needs to be weak?

Changed to Member.

[dmazzoni, 2015-06-08 15:25:50]

https://codereview.chromium.org/1072273006/diff/200001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:2134: cache->remove(&element);
What would happen if we called AXObjectCache::getOrCreate and passed it one of
these elements immediately after this call? Would it correctly return null
because these elements are not attached to a document?

What I'm worried about is that when we remove an element from the cache, that
triggers calling childrenChanged on the parent node - and in some cases that may
try to use element traversal to find the parent and construct a new AXObject
from it.

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObject.cpp:1485: visitor->trace(m_axObjectCache);
Why is this circular reference needed? It doesn't seem like each AXObject needs
to visit its AXObjectCache.

Suppose a document is destroyed and it destroys its AXObjectCache. A few of the
AXObjects may stick around briefly because a WebAXObject still has a reference
to them - why does the AXObjectCache need to persist too?

I think this points out a possible bug in the existing code - I'd expect that
AXObject::detach() should be clearing m_axObjectCache.

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:580:
remove(m_inlineTextBoxObjectMapping.get(inlineTextBox));
What's wrong with the code to remove by AXID? I don't understand why this needed
to be changed.

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:710:
RefPtrWillBeRawPtr<Document> protectorForCacheOwner(m_document.get());
Can we just delete this line of code in oilpan? The purpose is just to hold a
reference for the scope of this function, and that seems unnecessary under
oilpan.

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:1368:
visitor->trace(m_nodeObjectMapping);
Why not m_inlineTextBoxObjectMapping here too?

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.h (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.h:214:
HashMap<AbstractInlineTextBox*, AXID> m_inlineTextBoxObjectMapping;
Why not a HeapHashMap for this?

Would it make sense to have a comment explaining why some of these are
HeapHashMaps and others are not?

[haraken, 2015-06-09 02:25:46]

Thanks for working on this! It's getting a better shape.

I have a couple of fundamental questions about the lifetime relationship of
AXObjects, so let me ask offline.

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObject.cpp:1485: visitor->trace(m_axObjectCache);
On 2015/06/08 15:25:49, dmazzoni wrote:
> Why is this circular reference needed? It doesn't seem like each AXObject
needs
> to visit its AXObjectCache.
> 
> Suppose a document is destroyed and it destroys its AXObjectCache. A few of
the
> AXObjects may stick around briefly because a WebAXObject still has a reference
> to them - why does the AXObjectCache need to persist too?
> 
> I think this points out a possible bug in the existing code - I'd expect that
> AXObject::detach() should be clearing m_axObjectCache.

We don't need to worry about circular references when tracing objects. We just
need to visit all object graph edges -- if the target node is already visited
(because of circular references etc), the visitor just stops the traversal
there. That's the way a GC system works and that's exactly why a GC system
allows the program to create circular references that haven't allowed in the
reference counting system.

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:216: class AccessibilityText final :
public NoBaseWillBeGarbageCollectedFinalized<AccessibilityText> {

Let's make this in a separate CL (before landing this CL).

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:111: for (auto& entry :
m_objects) {

(This is a question not related to this CL.)

Sigbjorn or someone who is familiar with C++: When we have "auto", is it
guaranteed that we have an iterator object on the stack? The conservative
scanning relies on the fact that the iterator object (that holds the pointer to
the HashMap itself) is on the stack. Otherwise, the conservative scanning fails
in strongifying HashMap<WeakMember<>> on the stack.

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:592: for (auto& entry :
m_widgetObjectMapping) {

Why do we still need the weak processing for m_widgetObjectMapping?
m_widgetObjectMapping is a hash map of strong members, so I don't think we need
this.

Then I think we can completely get rid of AXObjectCacheImpl::clearWeakMembers.

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:710:
RefPtrWillBeRawPtr<Document> protectorForCacheOwner(m_document.get());
On 2015/06/08 15:25:49, dmazzoni wrote:
> Can we just delete this line of code in oilpan? The purpose is just to hold a
> reference for the scope of this function, and that seems unnecessary under
> oilpan.

You're right.

We had a similar discussion before including other ideas (e.g., introduce
RefPtrWillBeRemoved). Then we just decided to leave the protect pointer as is
and remove it when we make Oilpan the default. There is no strong reason for the
decision. Either is fine with me :)

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:1368:
visitor->trace(m_nodeObjectMapping);
On 2015/06/08 15:25:49, dmazzoni wrote:
> Why not m_inlineTextBoxObjectMapping here too?

It is not on the oilpan's heap, so don't need to trace.

If you forget to trace something, the error will be detected by the GC plugin.

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.h (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.h:214:
HashMap<AbstractInlineTextBox*, AXID> m_inlineTextBoxObjectMapping;
On 2015/06/08 15:25:49, dmazzoni wrote:
> Why not a HeapHashMap for this?
> 
> Would it make sense to have a comment explaining why some of these are
> HeapHashMaps and others are not?

Worth having a comment. The reason is that LayoutObject and
AbstractInlineTextBox are not (yet) in Oilpan's heap. So we cannot use a
HeapHashMap.

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXScrollView.cpp (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXScrollView.cpp:46: #if !ENABLE(OILPAN)

Do we need the #if? In other words, is it possible that the destructor of
AXScrollView gets called without AXScrollView::detach getting called?

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXScrollView.h (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXScrollView.h:84:
RawPtrWillBeWeakMember<FrameView> m_scrollView;

According to the above comment, this raw pointer is explicitly cleared when the
FrameView gets detached. Then this should be a Member.

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXScrollbar.h (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXScrollbar.h:66: RefPtrWillBeWeakMember<Scrollbar>
m_scrollbar;

Does this need to be weak? I think you need to make this a Member and clear it
in AXScrollbar::detach().

[keishi, 2015-06-09 05:51:20]

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObject.cpp:1485: visitor->trace(m_axObjectCache);
On 2015/06/09 02:25:45, haraken wrote:
> On 2015/06/08 15:25:49, dmazzoni wrote:
> > Why is this circular reference needed? It doesn't seem like each AXObject
> needs
> > to visit its AXObjectCache.
> > 
> > Suppose a document is destroyed and it destroys its AXObjectCache. A few of
> the
> > AXObjects may stick around briefly because a WebAXObject still has a
reference
> > to them - why does the AXObjectCache need to persist too?
> > 
> > I think this points out a possible bug in the existing code - I'd expect
that
> > AXObject::detach() should be clearing m_axObjectCache.
Like haraken said we don't need to worry about circular references for GCed
objects. (and the rule is to always trace all Member references)
I didn't like that m_axObjectCache might be keeping a stale pointer to
AXObjectCache and thats why I added m_axObjectCache=nullptr to
AXObject::detach(). I haven't done it yet but I think we could get rid of
m_detached and use m_axObjectCache instead for isDetached().

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:580:
remove(m_inlineTextBoxObjectMapping.get(inlineTextBox));
On 2015/06/08 15:25:49, dmazzoni wrote:
> What's wrong with the code to remove by AXID? I don't understand why this
needed
> to be changed.
> 

Sorry, this was left over from my previous patch. Removed.

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:592: for (auto& entry :
m_widgetObjectMapping) {
On 2015/06/09 02:25:45, haraken wrote:
> 
> Why do we still need the weak processing for m_widgetObjectMapping?
> m_widgetObjectMapping is a hash map of strong members, so I don't think we
need
> this.
> 
> Then I think we can completely get rid of AXObjectCacheImpl::clearWeakMembers.

Done. I thought I needed this but I I think I was wrong.

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.h (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.h:214:
HashMap<AbstractInlineTextBox*, AXID> m_inlineTextBoxObjectMapping;
On 2015/06/09 02:25:45, haraken wrote:
> On 2015/06/08 15:25:49, dmazzoni wrote:
> > Why not a HeapHashMap for this?
> > 
> > Would it make sense to have a comment explaining why some of these are
> > HeapHashMaps and others are not?
> 
> Worth having a comment. The reason is that LayoutObject and
> AbstractInlineTextBox are not (yet) in Oilpan's heap. So we cannot use a
> HeapHashMap.
>
 
Added comment.

[haraken, 2015-06-09 09:12:13]

Discussed offline.

- We decided to use a WeakMember to from an AXObject to its corresponding DOM
object. It seems possible to use a Member and explicitly clear the Member in all
cases (i.e., patch set 13), but we're not 100% sure if the Member doesn't lead
to leaking objects. From the perspective of semantics, the reference from an
AXObject to its corresponding DOM object should be weak, so it would be more
correct to use a WeakMember there.

- We want to make sure that the AXObject hierarchy is not leaking before landing
this CL. In the reference counting world, leaks in the AXObject hierarchy
haven't been a serious issue (even if they exist) because the AXObjects don't
keep the DOM hierarchy alive. However, in the oilpan world, AXObjects can keep
the DOM hierarchy alive with a chain of Members etc. So it is important to
verify that the AXObject hierarchy is not leaking. We're planning to add
window.internals.numberOfAXObjects (which counts the number of AXObjects) and
observe that value in the leak detector (and fix leaks if we find some).

[dmazzoni, 2015-06-09 18:07:28]

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObject.cpp:1485: visitor->trace(m_axObjectCache);
On 2015/06/09 05:51:19, keishi wrote:
> On 2015/06/09 02:25:45, haraken wrote:
> > On 2015/06/08 15:25:49, dmazzoni wrote:
> > > Why is this circular reference needed? It doesn't seem like each AXObject
> > needs
> > > to visit its AXObjectCache.
> > > 
> > > Suppose a document is destroyed and it destroys its AXObjectCache. A few
of
> > the
> > > AXObjects may stick around briefly because a WebAXObject still has a
> reference
> > > to them - why does the AXObjectCache need to persist too?
> > > 
> > > I think this points out a possible bug in the existing code - I'd expect
> that
> > > AXObject::detach() should be clearing m_axObjectCache.
> Like haraken said we don't need to worry about circular references for GCed
> objects. (and the rule is to always trace all Member references)
> I didn't like that m_axObjectCache might be keeping a stale pointer to
> AXObjectCache and thats why I added m_axObjectCache=nullptr to
> AXObject::detach(). I haven't done it yet but I think we could get rid of
> m_detached and use m_axObjectCache instead for isDetached().

I like the idea of using m_axObjectCache instead of m_detached.

Could you consider landing that as a separate change?

Are there any other changes that are part of this CL right now that could be
split out? If this ends up having a bug in it it'd be a lot to revert.

[haraken, 2015-06-10 00:55:57]

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1072273006/diff/200001/Source/modules/accessi...
Source/modules/accessibility/AXObject.cpp:1485: visitor->trace(m_axObjectCache);
On 2015/06/09 18:07:28, dmazzoni wrote:
> On 2015/06/09 05:51:19, keishi wrote:
> > On 2015/06/09 02:25:45, haraken wrote:
> > > On 2015/06/08 15:25:49, dmazzoni wrote:
> > > > Why is this circular reference needed? It doesn't seem like each
AXObject
> > > needs
> > > > to visit its AXObjectCache.
> > > > 
> > > > Suppose a document is destroyed and it destroys its AXObjectCache. A few
> of
> > > the
> > > > AXObjects may stick around briefly because a WebAXObject still has a
> > reference
> > > > to them - why does the AXObjectCache need to persist too?
> > > > 
> > > > I think this points out a possible bug in the existing code - I'd expect
> > that
> > > > AXObject::detach() should be clearing m_axObjectCache.
> > Like haraken said we don't need to worry about circular references for GCed
> > objects. (and the rule is to always trace all Member references)
> > I didn't like that m_axObjectCache might be keeping a stale pointer to
> > AXObjectCache and thats why I added m_axObjectCache=nullptr to
> > AXObject::detach(). I haven't done it yet but I think we could get rid of
> > m_detached and use m_axObjectCache instead for isDetached().
> 
> I like the idea of using m_axObjectCache instead of m_detached.
> 
> Could you consider landing that as a separate change?
> 
> Are there any other changes that are part of this CL right now that could be
> split out? If this ends up having a bug in it it'd be a lot to revert.

I think the AccessibilityText part can be landed separately. Incremental landing
is a key to make a quick progress :)

[keishi, 2015-06-11 05:33:52]

Separated out the AccessibilityText part and m_detached part
accessibility part: https://codereview.chromium.org/1169023003/
m_detached part: https://codereview.chromium.org/1175533004/

The new patch assumes these two have also landed.

On 2015/06/09 09:12:13, haraken wrote:
> Discussed offline.
> 
> - We decided to use a WeakMember to from an AXObject to its corresponding DOM
> object. It seems possible to use a Member and explicitly clear the Member in
all
> cases (i.e., patch set 13), but we're not 100% sure if the Member doesn't lead
> to leaking objects. From the perspective of semantics, the reference from an
> AXObject to its corresponding DOM object should be weak, so it would be more
> correct to use a WeakMember there.

Done. Used WeakMember for references to the DOM node.

> - We want to make sure that the AXObject hierarchy is not leaking before
landing
> this CL. In the reference counting world, leaks in the AXObject hierarchy
> haven't been a serious issue (even if they exist) because the AXObjects don't
> keep the DOM hierarchy alive. However, in the oilpan world, AXObjects can keep
> the DOM hierarchy alive with a chain of Members etc. So it is important to
> verify that the AXObject hierarchy is not leaking. We're planning to add
> window.internals.numberOfAXObjects (which counts the number of AXObjects) and
> observe that value in the leak detector (and fix leaks if we find some).

I've created the CL for the check https://codereview.chromium.org/1181613002/ .
However doLeakTest is only used for two tests and they don't seem to be leaking.

I checked if this CL is causing leaks by printing the numberOfLiveAXObjects
inside AXObjectCacheImpl::dispose() and comparing it to the non-oilpan build and
the oilpan build.
This revealed a leak for AXScrollObject so I fixed that with this CL
https://codereview.chromium.org/1171963003/

[keishi, 2015-06-11 06:34:04]

On 2015/06/11 05:33:52, keishi wrote:
> Separated out the AccessibilityText part and m_detached part
> accessibility part: https://codereview.chromium.org/1169023003/
> m_detached part: https://codereview.chromium.org/1175533004/
> 
> The new patch assumes these two have also landed.
> 
> On 2015/06/09 09:12:13, haraken wrote:
> > Discussed offline.
> > 
> > - We decided to use a WeakMember to from an AXObject to its corresponding
DOM
> > object. It seems possible to use a Member and explicitly clear the Member in
> all
> > cases (i.e., patch set 13), but we're not 100% sure if the Member doesn't
lead
> > to leaking objects. From the perspective of semantics, the reference from an
> > AXObject to its corresponding DOM object should be weak, so it would be more
> > correct to use a WeakMember there.
> 
> Done. Used WeakMember for references to the DOM node.
> 
> > - We want to make sure that the AXObject hierarchy is not leaking before
> landing
> > this CL. In the reference counting world, leaks in the AXObject hierarchy
> > haven't been a serious issue (even if they exist) because the AXObjects
don't
> > keep the DOM hierarchy alive. However, in the oilpan world, AXObjects can
keep
> > the DOM hierarchy alive with a chain of Members etc. So it is important to
> > verify that the AXObject hierarchy is not leaking. We're planning to add
> > window.internals.numberOfAXObjects (which counts the number of AXObjects)
and
> > observe that value in the leak detector (and fix leaks if we find some).
> 
> I've created the CL for the check https://codereview.chromium.org/1181613002/
.
> However doLeakTest is only used for two tests and they don't seem to be
leaking.
I was mistaken. accessibility/axobjectcache-leaks-node.html is failing the test.
I'm looking at it now.

[keishi, 2015-06-23 02:21:36]

I've changed all the members to counterpart objects to WeakMembers so that we
won't leak counterpart objects when we leak AXObjects. Like how it works right
now.

We can land this before all the leak fix CLs are landed so PTAL. Thanks.

[haraken, 2015-06-23 06:33:30]

I apologize for the back & forth, but I changed my mind -- let's use Member (not
WeakMember) to point to DOM objects from AXObjects.

It is guaranteed that AXObject::detach() is explicitly called by a corresponding
DOM object before the AXObject gets destructed. Thus there is not need to use a
WeakMember from the AXObject to the DOM object. (Even a raw pointer will work.)
Given that using WeakMembers adds an unnecessary pressure on Oilpan's GC, let's
just use a Member.

With that change, the CL looks good!

https://codereview.chromium.org/1072273006/diff/280001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/1072273006/diff/280001/Source/core/dom/Elemen...
Source/core/dom/Element.h:810: cache->remove(this);

Can we remove cache->remove() from TreeScopeAdopter::moveTreeToNewScope and
enable this code on both oilpan and non-oilpan? It's not nice that we have a
different lifetime management between oilpan and non-oilpan.

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:232: DEFINE_INLINE_TRACE() {

Nit: { should go to the next line.

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:111: for (auto& entry :
m_objects) {

The contents of m_objects can change during the iteration, but is it safe?

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
File Source/modules/accessibility/AXScrollView.cpp (right):

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
Source/modules/accessibility/AXScrollView.cpp:46: #if !ENABLE(OILPAN)

Maybe I asked this before but why do we need this?

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
File Source/modules/accessibility/AXScrollView.h (right):

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
Source/modules/accessibility/AXScrollView.h:79: // FIXME: Oilpan:
Frame/ScrollView is on the heap and its

Remove this FIXME.

[haraken, 2015-06-23 06:47:22]

https://codereview.chromium.org/1072273006/diff/280001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/1072273006/diff/280001/Source/core/dom/Elemen...
Source/core/dom/Element.h:810: cache->remove(this);
On 2015/06/23 06:33:29, haraken wrote:
> 
> Can we remove cache->remove() from TreeScopeAdopter::moveTreeToNewScope and
> enable this code on both oilpan and non-oilpan? It's not nice that we have a
> different lifetime management between oilpan and non-oilpan.

Or, in oilpan, isn't the cache->remove() in TreeScopeAdopter::moveTreeToNewScope
enough?

[keishi, 2015-06-24 05:08:22]

https://codereview.chromium.org/1072273006/diff/280001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/1072273006/diff/280001/Source/core/dom/Elemen...
Source/core/dom/Element.h:810: cache->remove(this);
On 2015/06/23 06:47:22, haraken wrote:
> On 2015/06/23 06:33:29, haraken wrote:
> > 
> > Can we remove cache->remove() from TreeScopeAdopter::moveTreeToNewScope and
> > enable this code on both oilpan and non-oilpan? It's not nice that we have a
> > different lifetime management between oilpan and non-oilpan.
> 
> Or, in oilpan, isn't the cache->remove() in
TreeScopeAdopter::moveTreeToNewScope
> enough?

Removed excess remove() calls. Node::removeFrom and Document::detach() I think
is enough.

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
Source/modules/accessibility/AXObject.h:232: DEFINE_INLINE_TRACE() {
On 2015/06/23 06:33:29, haraken wrote:
> 
> Nit: { should go to the next line.

Done.

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:111: for (auto& entry :
m_objects) {
On 2015/06/23 06:33:29, haraken wrote:
> 
> The contents of m_objects can change during the iteration, but is it safe?

m_objects doesn't change. It needs to be this way until the leak fix CL lands.

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
File Source/modules/accessibility/AXScrollView.cpp (right):

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
Source/modules/accessibility/AXScrollView.cpp:46: #if !ENABLE(OILPAN)
On 2015/06/23 06:33:29, haraken wrote:
> 
> Maybe I asked this before but why do we need this?

Removed.

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
File Source/modules/accessibility/AXScrollView.h (right):

https://codereview.chromium.org/1072273006/diff/280001/Source/modules/accessi...
Source/modules/accessibility/AXScrollView.h:79: // FIXME: Oilpan:
Frame/ScrollView is on the heap and its
On 2015/06/23 06:33:29, haraken wrote:
> 
> Remove this FIXME.

Done.

[haraken, 2015-06-24 13:27:20]

https://codereview.chromium.org/1072273006/diff/300001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:2190: } else if (AXObjectCache* cache =
existingAXObjectCache()) {

This change will be made in a separate CL, right?

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXImageMapLink.cpp (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXImageMapLink.cpp:47: {

Can we add ASSERT(!m_areaElement) and ASSERT(m_mapElement).

The same comment for all other Members to counterparts in this CL.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXImageMapLink.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXImageMapLink.h:73:
RefPtrWillBeWeakMember<HTMLAreaElement> m_areaElement;

This should be Member.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXImageMapLink.h:74:
RefPtrWillBeWeakMember<HTMLMapElement> m_mapElement;

Ditto.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXInlineTextBox.cpp (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXInlineTextBox.cpp:60: AXObject::detach();

detach() call should be first.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXMenuListOption.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXMenuListOption.h:63:
RawPtrWillBeWeakMember<HTMLOptionElement> m_element;

This should be Member.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:759: AXObject::detach();

detach() should be first.

Maybe you can fix this in a follow-up.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXNodeObject.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.h:213: RawPtrWillBeWeakMember<Node>
m_node;

Member.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:547: void
AXObjectCacheImpl::remove(Node* node)

Can we add an ASSERT to all remove methods to verify that the same DOM object
doesn't get removed multiple times?

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.h:215:
WillBeHeapHashSet<RawPtrWillBeWeakMember<Node>> m_textMarkerNodes;

I think this can be a Member as well, since the lifetime of m_textMarkerNodes is
explicitly managed.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXScrollView.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXScrollView.h:79:
RawPtrWillBeWeakMember<FrameView> m_scrollView;

Member.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXScrollbar.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXScrollbar.h:66: RefPtrWillBeWeakMember<Scrollbar>
m_scrollbar;

Member.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXSpinButton.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXSpinButton.h:57:
RawPtrWillBeWeakMember<SpinButtonElement> m_spinButtonElement;

Member.

[keishi, 2015-06-24 15:05:27]

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXImageMapLink.cpp (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXImageMapLink.cpp:47: {
On 2015/06/24 13:27:20, haraken wrote:
> 
> Can we add ASSERT(!m_areaElement) and ASSERT(m_mapElement).
> 
> The same comment for all other Members to counterparts in this CL.

Done.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXImageMapLink.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXImageMapLink.h:73:
RefPtrWillBeWeakMember<HTMLAreaElement> m_areaElement;
On 2015/06/24 13:27:20, haraken wrote:
> 
> This should be Member.

Done.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXImageMapLink.h:74:
RefPtrWillBeWeakMember<HTMLMapElement> m_mapElement;
On 2015/06/24 13:27:20, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXInlineTextBox.cpp (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXInlineTextBox.cpp:60: AXObject::detach();
On 2015/06/24 13:27:20, haraken wrote:
> 
> detach() call should be first.

Done.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXMenuListOption.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXMenuListOption.h:63:
RawPtrWillBeWeakMember<HTMLOptionElement> m_element;
On 2015/06/24 13:27:20, haraken wrote:
> 
> This should be Member.

Done.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:759: AXObject::detach();
On 2015/06/24 13:27:20, haraken wrote:
> 
> detach() should be first.
> 
> Maybe you can fix this in a follow-up.

Removed clearChildren because AXObject::detach does it.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXNodeObject.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.h:213: RawPtrWillBeWeakMember<Node>
m_node;
On 2015/06/24 13:27:20, haraken wrote:
> 
> Member.

Done.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:547: void
AXObjectCacheImpl::remove(Node* node)
On 2015/06/24 13:27:20, haraken wrote:
> 
> Can we add an ASSERT to all remove methods to verify that the same DOM object
> doesn't get removed multiple times?

I don't think we can. We might be calling remove() for a Node that didn't have a
corresponding AXObject so we can't assume that m_nodeObjectMapping always has an
entry for the Node.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.h:215:
WillBeHeapHashSet<RawPtrWillBeWeakMember<Node>> m_textMarkerNodes;
On 2015/06/24 13:27:20, haraken wrote:
> 
> I think this can be a Member as well, since the lifetime of m_textMarkerNodes
is
> explicitly managed.
> 

Done. Just noticed I can't find setNodeInUse used anywhere. I think this might
be dead code.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXScrollView.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXScrollView.h:79:
RawPtrWillBeWeakMember<FrameView> m_scrollView;
On 2015/06/24 13:27:20, haraken wrote:
> 
> Member.

Done.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXScrollbar.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXScrollbar.h:66: RefPtrWillBeWeakMember<Scrollbar>
m_scrollbar;
On 2015/06/24 13:27:20, haraken wrote:
> 
> Member.

Done.

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
File Source/modules/accessibility/AXSpinButton.h (right):

https://codereview.chromium.org/1072273006/diff/300001/Source/modules/accessi...
Source/modules/accessibility/AXSpinButton.h:57:
RawPtrWillBeWeakMember<SpinButtonElement> m_spinButtonElement;
On 2015/06/24 13:27:20, haraken wrote:
> 
> Member.

Done.

[haraken, 2015-06-24 23:37:33]

haraken@chromium.org changed reviewers:
+ tkent@chromium.org

[haraken, 2015-06-24 23:37:35]

LGTM finally!

+tkent-san for public/.

https://codereview.chromium.org/1072273006/diff/320001/Source/core/layout/lin...
File Source/core/layout/line/AbstractInlineTextBox.cpp (right):

https://codereview.chromium.org/1072273006/diff/320001/Source/core/layout/lin...
Source/core/layout/line/AbstractInlineTextBox.cpp:71: void
AbstractInlineTextBox::detach()

Add an ASSERT to ~AbstractInlineTextBox to verify that the detach has been
called before it gets destructed.

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
File Source/modules/accessibility/AXMenuListOption.h (right):

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
Source/modules/accessibility/AXMenuListOption.h:42: explicit
AXMenuListOption(HTMLOptionElement*, AXObjectCacheImpl&);

Drop explicit.

[tkent, 2015-06-25 00:20:48]

lgtm.

Can't we move AX objects on heap by default now?

https://codereview.chromium.org/1072273006/diff/320001/Source/core/dom/AXObje...
File Source/core/dom/AXObjectCache.h (right):

https://codereview.chromium.org/1072273006/diff/320001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.h:35: class AbstractInlineTextBox;
nit: add a blank line before this line.

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.cpp (left):

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
Source/modules/accessibility/AXObject.cpp:401: 
unnecessary change

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:93: : m_document(&document)
This change is unnecessary.

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
File Source/modules/accessibility/AXTable.cpp (left):

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
Source/modules/accessibility/AXTable.cpp:545: 
unnecessary change

[haraken, 2015-06-25 00:23:21]

> Can't we move AX objects on heap by default now?

That is a great question. I think it's doable, since the only substantial
reference from the Node hierarchy to accessibility is Document::m_axObjectCache.

Let's do that once we confirm that this CL works as expected on oilpan builds.

[keishi, 2015-06-25 03:29:28]

https://codereview.chromium.org/1072273006/diff/320001/Source/core/dom/AXObje...
File Source/core/dom/AXObjectCache.h (right):

https://codereview.chromium.org/1072273006/diff/320001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.h:35: class AbstractInlineTextBox;
On 2015/06/25 00:20:47, tkent wrote:
> nit: add a blank line before this line.

Done.

https://codereview.chromium.org/1072273006/diff/320001/Source/core/layout/lin...
File Source/core/layout/line/AbstractInlineTextBox.cpp (right):

https://codereview.chromium.org/1072273006/diff/320001/Source/core/layout/lin...
Source/core/layout/line/AbstractInlineTextBox.cpp:71: void
AbstractInlineTextBox::detach()
On 2015/06/24 23:37:34, haraken wrote:
> 
> Add an ASSERT to ~AbstractInlineTextBox to verify that the detach has been
> called before it gets destructed.

Done.

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
File Source/modules/accessibility/AXMenuListOption.h (right):

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
Source/modules/accessibility/AXMenuListOption.h:42: explicit
AXMenuListOption(HTMLOptionElement*, AXObjectCacheImpl&);
On 2015/06/24 23:37:35, haraken wrote:
> 
> Drop explicit.

Done.

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.cpp (left):

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
Source/modules/accessibility/AXObject.cpp:401: 
On 2015/06/25 00:20:47, tkent wrote:
> unnecessary change

Done.

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1072273006/diff/320001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:93: : m_document(&document)
On 2015/06/25 00:20:47, tkent wrote:
> This change is unnecessary.

Done.

[keishi, 2015-06-25 14:09:33]

dmazzoni@ PTAL Thanks.

[dmazzoni, 2015-06-25 23:13:36]

lgtm

[keishi, 2015-06-26 01:00:29]

The CQ bit was checked by keishi@chromium.org

[keishi, 2015-06-26 01:00:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1072273006/#ps340001
(title: " ")

[commit-bot: I haz the power, 2015-06-26 01:00:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1072273006/340001

[commit-bot: I haz the power, 2015-06-26 03:21:30]

Committed patchset #18 (id:340001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197881