Fix null dereference on MemoryCache.

Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=488373
Committed: https://crrev.com/5de01d904d77ba5e5beaf4776e45e6eecd47a87b
Cr-Commit-Position: refs/heads/master@{#367779}

Committed: https://crrev.com/843010ff7eeda6e9e87347aaaa6365efecd1161d
Cr-Commit-Position: refs/heads/master@{#367802}

[yhirano, 2015-12-21 05:18:26]

Description was changed from

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=483373
==========

to

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=483373
==========

[yhirano, 2015-12-21 05:18:26]

yhirano@chromium.org changed reviewers:
+ japhet@chromium.org

[yhirano, 2015-12-21 05:23:28]

Description was changed from

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=483373
==========

to

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=488373
==========

[Nate Chapin, 2015-12-21 22:03:56]

On 2015/12/21 07:16:35, yhirano wrote:

Is this dependent on oilpan GC timing then?

[yhirano, 2015-12-21 23:57:58]

On 2015/12/21 22:03:56, Nate Chapin wrote:
> On 2015/12/21 07:16:35, yhirano wrote:
> 
> Is this dependent on oilpan GC timing then?

If Resource were GarbageCollected, we could delete MemoryCacheEntry::dispose and
this change wouldn't be needed, I think.

[Nate Chapin, 2015-12-22 00:06:12]

On 2015/12/21 23:57:58, yhirano wrote:
> On 2015/12/21 22:03:56, Nate Chapin wrote:
> > On 2015/12/21 07:16:35, yhirano wrote:
> > 
> > Is this dependent on oilpan GC timing then?
> 
> If Resource were GarbageCollected, we could delete MemoryCacheEntry::dispose
and
> this change wouldn't be needed, I think.

I'm trying to figure out how a MemoryCacheEntry can be alive, but have a null
m_resource. It looks like m_resource is only nulled in dispose(). So dispose()
is getting called but MemoryCacheEntry hasn't been GCed yet and there are still
live pointers to it? Is that what is happening?

[yhirano, 2015-12-22 00:19:39]

On 2015/12/22 00:06:12, Nate Chapin wrote:
> On 2015/12/21 23:57:58, yhirano wrote:
> > On 2015/12/21 22:03:56, Nate Chapin wrote:
> > > On 2015/12/21 07:16:35, yhirano wrote:
> > > 
> > > Is this dependent on oilpan GC timing then?
> > 
> > If Resource were GarbageCollected, we could delete MemoryCacheEntry::dispose
> and
> > this change wouldn't be needed, I think.
> 
> I'm trying to figure out how a MemoryCacheEntry can be alive, but have a null
> m_resource. It looks like m_resource is only nulled in dispose(). So dispose()
> is getting called but MemoryCacheEntry hasn't been GCed yet and there are
still
> live pointers to it? Is that what is happening?

I think so, though I haven't found the exact path.
For example, Resource::prune may call 
CSSStyleSheetResource::destroyDecodedDataIfPossible and it clears a refptr
m_parsedStyleSheetCache. That may cause an eviction of another Resource.

[Nate Chapin, 2015-12-22 00:33:06]

Ok. It's unfortunate that this requires breaking out of the loop, but it appears
that is not a new behavior, just a different way to reach an existing behavior.

Is it possible to manufacture a test case for this? We don't have a
CSSStyleSheetResourceTest.cpp yet, but this seems like a good reason to add it
if we can get the test case to work.

[yhirano, 2015-12-24 06:05:39]

Patchset #3 (id:40001) has been deleted

[yhirano, 2015-12-24 06:07:13]

Patchset #2 (id:20001) has been deleted

[yhirano, 2015-12-24 06:07:15]

Patchset #2 (id:60001) has been deleted

[yhirano, 2015-12-24 06:10:27]

Patchset #2 (id:80001) has been deleted

[yhirano, 2015-12-24 06:12:57]

Patchset #2 (id:100001) has been deleted

[yhirano, 2015-12-24 07:40:53]

On 2015/12/22 00:33:06, Nate Chapin wrote:
> Ok. It's unfortunate that this requires breaking out of the loop, but it
appears
> that is not a new behavior, just a different way to reach an existing
behavior.
> 
> Is it possible to manufacture a test case for this? We don't have a
> CSSStyleSheetResourceTest.cpp yet, but this seems like a good reason to add it
> if we can get the test case to work.

I added a test case which crashes without the fix and doesn't crash with the fix
(PS2 doesn't include the fix whereas PS3 includes the fix).

The test uses CSS classes and hence it breaks the DEPS rule. If the DEPS comment
"core/fetch shouldn't depend on anything else in core/." is correct, I think
individual Resource subclasses (CSSStyleSheetResource, ImageResource and so on)
should be placed on their directories (core/css, for example) rather than on
core/fetch. What do you think about it?

[Nate Chapin, 2015-12-29 23:53:25]

On 2015/12/24 07:40:53, yhirano wrote:
> On 2015/12/22 00:33:06, Nate Chapin wrote:
> > Ok. It's unfortunate that this requires breaking out of the loop, but it
> appears
> > that is not a new behavior, just a different way to reach an existing
> behavior.
> > 
> > Is it possible to manufacture a test case for this? We don't have a
> > CSSStyleSheetResourceTest.cpp yet, but this seems like a good reason to add
it
> > if we can get the test case to work.
> 
> I added a test case which crashes without the fix and doesn't crash with the
fix
> (PS2 doesn't include the fix whereas PS3 includes the fix).
> 
> The test uses CSS classes and hence it breaks the DEPS rule. If the DEPS
comment
> "core/fetch shouldn't depend on anything else in core/." is correct, I think
> individual Resource subclasses (CSSStyleSheetResource, ImageResource and so
on)
> should be placed on their directories (core/css, for example) rather than on
> core/fetch. What do you think about it?

Sorry for the delay!

I wrote a change to do disperse the Resource subclasses to relevant directories
(https://codereview.chromium.org/1167583003/), but I considered it somewhat
controversial. I meant to send out a blink-dev thread about it, but I got
distracted and never came back to it :) I think it's a good idea, but I don't
know how the rest of the project feels about it.

Perhaps in the short term, we can sidestep the DEPS rule issue by putting
CSSStyleSheetResourceTest.cpp in core/css?

[yhirano, 2016-01-05 05:36:20]

Patchset #6 (id:200001) has been deleted

[yhirano, 2016-01-05 05:36:25]

Patchset #5 (id:180001) has been deleted

[yhirano, 2016-01-05 05:36:33]

Patchset #4 (id:160001) has been deleted

[yhirano, 2016-01-05 06:06:41]

Patchset #5 (id:240001) has been deleted

[yhirano, 2016-01-05 06:09:03]

Patchset #5 (id:260001) has been deleted

[yhirano, 2016-01-05 06:09:07]

Patchset #4 (id:220001) has been deleted

[yhirano, 2016-01-05 08:07:56]

On 2015/12/29 23:53:25, Nate Chapin wrote:
> I wrote a change to do disperse the Resource subclasses to relevant
directories
> (https://codereview.chromium.org/1167583003/), but I considered it somewhat
> controversial. I meant to send out a blink-dev thread about it, but I got
> distracted and never came back to it :) I think it's a good idea, but I don't
> know how the rest of the project feels about it.
> 
The change looks good. I think dispersing the Resource subclasses is a logical
consequence of the DEPS rule.
The current situation - core/fetch is not able to depend on other core/
directories and Resources subclasses are placed on it - looks wrong to me.

> Perhaps in the short term, we can sidestep the DEPS rule issue by putting
> CSSStyleSheetResourceTest.cpp in core/css?

Thanks, done.

[Nate Chapin, 2016-01-05 22:07:58]

lgtm

[yhirano, 2016-01-06 02:16:33]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-01-06 02:17:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1537343002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1537343002/280001

[commit-bot: I haz the power, 2016-01-06 05:23:03]

Description was changed from

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=488373
==========

to

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=488373
==========

[commit-bot: I haz the power, 2016-01-06 05:23:04]

Committed patchset #4 (id:280001)

[commit-bot: I haz the power, 2016-01-06 05:24:01]

Description was changed from

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=488373
==========

to

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=488373

Committed: https://crrev.com/5de01d904d77ba5e5beaf4776e45e6eecd47a87b
Cr-Commit-Position: refs/heads/master@{#367779}
==========

[commit-bot: I haz the power, 2016-01-06 05:24:01]

Patchset 4 (id:??) landed as
https://crrev.com/5de01d904d77ba5e5beaf4776e45e6eecd47a87b
Cr-Commit-Position: refs/heads/master@{#367779}

[Noel Gordon, 2016-01-06 06:46:58]

On 2016/01/06 05:24:01, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/5de01d904d77ba5e5beaf4776e45e6eecd47a87b
> Cr-Commit-Position: refs/heads/master@{#367779}

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Oilpan/b...

Failing in Oilpan compile.  Can we fix please?

[yhirano, 2016-01-06 06:49:46]

On 2016/01/06 06:46:58, noel gordon wrote:
> On 2016/01/06 05:24:01, commit-bot: I haz the power wrote:
> > Patchset 4 (id:??) landed as
> > https://crrev.com/5de01d904d77ba5e5beaf4776e45e6eecd47a87b
> > Cr-Commit-Position: refs/heads/master@{#367779}
> 
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Oilpan/b...
> 
> Failing in Oilpan compile.  Can we fix please?

Oh, sorry. Sure, I will fix the error.

[yhirano, 2016-01-06 06:51:11]

A revert of this CL (patchset #4 id:280001) has been created in
https://codereview.chromium.org/1566653002/ by yhirano@chromium.org.

The reason for reverting is: The change breaks the oilpan build..

[yhirano, 2016-01-06 06:53:57]

Description was changed from

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=488373

Committed: https://crrev.com/5de01d904d77ba5e5beaf4776e45e6eecd47a87b
Cr-Commit-Position: refs/heads/master@{#367779}
==========

to

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=488373

Committed: https://crrev.com/5de01d904d77ba5e5beaf4776e45e6eecd47a87b
Cr-Commit-Position: refs/heads/master@{#367779}
==========

[yhirano, 2016-01-06 07:03:33]

Sorry, I didn't notice sigbjornf@ had already fixed the compile error and I
reverted the original change. I will reland the change with the fix.

[yhirano, 2016-01-06 07:37:39]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-01-06 07:37:39]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1537343002/#ps300001
(title: " ")

[commit-bot: I haz the power, 2016-01-06 07:37:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1537343002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1537343002/300001

[commit-bot: I haz the power, 2016-01-06 09:33:52]

Description was changed from

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=488373

Committed: https://crrev.com/5de01d904d77ba5e5beaf4776e45e6eecd47a87b
Cr-Commit-Position: refs/heads/master@{#367779}
==========

to

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=488373

Committed: https://crrev.com/5de01d904d77ba5e5beaf4776e45e6eecd47a87b
Cr-Commit-Position: refs/heads/master@{#367779}
==========

[commit-bot: I haz the power, 2016-01-06 09:33:53]

Committed patchset #5 (id:300001)

[commit-bot: I haz the power, 2016-01-06 09:35:24]

Description was changed from

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=488373

Committed: https://crrev.com/5de01d904d77ba5e5beaf4776e45e6eecd47a87b
Cr-Commit-Position: refs/heads/master@{#367779}
==========

to

==========
Fix null dereference on MemoryCache.

Usually a valid MemoryCacheEntry holds a non-null Resource as |m_resource|. But
when we hold a valid MemoryCacheEntry beyond destructive statements, it may be
evicted from the cache and get stale. That means |m_resource| can be null in
such cases. This CL checks it in order to avoid null dereference.

BUG=488373

Committed: https://crrev.com/5de01d904d77ba5e5beaf4776e45e6eecd47a87b
Cr-Commit-Position: refs/heads/master@{#367779}

Committed: https://crrev.com/843010ff7eeda6e9e87347aaaa6365efecd1161d
Cr-Commit-Position: refs/heads/master@{#367802}
==========

[commit-bot: I haz the power, 2016-01-06 09:35:25]

Patchset 5 (id:??) landed as
https://crrev.com/843010ff7eeda6e9e87347aaaa6365efecd1161d
Cr-Commit-Position: refs/heads/master@{#367802}