Oilpan: switch ImageLoader to be prefinalized.

Oilpan: switch ImageLoader to be prefinalized.

If an ImageLoader for an SVGImage ends up being swept out along with a
FrameView (a ScrollableArea), the former may circuitously end up accessing
the latter via its destructor. Having both be eagerly finalized, or
prefinalized, doesn't guarantee a correct destruction order.

Hence, to address, arrange for the ImageLoader to be finalized/disposed of
first by way of a prefinalizing action.

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

[sof, 2015-07-13 05:17:30]

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

[sof, 2015-07-13 05:17:30]

please take a look.

As mentioned a couple of times, we've got a finalization issue related to
SVGImage that needs to be addressed. It surfaced after making ScrollableAreas
eagerly finalized. As can be seen here,


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...

I really don't want to have to rely on a double phase eager finalization
mechanism, but I'm at loss at how to otherwise bring about order.

[haraken, 2015-07-13 05:20:18]

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

[haraken, 2015-07-13 05:20:18]

LGTM

[sof, 2015-07-13 06:46:40]

We may get away with relying on prefinalizer ordering for this case, assuming
that the ImageLoader's prefinalizer will be register after any FrameViews' are.
But I'm not convinced prefinalizer ordering will be sufficient in the end.

btw, you could argue that eager sweeping should be done in a reverse order also.

[sof, 2015-07-13 06:47:14]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-07-13 06:47:21]

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

[commit-bot: I haz the power, 2015-07-13 06:50:44]

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

[haraken, 2015-07-13 06:55:33]

On 2015/07/13 06:46:40, sof wrote:
> We may get away with relying on prefinalizer ordering for this case, assuming
> that the ImageLoader's prefinalizer will be register after any FrameViews'
are.
> But I'm not convinced prefinalizer ordering will be sufficient in the end.

I'm not yet convinced either -- but I'm thinking that a pre-finalizer would be a
more powerful and easy-to-understand way to control destruction ordering issues.

> btw, you could argue that eager sweeping should be done in a reverse order
also.

It's not easy to introduce a notion of ordering to the heap structure, isn't it?

[sof, 2015-07-13 07:04:55]

On 2015/07/13 06:55:33, haraken wrote:
> On 2015/07/13 06:46:40, sof wrote:
> > We may get away with relying on prefinalizer ordering for this case,
assuming
> > that the ImageLoader's prefinalizer will be register after any FrameViews'
> are.
> > But I'm not convinced prefinalizer ordering will be sufficient in the end.
> 
> I'm not yet convinced either -- but I'm thinking that a pre-finalizer would be
a
> more powerful and easy-to-understand way to control destruction ordering
issues.
> 
> > btw, you could argue that eager sweeping should be done in a reverse order
> also.
> 
> It's not easy to introduce a notion of ordering to the heap structure, isn't
it?

It would be approximate, yes, but we already have a page ordering as wanted wrt
allocation times, but would have to do intra-page sweeps differently (stack up
the finalize()s first.)

[haraken, 2015-07-13 07:20:49]

On 2015/07/13 07:04:55, sof wrote:
> On 2015/07/13 06:55:33, haraken wrote:
> > On 2015/07/13 06:46:40, sof wrote:
> > > We may get away with relying on prefinalizer ordering for this case,
> assuming
> > > that the ImageLoader's prefinalizer will be register after any FrameViews'
> > are.
> > > But I'm not convinced prefinalizer ordering will be sufficient in the end.
> > 
> > I'm not yet convinced either -- but I'm thinking that a pre-finalizer would
be
> a
> > more powerful and easy-to-understand way to control destruction ordering
> issues.
> > 
> > > btw, you could argue that eager sweeping should be done in a reverse order
> > also.
> > 
> > It's not easy to introduce a notion of ordering to the heap structure, isn't
> it?
> 
> It would be approximate, yes, but we already have a page ordering as wanted
wrt
> allocation times, but would have to do intra-page sweeps differently (stack up
> the finalize()s first.)

But the freelist is managed on a intra-page basis. So I guess it's never
guaranteed that if A is allocated after B, the page which A belongs to is linked
after the page which B belongs to. I guess the only way to guarantee the
ordering would be to keep track of the ordering of the finalizers, which is
something the pre-finalizers are doing.