Make CSSPreloaderResourceClient hold weak references to its resource

Make CSSPreloaderResourceClient hold weak references to its resource

We don't want the client to hold on to the resources for longer than
necessary. This patch enforces this invariant by making the object
to longer a ResourceOwner, and instead hold a WeakMember to the style
resource.

BUG=596676
Committed: https://crrev.com/3224ae9404f9395a7f293b57da6cebe2f2371be5
Cr-Commit-Position: refs/heads/master@{#432864}

[Charlie Harrison, 2016-10-27 16:09:13]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 16:09:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 17:09:20]

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

[commit-bot: I haz the power, 2016-10-27 17:09:21]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-10-27 17:38:04]

csharrison@chromium.org changed reviewers:
+ yoav@yoav.ws

[Charlie Harrison, 2016-10-27 17:38:05]

Yoav: the CSS experiment is showing an increase in counts for
PreloadScanner.Counts2.Miss.CSSStyleSheet histogram, even for the experimental
branch that just scans and does not preload.

Could holding a strong reference to the Resource cause this sort of problem? I'm
afraid we are corrupting this metric with the experiment by attaching this
resource client.

[Yoav Weiss, 2016-10-27 21:12:04]

On 2016/10/27 17:38:05, Charlie Harrison wrote:
> Yoav: the CSS experiment is showing an increase in counts for
> PreloadScanner.Counts2.Miss.CSSStyleSheet histogram, even for the experimental
> branch that just scans and does not preload.
> 
> Could holding a strong reference to the Resource cause this sort of problem?
I'm
> afraid we are corrupting this metric with the experiment by attaching this
> resource client.

To me it makes more sense that one of our recent(ish) clearPreloads() changes
messed up this metric.
I don't see how the strong/weak reference in CSSPreloadScanner makes any impact
on the resources that are stores in m_preloads (which in itself is a strong ref)

At the same time, calling clearPreloads in both DCL and when the document is
detached[1], could very well have caused this... (my bad :/)

Seems to me like we should call logPreloadStats() only from DCL and include an
isLinkPreload() bailout to prevent these stats from being skewed by explicit
preloads.


[1] https://codereview.chromium.org/2174563003/

[Charlie Harrison, 2016-10-27 21:14:59]

You may be right, but we're only seeing the weirdness for experiment groups
where this resource client gets attached.

Somehow, just attaching the RC is affecting the metric.

[Yoav Weiss, 2016-10-27 21:42:44]

On 2016/10/27 21:14:59, Charlie Harrison wrote:
> You may be right, but we're only seeing the weirdness for experiment groups
> where this resource client gets attached.
> 
> Somehow, just attaching the RC is affecting the metric.

Hmm, that is weird indeed.

I'm not comfortable with us making this issue go away without fully
understanding it.
At the same time, landing this could prove or disprove the relationship between
the strong ref and the metric skew.
How long would it take to see the metrics change (or not change) after this
lands?

[Charlie Harrison, 2016-10-27 21:50:09]

I agree with you wrt understanding the problem. It is quite strange and I've
been very confused how it can happen :/

One day of Canary data should show us if the problem is fixed. It usually takes
a few days to process the data.

[Yoav Weiss, 2016-10-27 21:59:49]

yoav@yoav.ws changed reviewers:
+ esprehn@chromium.org

[Yoav Weiss, 2016-10-27 21:59:49]

Overall the patch looks good % missing DCHECK

I'd like us to have a plan though. If this makes the problem go away, what's our
next debugging move?

Also adding esprehn for advice

https://codereview.chromium.org/2452413002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (right):

https://codereview.chromium.org/2452413002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:259:
m_resource(toCSSStyleSheetResource(resource)) {
Can you bring back the DCHECK?

[Charlie Harrison, 2016-10-27 22:34:01]

Thanks. I'm not sure the steps that come after this (supposed) fix except poring
over the code :P

I think this can happen if we
1. Have a link preload
2. Finish parsing -> log link preload as miss (existing bug) but don't remove
from m_preloads
3. Preload lives longer than it would as a link preload.
4. ResourceFetcher teardown (clearPreloads double duty).

I can try to reproduce this...

https://codereview.chromium.org/2452413002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (right):

https://codereview.chromium.org/2452413002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:259:
m_resource(toCSSStyleSheetResource(resource)) {
On 2016/10/27 21:59:49, Yoav Weiss wrote:
> Can you bring back the DCHECK? 

AFAICT the to<Resource> type casts do this already (actually, they
ASSERT_WITH_SECURITY_IMPLICATION).

[Charlie Harrison, 2016-10-28 00:08:20]

Histograms are showing ~7x increase in the miss metric, so that hypothesis would
only explain a 2x increase :/

[Yoav Weiss, 2016-10-28 06:18:22]

On 2016/10/27 22:34:01, Charlie Harrison wrote:
> Thanks. I'm not sure the steps that come after this (supposed) fix except
poring
> over the code :P
> 
> I think this can happen if we
> 1. Have a link preload
> 2. Finish parsing -> log link preload as miss (existing bug) but don't remove
> from m_preloads
> 3. Preload lives longer than it would as a link preload.
> 4. ResourceFetcher teardown (clearPreloads double duty).
> 
> I can try to reproduce this...

I still don't get why the weak ref would matter as m_preloads is already a
strong ref (and removal from it is not dependent on other refs existing)

> 
>
https://codereview.chromium.org/2452413002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (right):
> 
>
https://codereview.chromium.org/2452413002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:259:
> m_resource(toCSSStyleSheetResource(resource)) {
> On 2016/10/27 21:59:49, Yoav Weiss wrote:
> > Can you bring back the DCHECK? 
> 
> AFAICT the to<Resource> type casts do this already (actually, they
> ASSERT_WITH_SECURITY_IMPLICATION).

Fair enough

[Yoav Weiss, 2016-10-28 06:27:24]

On 2016/10/28 00:08:20, Charlie Harrison wrote:
> Histograms are showing ~7x increase in the miss metric, so that hypothesis
would
> only explain a 2x increase :/

I have zero theories (outside of mem corruption) that can explain a 7x
increase...
clearPreloads is certainly not called that frequently...

What do we see reporting for PreloadScanner.Counts2.CSSStyleSheet? Is there a
similar increase there?

[Charlie Harrison, 2016-10-28 12:14:45]

Ah, I was under the false impression that the "link preload timer" logic also
removed the link preload from m_preloads after the 3 second mark (it just sends
a console warning). If that were the case then the strong ref here could cause
it to live longer, and link preloads *do* possibly get logged twice as misses if
they aren't used at all by the time clearPreloads() is called again in the
pre-finalizer or clearContext().

The non miss bucket seems normal / within noise. The Preloading group has a ~4%
increase and the non preloading group has a .6% increase. It's possible

[esprehn, 2016-11-04 23:46:10]

What's the status of this?

[Charlie Harrison, 2016-11-05 00:37:16]

On 2016/11/04 23:46:10, esprehn wrote:
> What's the status of this?

I'd like to land this as-is to help investigate the metrics issue. Ideally this
class *shouldn't* hold a strong reference to a resource or make it stay alive
any longer than it ought to, so the change is worthwhile in its own right (imo).

[Charlie Harrison, 2016-11-05 00:39:24]

On 2016/11/05 00:37:16, Charlie Harrison wrote:
> On 2016/11/04 23:46:10, esprehn wrote:
> > What's the status of this?
> 
> I'd like to land this as-is to help investigate the metrics issue. Ideally
this
> class *shouldn't* hold a strong reference to a resource or make it stay alive
> any longer than it ought to, so the change is worthwhile in its own right
(imo).

Although, I think Yoav and I are stumped on the root cause here, so if you have
any ideas on that it would be helpful :/

[Yoav Weiss, 2016-11-05 09:34:11]

On 2016/11/05 00:39:24, Charlie Harrison wrote:
> On 2016/11/05 00:37:16, Charlie Harrison wrote:
> > On 2016/11/04 23:46:10, esprehn wrote:
> > > What's the status of this?
> > 
> > I'd like to land this as-is to help investigate the metrics issue. Ideally
> this
> > class *shouldn't* hold a strong reference to a resource or make it stay
alive
> > any longer than it ought to, so the change is worthwhile in its own right
> (imo).
> 
> Although, I think Yoav and I are stumped on the root cause here, so if you
have
> any ideas on that it would be helpful :/

FWIW, I agree that this change is worthwhile and LGTM. I just don't want us to
land this, hide the real problem and then go *shrug*
So, I'd be happy with landing this as long as we have a plan to tackle that
scenario.

[Charlie Harrison, 2016-11-07 18:15:33]

Thanks. I'm going to throw up another CL to see if we can fix the logging code.

[Yoav Weiss, 2016-11-14 20:10:48]

On 2016/11/07 18:15:33, Charlie Harrison wrote:
> Thanks. I'm going to throw up another CL to see if we can fix the logging
code.

Just saw your comment on 660168. Fwiw, I support landing this to fix the memory
regressions. (But would be nice to see impact separate from
https://codereview.chromium.org/2484823002/ that just landed)

[Charlie Harrison, 2016-11-14 20:11:49]

On 2016/11/14 20:10:48, Yoav Weiss wrote:
> On 2016/11/07 18:15:33, Charlie Harrison wrote:
> > Thanks. I'm going to throw up another CL to see if we can fix the logging
> code.
> 
> Just saw your comment on 660168. Fwiw, I support landing this to fix the
memory
> regressions. (But would be nice to see impact separate from
> https://codereview.chromium.org/2484823002/ that just landed)

Yep, planning on landing this in a few days once we get data back from the
related CL.

[Charlie Harrison, 2016-11-17 12:59:48]

New data is not showing improvements, and since this may be causing a memory
regression (issue 660168), I'm going to land this now.

[Charlie Harrison, 2016-11-17 13:01:28]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-11-17 13:01:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 14:24:51]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-17 14:28:33]

Description was changed from

==========
Make CSSPreloaderResourceClient hold weak references to its resource

We don't want the client to hold on to the resources for longer than
necessary. This patch enforces this invariant by making the object
to longer a ResourceOwner, and instead hold a WeakMember to the style
resource.

BUG=596676
==========

to

==========
Make CSSPreloaderResourceClient hold weak references to its resource

We don't want the client to hold on to the resources for longer than
necessary. This patch enforces this invariant by making the object
to longer a ResourceOwner, and instead hold a WeakMember to the style
resource.

BUG=596676

Committed: https://crrev.com/3224ae9404f9395a7f293b57da6cebe2f2371be5
Cr-Commit-Position: refs/heads/master@{#432864}
==========

[commit-bot: I haz the power, 2016-11-17 14:28:35]

Patchset 1 (id:??) landed as
https://crrev.com/3224ae9404f9395a7f293b57da6cebe2f2371be5
Cr-Commit-Position: refs/heads/master@{#432864}