Make the WebURLLoaderClients in the pepper code destroy (or have some guard) the loader in didFail()

Make the WebURLLoaderClients in the pepper code destroy (or have some guard) the loader in didFail()

Currently Blink's ThreadableLoader calls didFinishLoading()
after didFail() when both of the following conditions are met:
- ThreadableLoader is not canceled (or destroyed) in didFail()
- The response to cross-origin request (simple CORS request
  or preflight request) fails CORS check

We plan to change ThreadableLoader not to call
didFinishLoading() after didFail() call. But ManifestDownloader
and PexeDownloader assumes the old behavior.

This CL copies the things done in didFinishLoading() to
didFail() of these classes and thus make a didFinishLoading()
call after didFail() unnecessary to prepare for the change.

----

I wondered if PepperURLLoaderHost is expecting both
didFinishLoading() and didFail() to be called. But it's only
exposed via PepperPluginInstanceImpl::document_loader(), and
to which only PluginDocumentParser::finish() delivers a
notification. PluginDocumentParser never call both of
didFinishLoading() and didFailLoading() on a PluginView.
So, we don't have to worry about breakage, I think.

----

R=hiroshige,bbudge
BUG=515850
Committed: https://crrev.com/431e4370269c39ab808997c0c204e220cd8cef2a
Cr-Commit-Position: refs/heads/master@{#342061}

[tyoshino (SeeGerritForStatus), 2015-08-03 05:32:05]

tyoshino@chromium.org changed reviewers:
+ hiroshige@chromium.org

[tyoshino (SeeGerritForStatus), 2015-08-03 05:32:18]

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

[commit-bot: I haz the power, 2015-08-03 05:33:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267713003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267713003/20001

[commit-bot: I haz the power, 2015-08-03 07:00:43]

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

[commit-bot: I haz the power, 2015-08-03 07:00:44]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2015-08-03 07:25:17]

> We plan to change Blink's ThreadableLoader not to call
> didFinishLoading() after didFail() call.

How about the following?
Currently Blink's ThreadableLoader calls didFinishLoading() after didFail() when
ThreadableLoader is not canceled in didFail() (FIXME: on what conditions?), but
we plan to change  ThreadableLoader not to call didFinishLoading() after
didFail() call.
ManifestDownloader and PexeDownloader assumes the old behavior.
This CL copies the things done in didFinishLoading() to didFail() of these
classes and thus make a didFinishLoading() call after didFail() unnecessary.

https://codereview.chromium.org/1267713003/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/1267713003/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:402: if (error_.get()) {
Changing this |if| into |else if| might be better to ensure at most only one of
didFinishLoading() or didFail() is called here.

https://codereview.chromium.org/1267713003/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:430:
DCHECK(!error_.get());
Should we also add if(finished_loading_) return; here in didFail()?

[hiroshige, 2015-08-03 07:27:42]

https://codereview.chromium.org/1267713003/diff/20001/components/nacl/rendere...
File components/nacl/renderer/manifest_downloader.cc (right):

https://codereview.chromium.org/1267713003/diff/20001/components/nacl/rendere...
components/nacl/renderer/manifest_downloader.cc:73: done();
Since didFinishLoading() is still called before the ThreadableLoader fix is
landed, does simply calling done() cause use-after-free?
(done() called from didFail() deletes |this|)

[tyoshino (SeeGerritForStatus), 2015-08-03 07:44:28]

On 2015/08/03 07:25:17, hiroshige wrote:
> > We plan to change Blink's ThreadableLoader not to call
> > didFinishLoading() after didFail() call.
> 
> How about the following?
> Currently Blink's ThreadableLoader calls didFinishLoading() after didFail()
when
> ThreadableLoader is not canceled in didFail() (FIXME: on what conditions?),
but
> we plan to change  ThreadableLoader not to call didFinishLoading() after
> didFail() call.
> ManifestDownloader and PexeDownloader assumes the old behavior.
> This CL copies the things done in didFinishLoading() to didFail() of these
> classes and thus make a didFinishLoading() call after didFail() unnecessary.

Thanks. Rephrased.

[tyoshino (SeeGerritForStatus), 2015-08-03 07:59:05]

https://codereview.chromium.org/1267713003/diff/20001/components/nacl/rendere...
File components/nacl/renderer/manifest_downloader.cc (right):

https://codereview.chromium.org/1267713003/diff/20001/components/nacl/rendere...
components/nacl/renderer/manifest_downloader.cc:73: done();
On 2015/08/03 07:27:42, hiroshige wrote:
> Since didFinishLoading() is still called before the ThreadableLoader fix is
> landed, does simply calling done() cause use-after-free?
> (done() called from didFail() deletes |this|)

I thought it's fine as deleting this also deletes url_loader_ to kill the loader
so that the loader gets removed from the resource.

- RawResource::didAddClient() doesn't continue to call Resource::didAddClient()
(which contains notifyFinished() call) when hasClient() returns false.
- Resource::checkNotify() uses ResourceClientWalker not to call clients that
have been already removed.

https://codereview.chromium.org/1267713003/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/1267713003/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:402: if (error_.get()) {
On 2015/08/03 07:25:17, hiroshige wrote:
> Changing this |if| into |else if| might be better to ensure at most only one
of
> didFinishLoading() or didFail() is called here.

Done.

https://codereview.chromium.org/1267713003/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:430:
DCHECK(!error_.get());
On 2015/08/03 07:25:17, hiroshige wrote:
> Should we also add if(finished_loading_) return; here in didFail()?

Done.

[hiroshige, 2015-08-03 08:21:16]

lgtm.

[tyoshino (SeeGerritForStatus), 2015-08-03 12:45:19]

Added analysis about the change on pepper_plugin_instance_impl.cc in the CL
description.

[tyoshino (SeeGerritForStatus), 2015-08-03 12:45:29]

tyoshino@chromium.org changed reviewers:
+ bbudge@chromium.org

[tyoshino (SeeGerritForStatus), 2015-08-03 12:45:39]

bbudge, please take a look.

[bbudge, 2015-08-03 12:59:42]

https://codereview.chromium.org/1267713003/diff/40001/components/nacl/rendere...
File components/nacl/renderer/manifest_downloader.cc (right):

https://codereview.chromium.org/1267713003/diff/40001/components/nacl/rendere...
components/nacl/renderer/manifest_downloader.cc:57: void
ManifestDownloader::done() {
nit: Rename this to Close or something similar. Lower case names are a Blink
convention. Lower case names in this file implement a Blink interface.

https://codereview.chromium.org/1267713003/diff/40001/components/nacl/rendere...
components/nacl/renderer/manifest_downloader.cc:66: delete this;
If you haven't changed the Blink ThreadableLoader yet, doesn't this now do a
double delete?

https://codereview.chromium.org/1267713003/diff/40001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/1267713003/diff/40001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1628: delete this;
If ThreadableLoader still calls both didFail and didFinishLoading, doesn't this
do a double-delete?

[tyoshino (SeeGerritForStatus), 2015-08-04 04:51:14]

Thank you for review!

https://codereview.chromium.org/1267713003/diff/40001/components/nacl/rendere...
File components/nacl/renderer/manifest_downloader.cc (right):

https://codereview.chromium.org/1267713003/diff/40001/components/nacl/rendere...
components/nacl/renderer/manifest_downloader.cc:57: void
ManifestDownloader::done() {
On 2015/08/03 12:59:42, bbudge wrote:
> nit: Rename this to Close or something similar. Lower case names are a Blink
> convention. Lower case names in this file implement a Blink interface.

Oops! Fixed!

https://codereview.chromium.org/1267713003/diff/40001/components/nacl/rendere...
components/nacl/renderer/manifest_downloader.cc:66: delete this;
On 2015/08/03 12:59:42, bbudge wrote:
> If you haven't changed the Blink ThreadableLoader yet, doesn't this now do a
> double delete?

Please see this comment. url_loader_ also gets deleted when deleting this.
https://codereview.chromium.org/1267713003/diff/20001/components/nacl/rendere...

https://codereview.chromium.org/1267713003/diff/40001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/1267713003/diff/40001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1628: delete this;
On 2015/08/03 12:59:42, bbudge wrote:
> If ThreadableLoader still calls both didFail and didFinishLoading, doesn't
this
> do a double-delete?

Please see the reply to the other comment.

[bbudge, 2015-08-04 17:52:53]

lgtm

[tyoshino (SeeGerritForStatus), 2015-08-06 04:08:24]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-08-06 04:08:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/1267713003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-06 04:08:55]

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

[commit-bot: I haz the power, 2015-08-06 05:09:17]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-06 05:09:59]

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