Oilpan: enable appcache + move DocumentLoader to the heap.

Oilpan: enable appcache + move DocumentLoader to the heap.

DocumentLoader belongs on the heap. References to it
are kept by an object that is on the heap (FrameLoader),
and DocumentLoader itself has heap references.

Along with moving DocumentLoader, have the two appcache
objects (ApplicationCache and ApplicationCacheHost) be
under Oilpan's control always.

R=haraken
BUG=340522, 497595
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197576

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

[sof, 2015-06-21 15:46:04]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-06-21 15:46:05]

please take a look.

Long overdue DocumentLoader treatment.

[haraken, 2015-06-22 01:57:57]

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

[haraken, 2015-06-22 01:57:59]

Thanks for working on this!

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/Doc...
File Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/Doc...
Source/core/loader/DocumentLoader.cpp:117: m_applicationCacheHost->dispose();

I'm wondering if we could move this to detachFromFrame. But I'm also wondering
if we could entirely remove the dispose() call. See my comment in
ApplicationCacheHost.

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/Doc...
Source/core/loader/DocumentLoader.cpp:124: // TODO(sof): start tracing
ResourcePtr<>s (and m_mainResource.)

I don't quite remember the current status of ResourcePtr, but do we need to
trace them or not?

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/Doc...
File Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/Doc...
Source/core/loader/DocumentLoader.h:145: EAGERLY_FINALIZE();

Can we add a comment for why this needs to be eagerly finalized?

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/Doc...
Source/core/loader/DocumentLoader.h:188: LocalFrame* m_frame;

Can we make this a Member?

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
File Source/core/loader/appcache/ApplicationCacheHost.cpp (right):

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
Source/core/loader/appcache/ApplicationCacheHost.cpp:169: #if !ENABLE(OILPAN)

Can we move the #if !ENABLE(OILPAN) to the entire dispose() method?

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
Source/core/loader/appcache/ApplicationCacheHost.cpp:177: m_documentLoader =
nullptr;

Now in non-oilpan, both DocumentLoader and ApplicationCacheHost are on the heap.
So it seems unsafe (and not needed) to clear
ApplicationCacheHost::m_documentLoader in dispose() which is called in
DocumentLoader's destructor.

Maybe can we entirely remove the dispose() method from both oilpan and
non-oilpan?

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
File Source/core/loader/appcache/ApplicationCacheHost.h (right):

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
Source/core/loader/appcache/ApplicationCacheHost.h:134: EAGERLY_FINALIZE();

Add a comment why this needs to be eagerly finalized (Is it for clearing m_host,
right).

[sof, 2015-06-22 05:03:46]

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
File Source/core/loader/appcache/ApplicationCacheHost.cpp (right):

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
Source/core/loader/appcache/ApplicationCacheHost.cpp:177: m_documentLoader =
nullptr;
On 2015/06/22 01:57:59, haraken wrote:
> 
> Now in non-oilpan, both DocumentLoader and ApplicationCacheHost are on the
heap.
> So it seems unsafe (and not needed) to clear
> ApplicationCacheHost::m_documentLoader in dispose() which is called in
> DocumentLoader's destructor.
> 
> Maybe can we entirely remove the dispose() method from both oilpan and
> non-oilpan?

That will not work. The Persistent<> on DocumentLoader will keep the Host alive
for another GC iteration. In time for the host to be called and a freed
m_documentLoader to be accessed.

[haraken, 2015-06-22 05:28:56]

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
File Source/core/loader/appcache/ApplicationCacheHost.cpp (right):

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
Source/core/loader/appcache/ApplicationCacheHost.cpp:177: m_documentLoader =
nullptr;
On 2015/06/22 05:03:46, sof wrote:
> On 2015/06/22 01:57:59, haraken wrote:
> > 
> > Now in non-oilpan, both DocumentLoader and ApplicationCacheHost are on the
> heap.
> > So it seems unsafe (and not needed) to clear
> > ApplicationCacheHost::m_documentLoader in dispose() which is called in
> > DocumentLoader's destructor.
> > 
> > Maybe can we entirely remove the dispose() method from both oilpan and
> > non-oilpan?
> 
> That will not work. The Persistent<> on DocumentLoader will keep the Host
alive
> for another GC iteration. In time for the host to be called and a freed
> m_documentLoader to be accessed.

I'm not sure but maybe can we clear the Persistent<> just after calling
ApplicationCacheHost::dispose()?

[sof, 2015-06-22 05:36:02]

On 2015/06/22 05:28:56, haraken wrote:
>
https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
> File Source/core/loader/appcache/ApplicationCacheHost.cpp (right):
> 
>
https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
> Source/core/loader/appcache/ApplicationCacheHost.cpp:177: m_documentLoader =
> nullptr;
> On 2015/06/22 05:03:46, sof wrote:
> > On 2015/06/22 01:57:59, haraken wrote:
> > > 
> > > Now in non-oilpan, both DocumentLoader and ApplicationCacheHost are on the
> > heap.
> > > So it seems unsafe (and not needed) to clear
> > > ApplicationCacheHost::m_documentLoader in dispose() which is called in
> > > DocumentLoader's destructor.
> > > 
> > > Maybe can we entirely remove the dispose() method from both oilpan and
> > > non-oilpan?
> > 
> > That will not work. The Persistent<> on DocumentLoader will keep the Host
> alive
> > for another GC iteration. In time for the host to be called and a freed
> > m_documentLoader to be accessed.
> 
> I'm not sure but maybe can we clear the Persistent<> just after calling
> ApplicationCacheHost::dispose()?

Isn't it cleared by code inserted by the compiler in ~DocumentLoader just after
that dispose() call?

[haraken, 2015-06-22 05:39:21]

On 2015/06/22 05:36:02, sof wrote:
> On 2015/06/22 05:28:56, haraken wrote:
> >
>
https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
> > File Source/core/loader/appcache/ApplicationCacheHost.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
> > Source/core/loader/appcache/ApplicationCacheHost.cpp:177: m_documentLoader =
> > nullptr;
> > On 2015/06/22 05:03:46, sof wrote:
> > > On 2015/06/22 01:57:59, haraken wrote:
> > > > 
> > > > Now in non-oilpan, both DocumentLoader and ApplicationCacheHost are on
the
> > > heap.
> > > > So it seems unsafe (and not needed) to clear
> > > > ApplicationCacheHost::m_documentLoader in dispose() which is called in
> > > > DocumentLoader's destructor.
> > > > 
> > > > Maybe can we entirely remove the dispose() method from both oilpan and
> > > > non-oilpan?
> > > 
> > > That will not work. The Persistent<> on DocumentLoader will keep the Host
> > alive
> > > for another GC iteration. In time for the host to be called and a freed
> > > m_documentLoader to be accessed.
> > 
> > I'm not sure but maybe can we clear the Persistent<> just after calling
> > ApplicationCacheHost::dispose()?
> 
> Isn't it cleared by code inserted by the compiler in ~DocumentLoader just
after
> that dispose() call?

Maybe I'm misunderstanding your comment. I thought we can address:

> > > That will not work. The Persistent<> on DocumentLoader will keep the Host
> > alive
> > > for another GC iteration. In time for the host to be called and a freed
> > > m_documentLoader to be accessed.

^^^ this concern by calling ApplicationCacheHost::dispose and
m_applicationCacheHost.clear() at the same time (i.e., in detachFromFrame).

[sof, 2015-06-22 05:56:36]

On 2015/06/22 05:39:21, haraken wrote:
> On 2015/06/22 05:36:02, sof wrote:
> > On 2015/06/22 05:28:56, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
> > > File Source/core/loader/appcache/ApplicationCacheHost.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
> > > Source/core/loader/appcache/ApplicationCacheHost.cpp:177: m_documentLoader
=
> > > nullptr;
> > > On 2015/06/22 05:03:46, sof wrote:
> > > > On 2015/06/22 01:57:59, haraken wrote:
> > > > > 
> > > > > Now in non-oilpan, both DocumentLoader and ApplicationCacheHost are on
> the
> > > > heap.
> > > > > So it seems unsafe (and not needed) to clear
> > > > > ApplicationCacheHost::m_documentLoader in dispose() which is called in
> > > > > DocumentLoader's destructor.
> > > > > 
> > > > > Maybe can we entirely remove the dispose() method from both oilpan and
> > > > > non-oilpan?
> > > > 
> > > > That will not work. The Persistent<> on DocumentLoader will keep the
Host
> > > alive
> > > > for another GC iteration. In time for the host to be called and a freed
> > > > m_documentLoader to be accessed.
> > > 
> > > I'm not sure but maybe can we clear the Persistent<> just after calling
> > > ApplicationCacheHost::dispose()?
> > 
> > Isn't it cleared by code inserted by the compiler in ~DocumentLoader just
> after
> > that dispose() call?
> 
> Maybe I'm misunderstanding your comment. I thought we can address:
> 
> > > > That will not work. The Persistent<> on DocumentLoader will keep the
Host
> > > alive
> > > > for another GC iteration. In time for the host to be called and a freed
> > > > m_documentLoader to be accessed.
> 
> ^^^ this concern by calling ApplicationCacheHost::dispose and
> m_applicationCacheHost.clear() at the same time (i.e., in detachFromFrame).

Sure, we can try to move a line of code out of the destructor. but dispose() is
needed !ENABLE(OILPAN).

[sof, 2015-06-22 06:49:18]

Given that we have an explicit detach step over DocumentLoader, we can extend
that to ApplicationCacheHost.

I think that leads to a more balanced treatment of destruction/disposal. to wit,
eager finalization no longer reqd.

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/Doc...
File Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/Doc...
Source/core/loader/DocumentLoader.cpp:124: // TODO(sof): start tracing
ResourcePtr<>s (and m_mainResource.)
On 2015/06/22 01:57:58, haraken wrote:
> 
> I don't quite remember the current status of ResourcePtr, but do we need to
> trace them or not?

We definitely ought to when present on Oilpan heap-residing objects, but not
currently done.

http://crbug.com/393516 for Resource handling matters.

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/Doc...
File Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/Doc...
Source/core/loader/DocumentLoader.h:145: EAGERLY_FINALIZE();
On 2015/06/22 01:57:58, haraken wrote:
> 
> Can we add a comment for why this needs to be eagerly finalized?

I've removed it, as DocumentLoader currently requires that an explicit detach is
first called. We can take care of the 'eager' operations via that detach
operation instead.

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/Doc...
Source/core/loader/DocumentLoader.h:188: LocalFrame* m_frame;
On 2015/06/22 01:57:58, haraken wrote:
> 
> Can we make this a Member?

Done.

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
File Source/core/loader/appcache/ApplicationCacheHost.cpp (right):

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
Source/core/loader/appcache/ApplicationCacheHost.cpp:169: #if !ENABLE(OILPAN)
On 2015/06/22 01:57:58, haraken wrote:
> 
> Can we move the #if !ENABLE(OILPAN) to the entire dispose() method?

Renamed as detachFromDocumentLoader() instead (cf. detachFromFrame() naming on
DocumentLoader), and have it be called always (from detachFromFrame().)

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
File Source/core/loader/appcache/ApplicationCacheHost.h (right):

https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/app...
Source/core/loader/appcache/ApplicationCacheHost.h:134: EAGERLY_FINALIZE();
On 2015/06/22 01:57:59, haraken wrote:
> 
> Add a comment why this needs to be eagerly finalized (Is it for clearing
m_host,
> right).

No longer needed; asserted for that in the destructor.

[sof, 2015-06-22 06:55:16]

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

https://codereview.chromium.org/1194003004/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:2656: RefPtrWillBeRawPtr<DocumentLoader>
documentLoader =  m_frame->loader().provisionalDocumentLoader();
Redundant space present on RHS; removed.

[haraken, 2015-06-22 07:13:17]

LGTM, much nicer!

[sof, 2015-06-22 07:45:23]

will wait on oilpan results.

[sof, 2015-06-22 11:23:52]

The "FrameLoader always detaches its DocumentLoaders" only held 95% of the time,
so some more changes needed to enforce that invariant.

ptal? Changes to FrameLoader and DocumentLoader only.

[haraken, 2015-06-22 11:51:31]

On 2015/06/22 11:23:52, sof wrote:
> The "FrameLoader always detaches its DocumentLoaders" only held 95% of the
time,
> so some more changes needed to enforce that invariant.
> 
> ptal? Changes to FrameLoader and DocumentLoader only.

LGTM

[sof, 2015-06-22 14:04:30]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-06-22 14:04:40]

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

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

Committed patchset #11 (id:190001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197576

[Stephen Chennney, 2015-06-22 17:56:49]

A revert of this CL (patchset #11 id:190001) has been created in
https://codereview.chromium.org/1200043002/ by schenney@chromium.org.

The reason for reverting is: Linux Leak is failing on
fast/html/imports/import-expando-gc.html, and this seems to be the most viable
candidate for the cause.

See
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/bu...
and
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas....

[sof, 2015-06-23 15:17:42]

The initial landing attempt got caught up in some unrelated test instability;
addressed by http://code.google.com/p/chromium/issues/detail?id=503583.

Relanding.

[sof, 2015-06-23 15:17:48]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-06-23 15:17:49]

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

[commit-bot: I haz the power, 2015-06-23 15:18:08]

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

[commit-bot: I haz the power, 2015-06-23 16:06:59]

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

[commit-bot: I haz the power, 2015-06-23 16:07:01]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60320)

[sof, 2015-06-23 16:20:08]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-06-23 16:20:38]

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

[commit-bot: I haz the power, 2015-06-23 17:10:37]

Committed patchset #12 (id:210001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197664