Oilpan: dispose an HTMLImportsController on document detach/removal.

Oilpan: dispose an HTMLImportsController on document detach/removal.

When a Document becomes detached and removes its HTMLImportsController
supplement, this will synchronously remove the controller and all
its "HTML imports" owned objects, in a non-Oilpan setting. This cancels
all ongoing loaders and more (cf. HTMLImportsController::removeFrom().)

ResourceFetcher objects are moving to accurately detaching from their
FrameFetchContexts when their frames become detached from a document
(http://crbug.com/458222). This implies that we cannot have loader objects
active, as that may cause FrameFetchContexts to be accessed, but once
lacking a frame. The code is no longer that expecting that.

For HTML imports loaders, this is avoided by synchronously cancelling &
finalizing the loaders, but that doesn't happen for Oilpan. It needs to,
so an explicit dispose() step is introduced for HTMLImportsController
and all its associated objects.

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

[sof, 2015-03-26 11:30:24]

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

[sof, 2015-03-26 11:30:25]

please take a look.

Test results are a bit noisy atm, but none of the failures are related. (Still
waiting on the oilpan bots tho.)

[haraken, 2015-03-26 23:27:26]

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

[haraken, 2015-03-26 23:27:27]

https://codereview.chromium.org/1032293002/diff/1/Source/core/html/imports/HT...
File Source/core/html/imports/HTMLImportsController.cpp (right):

https://codereview.chromium.org/1032293002/diff/1/Source/core/html/imports/HT...
Source/core/html/imports/HTMLImportsController.cpp:58: void
HTMLImportsController::removeFrom(Document& master)

It seems that HTMLImportsController::removeFrom is not at all called in oilpan
(HTMLImportsController::removeFrom is called only in !ENABLE(OILPAN)).

Probably do we need to call HTMLImportsController::dispose in
HTMLImportsController's pre-finalizer?

Even if I'm missing something and HTMLImportsController::removeFrom is called
somewhere, is it guaranteed that HTMLImportsController::removeFrom is _always_
called for all the HTMLImportsControllers that are going to be destructed? If
no, that would be problematic. If yes, we won't need to call dispose() in
~HTMLImportsController().

https://codereview.chromium.org/1032293002/diff/1/Source/core/html/imports/HT...
Source/core/html/imports/HTMLImportsController.cpp:76: dispose();

As described above, I'm wondering why we need to call dispose() here.

https://codereview.chromium.org/1032293002/diff/1/Source/core/html/imports/HT...
Source/core/html/imports/HTMLImportsController.cpp:77: #endif

In oilpan, we want to have an ASSERT to verify that dispose() has been called.
The same comment for other dispose()s in this CL.

[sof, 2015-03-27 06:30:24]

https://codereview.chromium.org/1032293002/diff/1/Source/core/html/imports/HT...
File Source/core/html/imports/HTMLImportsController.cpp (right):

https://codereview.chromium.org/1032293002/diff/1/Source/core/html/imports/HT...
Source/core/html/imports/HTMLImportsController.cpp:58: void
HTMLImportsController::removeFrom(Document& master)
On 2015/03/26 23:27:26, haraken wrote:
> 
> It seems that HTMLImportsController::removeFrom is not at all called in oilpan
> (HTMLImportsController::removeFrom is called only in !ENABLE(OILPAN)).
> 
> Probably do we need to call HTMLImportsController::dispose in
> HTMLImportsController's pre-finalizer?
> 
> Even if I'm missing something and HTMLImportsController::removeFrom is called
> somewhere, is it guaranteed that HTMLImportsController::removeFrom is _always_
> called for all the HTMLImportsControllers that are going to be destructed? If
> no, that would be problematic. If yes, we won't need to call dispose() in
> ~HTMLImportsController().

removeFrom() is called from Document upon detach; see the Document code +
https://codereview.chromium.org/1035803002/ for code that touches on it (and
triggers the need for this CL.)

[haraken, 2015-03-27 06:59:38]

On 2015/03/27 06:30:24, sof wrote:
>
https://codereview.chromium.org/1032293002/diff/1/Source/core/html/imports/HT...
> File Source/core/html/imports/HTMLImportsController.cpp (right):
> 
>
https://codereview.chromium.org/1032293002/diff/1/Source/core/html/imports/HT...
> Source/core/html/imports/HTMLImportsController.cpp:58: void
> HTMLImportsController::removeFrom(Document& master)
> On 2015/03/26 23:27:26, haraken wrote:
> > 
> > It seems that HTMLImportsController::removeFrom is not at all called in
oilpan
> > (HTMLImportsController::removeFrom is called only in !ENABLE(OILPAN)).
> > 
> > Probably do we need to call HTMLImportsController::dispose in
> > HTMLImportsController's pre-finalizer?
> > 
> > Even if I'm missing something and HTMLImportsController::removeFrom is
called
> > somewhere, is it guaranteed that HTMLImportsController::removeFrom is
_always_
> > called for all the HTMLImportsControllers that are going to be destructed?
If
> > no, that would be problematic. If yes, we won't need to call dispose() in
> > ~HTMLImportsController().
> 
> removeFrom() is called from Document upon detach; see the Document code +
> https://codereview.chromium.org/1035803002/ for code that touches on it (and
> triggers the need for this CL.)

Ah, understood, thanks.

Then removedFrom() is not guaranteed to be called for all HTMLImportControllers
that die, and thus dispose() is not guaranteed to be called for everything,
right?

[sof, 2015-03-27 07:24:49]

On 2015/03/27 06:59:38, haraken wrote:
> On 2015/03/27 06:30:24, sof wrote:
> >
>
https://codereview.chromium.org/1032293002/diff/1/Source/core/html/imports/HT...
> > File Source/core/html/imports/HTMLImportsController.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1032293002/diff/1/Source/core/html/imports/HT...
> > Source/core/html/imports/HTMLImportsController.cpp:58: void
> > HTMLImportsController::removeFrom(Document& master)
> > On 2015/03/26 23:27:26, haraken wrote:
> > > 
> > > It seems that HTMLImportsController::removeFrom is not at all called in
> oilpan
> > > (HTMLImportsController::removeFrom is called only in !ENABLE(OILPAN)).
> > > 
> > > Probably do we need to call HTMLImportsController::dispose in
> > > HTMLImportsController's pre-finalizer?
> > > 
> > > Even if I'm missing something and HTMLImportsController::removeFrom is
> called
> > > somewhere, is it guaranteed that HTMLImportsController::removeFrom is
> _always_
> > > called for all the HTMLImportsControllers that are going to be destructed?
> If
> > > no, that would be problematic. If yes, we won't need to call dispose() in
> > > ~HTMLImportsController().
> > 
> > removeFrom() is called from Document upon detach; see the Document code +
> > https://codereview.chromium.org/1035803002/ for code that touches on it (and
> > triggers the need for this CL.)
> 
> Ah, understood, thanks.
> 
> Then removedFrom() is not guaranteed to be called for all
HTMLImportControllers
> that die, and thus dispose() is not guaranteed to be called for everything,
> right?

Yes, for Oilpan. But it will now be called where it matters, i.e., where there's
a "clean" detach from the frame first and we need to ensure that no htmlimports/
object live longer under the assumption that they can locate a valid frame().

I think we can empty the destructor non-Oilpan, as removeFrom() is called from
~Document, I can certainly try.

[sof, 2015-03-27 08:44:31]

https://codereview.chromium.org/1032293002/diff/1/Source/core/html/imports/HT...
File Source/core/html/imports/HTMLImportsController.cpp (right):

https://codereview.chromium.org/1032293002/diff/1/Source/core/html/imports/HT...
Source/core/html/imports/HTMLImportsController.cpp:76: dispose();
On 2015/03/26 23:27:26, haraken wrote:
> 
> As described above, I'm wondering why we need to call dispose() here.

You're right, it's not needed. Replaced with a non-oilpan assert.

[haraken, 2015-03-27 11:02:41]

LGTM

The last thing I don't fully understand is why it was OK not to have
HTMLImportController::removeFrom() in Document::detach() in oilpan builds.

[sof, 2015-03-27 12:54:17]

On 2015/03/27 11:02:41, haraken wrote:
> LGTM
> 
> The last thing I don't fully understand is why it was OK not to have
> HTMLImportController::removeFrom() in Document::detach() in oilpan builds.

Before https://codereview.chromium.org/1035803002/ , the fetcher object would be
resistant to being frame-detached, so it didn't matter.

It does with its introduction -
https://codereview.chromium.org/1035803002/#msg10 needs to be addressed though,
i think.

[sof, 2015-03-27 13:14:07]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-03-27 13:14:26]

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

[commit-bot: I haz the power, 2015-03-27 13:18:44]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192677