Oilpan: Prepare to move AXObjectCache to the heap

Oilpan: Prepare to move AXObjectCache to the heap

ScopedAXObjectCache is only used one place at a time so it doesn't need to be a RefCounted.

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

[keishi, 2015-04-28 06:19:36]

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

[keishi, 2015-04-28 06:19:36]

Depends on Refactor ScopedAXObjectCache to remove ref count  (
https://codereview.chromium.org/1088473002/ )

[kouhei (in TOK), 2015-04-28 07:31:54]

lgtm

[haraken, 2015-04-28 10:45:21]

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

https://codereview.chromium.org/1081673003/diff/100001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.h:148: class CORE_EXPORT ScopedAXObjectCache {

Can we make this GarbageCollected? Then we can avoid introducing a Persistent
handle.

https://codereview.chromium.org/1081673003/diff/100001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.h:161: OwnPtr<AXObjectCache> m_ownedCache;

Why can't we just use OwnPtrWillBePersistent (and avoid having the OILPAN
branch)?

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

https://codereview.chromium.org/1081673003/diff/100001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:2207: void Document::clearAXObjectCache()

It is unsafe to land this CL at the moment since Document's destructor is
calling clearAXObjectCache().

We need to wait for https://codereview.chromium.org/1089813004/ to land and
remove clearAXObjectCache() from the destructor. However, it's not easy to
remove it since currently Document::detach is not always called for all
Documents.

https://codereview.chromium.org/1081673003/diff/100001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:2210: // Clear the cache member variable before
calling discard because attempts

What is discard?

https://codereview.chromium.org/1081673003/diff/100001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:2214: cache->dispose();

Nit:

if (m_axObjectCache)
  m_axObjectCache->dispose();
m_axObjectCache.clear();

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

https://codereview.chromium.org/1081673003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:100:
AXObjectCacheImpl::~AXObjectCacheImpl()

Can we insert an ASSERT to verify that dispose() has been called?

https://codereview.chromium.org/1081673003/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/InspectorAccessibilityAgent.cpp (right):

https://codereview.chromium.org/1081673003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/InspectorAccessibilityAgent.cpp:428: #if
ENABLE(OILPAN)

Would you elaborate on why we need a different logic between oilpan and
non-oilpan?

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

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

https://codereview.chromium.org/1081673003/diff/100001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.h:148: class CORE_EXPORT ScopedAXObjectCache {
On 2015/04/28 10:45:20, haraken wrote:
> 
> Can we make this GarbageCollected? Then we can avoid introducing a Persistent
> handle.
> 

Done.

https://codereview.chromium.org/1081673003/diff/100001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.h:161: OwnPtr<AXObjectCache> m_ownedCache;
On 2015/04/28 10:45:20, haraken wrote:
> 
> Why can't we just use OwnPtrWillBePersistent (and avoid having the OILPAN
> branch)?

Done.

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

https://codereview.chromium.org/1081673003/diff/100001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:2210: // Clear the cache member variable before
calling discard because attempts
On 2015/04/28 10:45:21, haraken wrote:
> 
> What is discard?

My mistake. I got confused. Reverted.

https://codereview.chromium.org/1081673003/diff/100001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:2214: cache->dispose();
On 2015/04/28 10:45:21, haraken wrote:
> 
> Nit:
> 
> if (m_axObjectCache)
>   m_axObjectCache->dispose();
> m_axObjectCache.clear();

Done.

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

https://codereview.chromium.org/1081673003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:100:
AXObjectCacheImpl::~AXObjectCacheImpl()
On 2015/04/28 10:45:21, haraken wrote:
> 
> Can we insert an ASSERT to verify that dispose() has been called?

Done.

[haraken, 2015-05-28 08:57:57]

Maybe I should have suggested this earlier but...

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

https://codereview.chromium.org/1081673003/diff/160001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.h:148: class CORE_EXPORT ScopedAXObjectCache :
public NoBaseWillBeGarbageCollectedFinalized<ScopedAXObjectCache> {

Is ScopedAXObjectCache used in performance-sensitive places?

If no, we might want to keep ScopedAXObjectCache off-heap. That way we can call
m_cache->dispose() in the destructor of ScopedAXObjectCache. Then we don't need
to call ->dispose() in the call sites of ScopedAXObjectCache.

https://codereview.chromium.org/1081673003/diff/160001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.h:163: RawPtrWillBeMember<AXObjectCache> m_cache;

If we keep ScopedAXObjectCache off-heap, we need to use Persistent here (it is
ok as long as ScopedAXObjectCache is not performance-sensitive).

[dmazzoni, 2015-05-28 15:10:09]

On Thu, May 28, 2015 at 1:57 AM, <haraken@chromium.org> wrote:

> Is ScopedAXObjectCache used in performance-sensitive places?
>

No, currently we use it for two things, and neither of those are
performance-sensitive:
1. Inspector lets you query the accessibility properties of one node at a
time as you explore the element tree
2. Android does a one-time snapshot of the entire accessibility tree

Your idea of keeping it off-heap sounds fine to me, it'd make it easier to
use.

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

[keishi, 2015-05-29 09:00:57]

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

https://codereview.chromium.org/1081673003/diff/160001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.h:148: class CORE_EXPORT ScopedAXObjectCache :
public NoBaseWillBeGarbageCollectedFinalized<ScopedAXObjectCache> {
On 2015/05/28 08:57:57, haraken wrote:
> 
> Is ScopedAXObjectCache used in performance-sensitive places?
> 
> If no, we might want to keep ScopedAXObjectCache off-heap. That way we can
call
> m_cache->dispose() in the destructor of ScopedAXObjectCache. Then we don't
need
> to call ->dispose() in the call sites of ScopedAXObjectCache.

Done.

[haraken, 2015-05-29 09:16:19]

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

https://codereview.chromium.org/1081673003/diff/180001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.h:162: RawPtrWillBePersistent<AXObjectCache>
m_cache;

RawPtrWillBePersistent is a bit concerning since it has a risk of producing
leaks.

Just help me understand: In non-oilpan, why is it safe to hold a raw pointer to
AXObjectCache? How is it guaranteed that Document is not disposed while the
ScopedAXObjectCache is holding the raw pointer?

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

https://codereview.chromium.org/1081673003/diff/180001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:1364:
AXObjectCache::trace(visitor);

Make this a tail call.

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

https://codereview.chromium.org/1081673003/diff/180001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.h:59:
WTF_MAKE_FAST_ALLOCATED_WILL_BE_REMOVED(AXObjectCacheImpl);

I think you can just remove this, since we have
WTF_MAKE_FAST_ALLOCATED_WILL_BE_REMOVED on AXObjectCache.

[keishi, 2015-06-05 06:00:22]

Patchset #11 (id:200001) has been deleted

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

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

https://codereview.chromium.org/1081673003/diff/180001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.h:162: RawPtrWillBePersistent<AXObjectCache>
m_cache;
On 2015/05/29 09:16:19, haraken wrote:
> 
> RawPtrWillBePersistent is a bit concerning since it has a risk of producing
> leaks.
> 
> Just help me understand: In non-oilpan, why is it safe to hold a raw pointer
to
> AXObjectCache? How is it guaranteed that Document is not disposed while the
> ScopedAXObjectCache is holding the raw pointer?

ScopedAXObjectCache and WebScopedAXContext is only used immediately after
creation so it looks safe but I think we can do something like
https://codereview.chromium.org/1167983002/ to remove the raw pointer member to
AXObjectCache.

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

https://codereview.chromium.org/1081673003/diff/180001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:1364:
AXObjectCache::trace(visitor);
On 2015/05/29 09:16:19, haraken wrote:
> 
> Make this a tail call.

Done.

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

https://codereview.chromium.org/1081673003/diff/180001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.h:59:
WTF_MAKE_FAST_ALLOCATED_WILL_BE_REMOVED(AXObjectCacheImpl);
On 2015/05/29 09:16:19, haraken wrote:
> 
> I think you can just remove this, since we have
> WTF_MAKE_FAST_ALLOCATED_WILL_BE_REMOVED on AXObjectCache.
> 

Done.

[haraken, 2015-06-08 06:39:59]

On 2015/06/08 06:27:52, keishi wrote:
>
https://codereview.chromium.org/1081673003/diff/180001/Source/core/dom/AXObje...
> File Source/core/dom/AXObjectCache.h (right):
> 
>
https://codereview.chromium.org/1081673003/diff/180001/Source/core/dom/AXObje...
> Source/core/dom/AXObjectCache.h:162: RawPtrWillBePersistent<AXObjectCache>
> m_cache;
> On 2015/05/29 09:16:19, haraken wrote:
> > 
> > RawPtrWillBePersistent is a bit concerning since it has a risk of producing
> > leaks.
> > 
> > Just help me understand: In non-oilpan, why is it safe to hold a raw pointer
> to
> > AXObjectCache? How is it guaranteed that Document is not disposed while the
> > ScopedAXObjectCache is holding the raw pointer?
> 
> ScopedAXObjectCache and WebScopedAXContext is only used immediately after
> creation so it looks safe but I think we can do something like
> https://codereview.chromium.org/1167983002/ to remove the raw pointer member
to
> AXObjectCache.

That makes much more sense to me. Are you intending to land
https://codereview.chromium.org/1167983002/ instead of this CL, right?

[keishi, 2015-06-08 06:42:24]

On 2015/06/08 06:39:59, haraken wrote:
> On 2015/06/08 06:27:52, keishi wrote:
> >
>
https://codereview.chromium.org/1081673003/diff/180001/Source/core/dom/AXObje...
> > File Source/core/dom/AXObjectCache.h (right):
> > 
> >
>
https://codereview.chromium.org/1081673003/diff/180001/Source/core/dom/AXObje...
> > Source/core/dom/AXObjectCache.h:162: RawPtrWillBePersistent<AXObjectCache>
> > m_cache;
> > On 2015/05/29 09:16:19, haraken wrote:
> > > 
> > > RawPtrWillBePersistent is a bit concerning since it has a risk of
producing
> > > leaks.
> > > 
> > > Just help me understand: In non-oilpan, why is it safe to hold a raw
pointer
> > to
> > > AXObjectCache? How is it guaranteed that Document is not disposed while
the
> > > ScopedAXObjectCache is holding the raw pointer?
> > 
> > ScopedAXObjectCache and WebScopedAXContext is only used immediately after
> > creation so it looks safe but I think we can do something like
> > https://codereview.chromium.org/1167983002/ to remove the raw pointer member
> to
> > AXObjectCache.
> 
> That makes much more sense to me. Are you intending to land
> https://codereview.chromium.org/1167983002/ instead of this CL, right?

Yes. Then I will reupload 1167983002 to this issue for review.

[haraken, 2015-06-08 06:53:06]

On 2015/06/08 06:42:24, keishi wrote:
> On 2015/06/08 06:39:59, haraken wrote:
> > On 2015/06/08 06:27:52, keishi wrote:
> > >
> >
>
https://codereview.chromium.org/1081673003/diff/180001/Source/core/dom/AXObje...
> > > File Source/core/dom/AXObjectCache.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1081673003/diff/180001/Source/core/dom/AXObje...
> > > Source/core/dom/AXObjectCache.h:162: RawPtrWillBePersistent<AXObjectCache>
> > > m_cache;
> > > On 2015/05/29 09:16:19, haraken wrote:
> > > > 
> > > > RawPtrWillBePersistent is a bit concerning since it has a risk of
> producing
> > > > leaks.
> > > > 
> > > > Just help me understand: In non-oilpan, why is it safe to hold a raw
> pointer
> > > to
> > > > AXObjectCache? How is it guaranteed that Document is not disposed while
> the
> > > > ScopedAXObjectCache is holding the raw pointer?
> > > 
> > > ScopedAXObjectCache and WebScopedAXContext is only used immediately after
> > > creation so it looks safe but I think we can do something like
> > > https://codereview.chromium.org/1167983002/ to remove the raw pointer
member
> > to
> > > AXObjectCache.
> > 
> > That makes much more sense to me. Are you intending to land
> > https://codereview.chromium.org/1167983002/ instead of this CL, right?
> 
> Yes. Then I will reupload 1167983002 to this issue for review.

Yes, let's do that.

[keishi, 2015-06-08 10:44:20]

On 2015/06/08 06:53:06, haraken wrote:
> On 2015/06/08 06:42:24, keishi wrote:
> > On 2015/06/08 06:39:59, haraken wrote:
> > > On 2015/06/08 06:27:52, keishi wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1081673003/diff/180001/Source/core/dom/AXObje...
> > > > File Source/core/dom/AXObjectCache.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1081673003/diff/180001/Source/core/dom/AXObje...
> > > > Source/core/dom/AXObjectCache.h:162:
RawPtrWillBePersistent<AXObjectCache>
> > > > m_cache;
> > > > On 2015/05/29 09:16:19, haraken wrote:
> > > > > 
> > > > > RawPtrWillBePersistent is a bit concerning since it has a risk of
> > producing
> > > > > leaks.
> > > > > 
> > > > > Just help me understand: In non-oilpan, why is it safe to hold a raw
> > pointer
> > > > to
> > > > > AXObjectCache? How is it guaranteed that Document is not disposed
while
> > the
> > > > > ScopedAXObjectCache is holding the raw pointer?
> > > > 
> > > > ScopedAXObjectCache and WebScopedAXContext is only used immediately
after
> > > > creation so it looks safe but I think we can do something like
> > > > https://codereview.chromium.org/1167983002/ to remove the raw pointer
> member
> > > to
> > > > AXObjectCache.
> > > 
> > > That makes much more sense to me. Are you intending to land
> > > https://codereview.chromium.org/1167983002/ instead of this CL, right?
> > 
> > Yes. Then I will reupload 1167983002 to this issue for review.
> 
> Yes, let's do that.

Done. PTAL

[haraken, 2015-06-08 11:03:13]

LGTM

https://codereview.chromium.org/1081673003/diff/240001/Source/core/dom/AXObje...
File Source/core/dom/AXObjectCache.cpp (right):

https://codereview.chromium.org/1081673003/diff/240001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.cpp:66: m_cache =
AXObjectCache::create(*m_document);

A slightly better way would be to create the cache lazily in get(). See the
comment below.

https://codereview.chromium.org/1081673003/diff/240001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.cpp:77: if (m_cache)

You can do something like:

if (m_document->axObjectCache())
  return m_document->axObjectCache();
if (!m_cache)
  m_cache = AXObjectCache::create(*m_document);
return m_cache.get();

[dmazzoni, 2015-06-08 15:04:05]

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

[dmazzoni, 2015-06-08 15:04:06]

lgtm with just one concern about a suggested change.

https://codereview.chromium.org/1081673003/diff/240001/Source/core/dom/AXObje...
File Source/core/dom/AXObjectCache.cpp (right):

https://codereview.chromium.org/1081673003/diff/240001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.cpp:77: if (m_cache)
On 2015/06/08 11:03:13, haraken wrote:
> 
> You can do something like:
> 
> if (m_document->axObjectCache())
>   return m_document->axObjectCache();
> if (!m_cache)
>   m_cache = AXObjectCache::create(*m_document);
> return m_cache.get();

Doing this assumes that the accessibility flag doesn't change. Suppose someone
creates a ScopedAXObjectCache, but then enables the accessibility flag before
disposing of it - then a subsequent call to get() would return a different
object.

I don't expect this should happen on purpose, but still I'd prefer that this
code doesn't make assumptions it shouldn't have to.

[haraken, 2015-06-08 23:38:15]

https://codereview.chromium.org/1081673003/diff/240001/Source/core/dom/AXObje...
File Source/core/dom/AXObjectCache.cpp (right):

https://codereview.chromium.org/1081673003/diff/240001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.cpp:77: if (m_cache)
On 2015/06/08 15:04:06, dmazzoni wrote:
> On 2015/06/08 11:03:13, haraken wrote:
> > 
> > You can do something like:
> > 
> > if (m_document->axObjectCache())
> >   return m_document->axObjectCache();
> > if (!m_cache)
> >   m_cache = AXObjectCache::create(*m_document);
> > return m_cache.get();
> 
> Doing this assumes that the accessibility flag doesn't change. Suppose someone
> creates a ScopedAXObjectCache, but then enables the accessibility flag before
> disposing of it - then a subsequent call to get() would return a different
> object.
> 
> I don't expect this should happen on purpose, but still I'd prefer that this
> code doesn't make assumptions it shouldn't have to.

Makes sense -- let's go with the current CL.

[keishi, 2015-06-22 02:00:38]

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

[keishi, 2015-06-22 02:00:39]

+tkent , could you review the change to public/web/WebAXObject.h ? Thanks.

[tkent, 2015-06-22 03:14:47]

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

https://codereview.chromium.org/1081673003/diff/240001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.h:148: class CORE_EXPORT ScopedAXObjectCache {
Please explain the reason of this change in the CL description.

https://codereview.chromium.org/1081673003/diff/240001/public/web/WebAXObject.h
File public/web/WebAXObject.h (left):

https://codereview.chromium.org/1081673003/diff/240001/public/web/WebAXObject...
public/web/WebAXObject.h:62: BLINK_EXPORT ~WebScopedAXContext();
Please do not remove the destructor.

WebPrivatePtr.h:
> // Note: you must call reset(0) on the implementation side in order to delete
> // the WebCore pointer.

[tkent, 2015-06-22 03:15:10]

> WebPrivatePtr.h:
> > // Note: you must call reset(0) on the implementation side in order to
delete
> > // the WebCore pointer.

WebPrivatePtr.h -> WebPrivateOwnPtr.h

[keishi, 2015-06-22 05:23:56]

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

https://codereview.chromium.org/1081673003/diff/240001/Source/core/dom/AXObje...
Source/core/dom/AXObjectCache.h:148: class CORE_EXPORT ScopedAXObjectCache {
On 2015/06/22 03:14:47, tkent wrote:
> Please explain the reason of this change in the CL description.

Done.

https://codereview.chromium.org/1081673003/diff/240001/public/web/WebAXObject.h
File public/web/WebAXObject.h (left):

https://codereview.chromium.org/1081673003/diff/240001/public/web/WebAXObject...
public/web/WebAXObject.h:62: BLINK_EXPORT ~WebScopedAXContext();
On 2015/06/22 03:14:47, tkent wrote:
> Please do not remove the destructor.
> 
> WebPrivatePtr.h:
> > // Note: you must call reset(0) on the implementation side in order to
delete
> > // the WebCore pointer.

Done.

[tkent, 2015-06-22 05:32:50]

lgtm

[keishi, 2015-06-22 08:14:11]

The CQ bit was checked by keishi@chromium.org

[keishi, 2015-06-22 08:14:11]

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

[commit-bot: I haz the power, 2015-06-22 08:14:21]

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

[commit-bot: I haz the power, 2015-06-22 09:32:50]

Committed patchset #13 (id:260001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197561