Make StorageArea cleanly observe its cached frame

Make StorageArea cleanly observe its cached frame

StorageArea caches the result of its last access check wrt to a
LocalFrame. With Oilpan, we have to weakly hold on to that frame
so as not to have scripts be able to retain an entire frame.

Also, the cached frame might be destroyed before the StorageArea
holding a reference is; the implementation is better off clearing
that reference when this happens (with or without Oilpan.)

Address both by making StorageArea a FrameDestructionObserver that
observes its cached frame. It is a weak reference and it will be
cleared upon frame destruction.

To accommodate StorageArea switching the frame it is observing,
make FrameDestructionObserver::observeFrame() again a protected
method.

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

[sof, 2014-09-23 10:03:33]

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

[sof, 2014-09-23 10:03:33]

Please take a look.

Addresses the Oilpan leaks reported after r182337, afaict.

[haraken, 2014-09-23 10:06:44]

Who clears out the m_canAccessStorageCachedFrame when the frame is gone? Or is
it guaranteed that the StorageArea doesn't outlive the frame?

[zerny-chromium, 2014-09-23 10:26:16]

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

[zerny-chromium, 2014-09-23 10:26:17]

lgtm

[sof, 2014-09-23 10:41:24]

On 2014/09/23 10:06:44, haraken wrote:
> Who clears out the m_canAccessStorageCachedFrame when the frame is gone? Or is
> it guaranteed that the StorageArea doesn't outlive the frame?

No one does non-Oilpan, and the StorageArea might outlive the frame via a script
reference.

Given how it is used (raw pointer equality check) that's acceptable though as
any next storage check will cause a cache miss & it'll be replaced.

[Mads Ager (chromium), 2014-09-23 10:48:34]

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

[Mads Ager (chromium), 2014-09-23 10:48:34]

lgtm2

[haraken, 2014-09-23 10:54:45]

On 2014/09/23 10:41:24, sof wrote:
> On 2014/09/23 10:06:44, haraken wrote:
> > Who clears out the m_canAccessStorageCachedFrame when the frame is gone? Or
is
> > it guaranteed that the StorageArea doesn't outlive the frame?
> 
> No one does non-Oilpan, and the StorageArea might outlive the frame via a
script
> reference.
> 
> Given how it is used (raw pointer equality check) that's acceptable though as
> any next storage check will cause a cache miss & it'll be replaced.

Hmm, probably would it be better to:

- Make the StorageArea a FrameDestructionObserver.
- Change 'WeakMember<LocalFrame> m_canAccessStorageCachedFrame' to 'bool
m_canAccessStorageCachedFrame'.
- Clear the m_canAccessStorageCachedFrame flag when the current frame is gone.

I agree that this is not related to your CL, but it doesn't look nice to allow
the StorageArea to hold a stale pointer to a dead frame (even if it's used only
for the pointer equality check).

[sof, 2014-09-23 10:59:51]

On 2014/09/23 10:54:45, haraken wrote:
> On 2014/09/23 10:41:24, sof wrote:
> > On 2014/09/23 10:06:44, haraken wrote:
> > > Who clears out the m_canAccessStorageCachedFrame when the frame is gone?
Or
> is
> > > it guaranteed that the StorageArea doesn't outlive the frame?
> > 
> > No one does non-Oilpan, and the StorageArea might outlive the frame via a
> script
> > reference.
> > 
> > Given how it is used (raw pointer equality check) that's acceptable though
as
> > any next storage check will cause a cache miss & it'll be replaced.
> 
> Hmm, probably would it be better to:
> 
> - Make the StorageArea a FrameDestructionObserver.
> - Change 'WeakMember<LocalFrame> m_canAccessStorageCachedFrame' to 'bool
> m_canAccessStorageCachedFrame'.
> - Clear the m_canAccessStorageCachedFrame flag when the current frame is gone.
> 
> I agree that this is not related to your CL, but it doesn't look nice to allow
> the StorageArea to hold a stale pointer to a dead frame (even if it's used
only
> for the pointer equality check).

Great suggestion, let's improve it along those lines.

[sof, 2014-09-23 12:34:36]

On 2014/09/23 10:59:51, sof wrote:
> On 2014/09/23 10:54:45, haraken wrote:
> > On 2014/09/23 10:41:24, sof wrote:
> > > On 2014/09/23 10:06:44, haraken wrote:
> > > > Who clears out the m_canAccessStorageCachedFrame when the frame is gone?
> Or
> > is
> > > > it guaranteed that the StorageArea doesn't outlive the frame?
> > > 
> > > No one does non-Oilpan, and the StorageArea might outlive the frame via a
> > script
> > > reference.
> > > 
> > > Given how it is used (raw pointer equality check) that's acceptable though
> as
> > > any next storage check will cause a cache miss & it'll be replaced.
> > 
> > Hmm, probably would it be better to:
> > 
> > - Make the StorageArea a FrameDestructionObserver.
> > - Change 'WeakMember<LocalFrame> m_canAccessStorageCachedFrame' to 'bool
> > m_canAccessStorageCachedFrame'.
> > - Clear the m_canAccessStorageCachedFrame flag when the current frame is
gone.
> > 
> > I agree that this is not related to your CL, but it doesn't look nice to
allow
> > the StorageArea to hold a stale pointer to a dead frame (even if it's used
> only
> > for the pointer equality check).
> 
> Great suggestion, let's improve it along those lines.

Please take a look, patch + description updated.

An uncommon use of FrameDestructionObserver, but it fits the behavior wanted
here well.

[haraken, 2014-09-23 12:38:24]

On 2014/09/23 12:34:36, sof wrote:
> On 2014/09/23 10:59:51, sof wrote:
> > On 2014/09/23 10:54:45, haraken wrote:
> > > On 2014/09/23 10:41:24, sof wrote:
> > > > On 2014/09/23 10:06:44, haraken wrote:
> > > > > Who clears out the m_canAccessStorageCachedFrame when the frame is
gone?
> > Or
> > > is
> > > > > it guaranteed that the StorageArea doesn't outlive the frame?
> > > > 
> > > > No one does non-Oilpan, and the StorageArea might outlive the frame via
a
> > > script
> > > > reference.
> > > > 
> > > > Given how it is used (raw pointer equality check) that's acceptable
though
> > as
> > > > any next storage check will cause a cache miss & it'll be replaced.
> > > 
> > > Hmm, probably would it be better to:
> > > 
> > > - Make the StorageArea a FrameDestructionObserver.
> > > - Change 'WeakMember<LocalFrame> m_canAccessStorageCachedFrame' to 'bool
> > > m_canAccessStorageCachedFrame'.
> > > - Clear the m_canAccessStorageCachedFrame flag when the current frame is
> gone.
> > > 
> > > I agree that this is not related to your CL, but it doesn't look nice to
> allow
> > > the StorageArea to hold a stale pointer to a dead frame (even if it's used
> > only
> > > for the pointer equality check).
> > 
> > Great suggestion, let's improve it along those lines.
> 
> Please take a look, patch + description updated.
> 
> An uncommon use of FrameDestructionObserver, but it fits the behavior wanted
> here well.

Looks much nicer! LGTM.

[Mads Ager (chromium), 2014-09-23 12:39:26]

LGTM

Nice catch Haraken. It is good to treat this the same in both builds. I guess
there was a small chance here that the Frame could die, a new Frame get created
at the same address and the old cached value used for the new Frame. Don't know
the code well enough to know if that can actually happen, but it is good to
guard against it and with this code there is no chance of that.

[zerny-chromium, 2014-09-23 12:44:54]

lgtm2

[sof, 2014-09-23 12:52:48]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-23 12:53:14]

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

[commit-bot: I haz the power, 2014-09-23 14:52:14]

Committed patchset #3 (id:40001) as 182501