Oilpan: move MHTML objects to the heap.

Oilpan: move MHTML objects to the heap.

R=haraken@chromium.org,ager@chromium.org,tkent@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177719

[sof, 2014-07-08 08:15:39]

Please take a look.

[haraken, 2014-07-08 08:50:08]

LGTM

https://codereview.chromium.org/374053002/diff/60001/Source/platform/mhtml/MH...
File Source/platform/mhtml/MHTMLArchive.cpp (right):

https://codereview.chromium.org/374053002/diff/60001/Source/platform/mhtml/MH...
Source/platform/mhtml/MHTMLArchive.cpp:220: void
MHTMLArchive::clearAllSubframeArchives()

Can we add #if !ENABLE(OILPAN) to this method?

https://codereview.chromium.org/374053002/diff/60001/Source/platform/mhtml/MH...
Source/platform/mhtml/MHTMLArchive.cpp:220: void
MHTMLArchive::clearAllSubframeArchives()

I'm curious why we need to call clearAllSubframeArchives() even in non-oilpan
builds. What clearAllSubframeArchives() does is just to call
m_subframeArchives.clear() for all subframes. I don't think this is needed,
because m_subframeArchives.clear() will be called in the destructor of the
subframes.

In other words, if we really need to call clearAllSubframeArchives() in
non-oilpan builds for some reason (e.g., we want to forcibly clear archives of
the subframes even though there are other retainers of the subframes somewhere),
won't it mean that we want to call clearAllSubframeArchives() in oilpan builds
as well?

https://codereview.chromium.org/374053002/diff/60001/Source/platform/mhtml/MH...
Source/platform/mhtml/MHTMLArchive.cpp:226: void
MHTMLArchive::clearAllSubframeArchivesImpl(SubFrameArchives* clearedArchives)

Ditto.

[sof, 2014-07-08 09:10:00]

https://codereview.chromium.org/374053002/diff/60001/Source/platform/mhtml/MH...
File Source/platform/mhtml/MHTMLArchive.cpp (right):

https://codereview.chromium.org/374053002/diff/60001/Source/platform/mhtml/MH...
Source/platform/mhtml/MHTMLArchive.cpp:220: void
MHTMLArchive::clearAllSubframeArchives()
On 2014/07/08 08:50:08, haraken wrote:
> 
> I'm curious why we need to call clearAllSubframeArchives() even in non-oilpan
> builds. What clearAllSubframeArchives() does is just to call
> m_subframeArchives.clear() for all subframes. I don't think this is needed,
> because m_subframeArchives.clear() will be called in the destructor of the
> subframes.
> 
> In other words, if we really need to call clearAllSubframeArchives() in
> non-oilpan builds for some reason (e.g., we want to forcibly clear archives of
> the subframes even though there are other retainers of the subframes
somewhere),
> won't it mean that we want to call clearAllSubframeArchives() in oilpan builds
> as well?

I think you're right, I didn't see the point of this operation either, but
didn't want to upset the apple cart. The operation being to DFS visit all
archives and then deref() them all in the end.

The deref() storm that will happen when the vector is destructed should take
care of this already. The comment in the dtor makes me uncertain if the thinking
is that there might be loops here needing this..in which case we'll never get
here due to the use of RefPtrs.

[haraken, 2014-07-08 09:14:26]

+jcivelli

> > I'm curious why we need to call clearAllSubframeArchives() even in
non-oilpan
> > builds. What clearAllSubframeArchives() does is just to call
> > m_subframeArchives.clear() for all subframes. I don't think this is needed,
> > because m_subframeArchives.clear() will be called in the destructor of the
> > subframes.
> > 
> > In other words, if we really need to call clearAllSubframeArchives() in
> > non-oilpan builds for some reason (e.g., we want to forcibly clear archives
of
> > the subframes even though there are other retainers of the subframes
> somewhere),
> > won't it mean that we want to call clearAllSubframeArchives() in oilpan
builds
> > as well?
> 
> I think you're right, I didn't see the point of this operation either, but
> didn't want to upset the apple cart. The operation being to DFS visit all
> archives and then deref() them all in the end.
> 
> The deref() storm that will happen when the vector is destructed should take
> care of this already. The comment in the dtor makes me uncertain if the
thinking
> is that there might be loops here needing this..in which case we'll never get
> here due to the use of RefPtrs.

Understood. Either way removing the destruction logic from oilpan builds looks
correct to me.

[Mads Ager (chromium), 2014-07-08 09:39:42]

LGTM2

I don't see how the clearing DFS adds anything in the non-oilpan build either.

[sof, 2014-07-08 09:49:55]

On 2014/07/08 09:39:42, Mads Ager (chromium) wrote:
> LGTM2
> 
> I don't see how the clearing DFS adds anything in the non-oilpan build either.

I looked closer at the original patch and MHTMLArchive::create(), and now it
makes sense to me non-Oilpan :) The top frame/document is kept out of the
bi-directional RefPtr construction, but the sub archive objects are otherwise
wired up to refer to each other.

Explicitly clearing out the ref counts as done is then needed to prevent cycles
keeping these sub archive objects alive when the top frame goes away. Not needed
for Oilpan, where cycles aren't a problem.

The clear*() methods are now only defined with !ENABLE(OILPAN).

[sof, 2014-07-08 09:55:15]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-07-08 09:56:27]

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

[sof, 2014-07-08 10:01:39]

Ah, needs Source/{platform,web}/ approval.

[Mads Ager (chromium), 2014-07-08 10:07:13]

On 2014/07/08 09:49:55, sof wrote:
> On 2014/07/08 09:39:42, Mads Ager (chromium) wrote:
> > LGTM2
> > 
> > I don't see how the clearing DFS adds anything in the non-oilpan build
either.
> 
> I looked closer at the original patch and MHTMLArchive::create(), and now it
> makes sense to me non-Oilpan :) The top frame/document is kept out of the
> bi-directional RefPtr construction, but the sub archive objects are otherwise
> wired up to refer to each other.
> 
> Explicitly clearing out the ref counts as done is then needed to prevent
cycles
> keeping these sub archive objects alive when the top frame goes away. Not
needed
> for Oilpan, where cycles aren't a problem.
> 
> The clear*() methods are now only defined with !ENABLE(OILPAN).

Thanks for the clearification! :)

[commit-bot: I haz the power, 2014-07-08 10:52:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/9571)

[commit-bot: I haz the power, 2014-07-08 10:58:01]

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

[commit-bot: I haz the power, 2014-07-08 10:58:02]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/9573)

[sof, 2014-07-08 15:05:56]

tkent@: if you could look through this CL when you have a moment next, that'd be
great. tia.

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

https://codereview.chromium.org/374053002/diff/80001/Source/web/WebPageSerial...
File Source/web/WebPageSerializer.cpp (right):

https://codereview.chromium.org/374053002/diff/80001/Source/web/WebPageSerial...
Source/web/WebPageSerializer.cpp:47: #include "platform/mhtml/ArchiveResource.h"
Why do we need to include it? It seems ArchiveResource-related functions are not
used in this file.

[sof, 2014-07-09 04:42:19]

https://codereview.chromium.org/374053002/diff/80001/Source/web/WebPageSerial...
File Source/web/WebPageSerializer.cpp (right):

https://codereview.chromium.org/374053002/diff/80001/Source/web/WebPageSerial...
Source/web/WebPageSerializer.cpp:47: #include "platform/mhtml/ArchiveResource.h"
On 2014/07/09 01:04:34, tkent wrote:
> Why do we need to include it? It seems ArchiveResource-related functions are
not
> used in this file.
> 

MHTMLArchive inlines a vector append() so as to require having the full type of
ArchiveResource in scope. We can either "outline" it or revert back to having a
full #include of "ArchiveResource.h" in "MHTMLArchive.h"

[tkent, 2014-07-09 04:49:25]

lgtm

https://codereview.chromium.org/374053002/diff/80001/Source/web/WebPageSerial...
File Source/web/WebPageSerializer.cpp (right):

https://codereview.chromium.org/374053002/diff/80001/Source/web/WebPageSerial...
Source/web/WebPageSerializer.cpp:47: #include "platform/mhtml/ArchiveResource.h"
On 2014/07/09 04:42:19, sof wrote:
> On 2014/07/09 01:04:34, tkent wrote:
> > Why do we need to include it? It seems ArchiveResource-related functions are
> not
> > used in this file.
> > 
> 
> MHTMLArchive inlines a vector append() so as to require having the full type
of
> ArchiveResource in scope. We can either "outline" it or revert back to having
a
> full #include of "ArchiveResource.h" in "MHTMLArchive.h"

I see.  So this include looks wrong to me.
Please do either.

[sof, 2014-07-09 05:11:20]

https://codereview.chromium.org/374053002/diff/80001/Source/web/WebPageSerial...
File Source/web/WebPageSerializer.cpp (right):

https://codereview.chromium.org/374053002/diff/80001/Source/web/WebPageSerial...
Source/web/WebPageSerializer.cpp:47: #include "platform/mhtml/ArchiveResource.h"
On 2014/07/09 04:49:24, tkent wrote:
> On 2014/07/09 04:42:19, sof wrote:
> > On 2014/07/09 01:04:34, tkent wrote:
> > > Why do we need to include it? It seems ArchiveResource-related functions
are
> > not
> > > used in this file.
> > > 
> > 
> > MHTMLArchive inlines a vector append() so as to require having the full type
> of
> > ArchiveResource in scope. We can either "outline" it or revert back to
having
> a
> > full #include of "ArchiveResource.h" in "MHTMLArchive.h"
> 
> I see.  So this include looks wrong to me.
> Please do either.
> 

Yes, removed. To make MHTMLArchive.h closed, I moved the definitions of these
helpers to the implementation file.

(Looked over the other #includes of "ArchiveResource.h" added in this CL;
they're needed.)

[tkent, 2014-07-09 05:24:21]

lgtm

[sof, 2014-07-09 05:27:19]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-07-09 05:28:04]

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

[commit-bot: I haz the power, 2014-07-09 06:30:10]

Change committed as 177719