Fix memory corruption related to load blocking resource move

Fix memory corruption related to load blocking resource move

This fixes an issue where a resource loader belonging to one
ResourceFetcher was accidentally added as a blocking loader to another
ResourceFetcher, by checking the loader is part of the already
non-blocking loaders belonging to current ResourceFetcher.

This also adds DCHECKs on a couple of methods removing loaders from
hashmaps, to make sure we're not trying to remove a nullptr.

BUG=666563
Committed: https://crrev.com/7497990e6eb19dc8dd61de4f188553c9c054cef9
Cr-Commit-Position: refs/heads/master@{#435573}

[Yoav Weiss, 2016-11-30 14:01:04]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 14:01:31]

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

[Yoav Weiss, 2016-11-30 14:03:35]

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

[Yoav Weiss, 2016-11-30 14:03:36]

Hey Charlie,

This fixes an issue David found related to insufficient checks when moving
resourceLoaders from the nonBlocking map to the blocking one.

Can you take a look?

[Yoav Weiss, 2016-11-30 14:05:21]

On 2016/11/30 14:03:36, Yoav Weiss wrote:
> Hey Charlie,
> 
> This fixes an issue David found related to insufficient checks when moving
> resourceLoaders from the nonBlocking map to the blocking one.
> 
> Can you take a look?

I still need to add a test. I'll probably add a unit test to ResourceFetcherTest

[bokan, 2016-11-30 14:29:03]

This fixes the immediate issue, but is there an underlying bug? How do we get to
a place where we move a Resource to blocking on a ResourceFetcher that it
doesn't belong to?

[Yoav Weiss, 2016-11-30 14:49:23]

On 2016/11/30 14:29:03, bokan wrote:
> This fixes the immediate issue, but is there an underlying bug? How do we get
to
> a place where we move a Resource to blocking on a ResourceFetcher that it
> doesn't belong to?

MemoryCache() is serving those resources regardless of the ResourceFetcher that
initially created them. I'm not sure that's an issue.

[bokan, 2016-11-30 14:52:42]

Got it, thanks. Patch looks fine to me, just needs a test.

https://codereview.chromium.org/2537303003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2537303003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1286:
m_loaders.remove(loader);
Should we check that loader is in m_loaders here too? Maybe at least a DCHECK?

[Charlie Harrison, 2016-11-30 14:54:41]

Yeah, this problem has come up before in subtle ways. We don't want a Resource
to "belong" to any one ResourceFetcher, as it can be shared between multiple of
them (by design).

We should probably invest a bit of time in an audit to make sure we aren't
making other assumptions like this.

[commit-bot: I haz the power, 2016-11-30 15:13:52]

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

[commit-bot: I haz the power, 2016-11-30 15:13:53]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-11-30 16:05:05]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 16:05:37]

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

[Yoav Weiss, 2016-11-30 16:11:42]

Added a test

https://codereview.chromium.org/2537303003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2537303003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1286:
m_loaders.remove(loader);
On 2016/11/30 14:52:42, bokan wrote:
> Should we check that loader is in m_loaders here too? Maybe at least a DCHECK?

That makes sense. Not sure how to exercise that code with an indirect test...

[Charlie Harrison, 2016-11-30 16:51:33]

https://codereview.chromium.org/2537303003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2537303003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1286:
m_loaders.remove(loader);
On 2016/11/30 16:11:42, Yoav Weiss wrote:
> On 2016/11/30 14:52:42, bokan wrote:
> > Should we check that loader is in m_loaders here too? Maybe at least a
DCHECK?
> 
> That makes sense. Not sure how to exercise that code with an indirect test...

I think we can DCHECK, afaict.

[bokan, 2016-11-30 17:01:22]

https://codereview.chromium.org/2537303003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2537303003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1286:
m_loaders.remove(loader);
On 2016/11/30 16:51:33, Charlie Harrison wrote:
> On 2016/11/30 16:11:42, Yoav Weiss wrote:
> > On 2016/11/30 14:52:42, bokan wrote:
> > > Should we check that loader is in m_loaders here too? Maybe at least a
> DCHECK?
> > 
> > That makes sense. Not sure how to exercise that code with an indirect
test...
> 
> I think we can DCHECK, afaict.

I would DCHECK and add an early return since otherwise we end up hanging the
renderer in shipped builds that don't DCHECK. Here and the other DCHECKs.

[Yoav Weiss, 2016-11-30 17:01:35]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 17:02:19]

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

[Yoav Weiss, 2016-11-30 17:06:28]

On 2016/11/30 17:01:22, bokan wrote:
>
https://codereview.chromium.org/2537303003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/2537303003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1286:
> m_loaders.remove(loader);
> On 2016/11/30 16:51:33, Charlie Harrison wrote:
> > On 2016/11/30 16:11:42, Yoav Weiss wrote:
> > > On 2016/11/30 14:52:42, bokan wrote:
> > > > Should we check that loader is in m_loaders here too? Maybe at least a
> > DCHECK?
> > > 
> > > That makes sense. Not sure how to exercise that code with an indirect
> test...
> > 
> > I think we can DCHECK, afaict.
> 
> I would DCHECK and add an early return since otherwise we end up hanging the
> renderer in shipped builds that don't DCHECK. Here and the other DCHECKs.

Added a DCHECK. Do we do DCHECK + early return elsewhere?
I've never seen this pattern (usually DCHECK are where we're confident enough
something should never happen under normal circumstances, and early returns are
elsewhere)

[Charlie Harrison, 2016-11-30 17:09:16]

Handling DCHECK failures is against the style guide. We should add a temporary
CHECK if we think this is possible. Otherwise, DCHECK should suffice.

[bokan, 2016-11-30 17:11:47]

On 2016/11/30 17:09:16, Charlie Harrison wrote:
> Handling DCHECK failures is against the style guide. We should add a temporary
> CHECK if we think this is possible. Otherwise, DCHECK should suffice.

I would argue for CHECK in that case. It'll give us crash reports we can use to
debug. The current experience just leaves the renderer hanging until the "wait
for renderer" dialog which is a poor user experience.

[Charlie Harrison, 2016-11-30 17:15:23]

On 2016/11/30 17:11:47, bokan wrote:
> On 2016/11/30 17:09:16, Charlie Harrison wrote:
> > Handling DCHECK failures is against the style guide. We should add a
temporary
> > CHECK if we think this is possible. Otherwise, DCHECK should suffice.
> 
> I would argue for CHECK in that case. It'll give us crash reports we can use
to
> debug. The current experience just leaves the renderer hanging until the "wait
> for renderer" dialog which is a poor user experience.

That sounds ok to me.

[commit-bot: I haz the power, 2016-11-30 18:16:53]

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

[commit-bot: I haz the power, 2016-11-30 18:16:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yoav Weiss, 2016-11-30 19:42:21]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 19:43:11]

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

[Yoav Weiss, 2016-11-30 19:48:45]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 19:49:18]

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-11-30 21:10:11]

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

[commit-bot: I haz the power, 2016-11-30 21:10:12]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-11-30 22:51:23]

lgtm

[Charlie Harrison, 2016-12-01 01:30:04]

lgtm too

[Yoav Weiss, 2016-12-01 07:08:18]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2016-12-01 07:08:37]

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-12-01 07:13:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480576098527060,
"parent_rev": "b7175462477e83e807115f26b6a015ffb909b4f4", "commit_rev":
"3a4bd8013238cd4d1ba180d2d727a6cc821aee2e"}

[commit-bot: I haz the power, 2016-12-01 07:13:54]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-01 07:16:53]

Description was changed from

==========
Fix memory corruption related to load blocking resource move

This fixes an issue where a resource loader belonging to one
ResourceFetcher was accidentally added as a blocking loader to another
ResourceFetcher, by checking the loader is part of the already
non-blocking loaders belonging to current ResourceFetcher.

This also adds DCHECKs on a couple of methods removing loaders from
hashmaps, to make sure we're not trying to remove a nullptr.

BUG=666563
==========

to

==========
Fix memory corruption related to load blocking resource move

This fixes an issue where a resource loader belonging to one
ResourceFetcher was accidentally added as a blocking loader to another
ResourceFetcher, by checking the loader is part of the already
non-blocking loaders belonging to current ResourceFetcher.

This also adds DCHECKs on a couple of methods removing loaders from
hashmaps, to make sure we're not trying to remove a nullptr.

BUG=666563

Committed: https://crrev.com/7497990e6eb19dc8dd61de4f188553c9c054cef9
Cr-Commit-Position: refs/heads/master@{#435573}
==========

[commit-bot: I haz the power, 2016-12-01 07:16:54]

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