Deal with canceled requests when flushing deferred messages.

Deal with canceled requests when flushing deferred messages.

Flushing deferred messages might lead to a request being canceled
(e.g. when an ImageResource loads a corrupt image). The code didn't
fully take this into account which would cause crashes (and resource
leaks if it would have survived).

BUG=

Committed: https://crrev.com/864cf45b30cce9d7cc15928a3e0b6be5422693c1
Cr-Commit-Position: refs/heads/master@{#427298}

[jb, 2016-10-18 11:54:54]

Description was changed from

==========
Deal with canceled requests when flushing deferred messages.

Flushing deferred messages might lead to a request being canceled
(e.g. when an ImageResource loads a corrupt image). The code didn't
fully take this into account which would cause crashes (and resource
leaks if it would have survived).

BUG=
==========

to

==========
Deal with canceled requests when flushing deferred messages.

Flushing deferred messages might lead to a request being canceled
(e.g. when an ImageResource loads a corrupt image). The code didn't
fully take this into account which would cause crashes (and resource
leaks if it would have survived).

BUG=
==========

[jb, 2016-10-18 11:54:54]

jb@opera.com changed reviewers:
+ jam@chromium.org, kinuko@chromium.org

[jb, 2016-10-18 11:59:46]

PTAL

[jam, 2016-10-19 04:55:43]

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

[jam, 2016-10-19 04:56:21]

This seems reasonable, but can you write a test? And is there no bug for this?

[commit-bot: I haz the power, 2016-10-19 04:57:55]

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

[jb, 2016-10-19 07:46:47]

On 2016/10/19 04:56:21, jam wrote:
> This seems reasonable, but can you write a test? And is there no bug for this?

I haven't really looked into making a proper test for this. I will see what I
can do. I am a bit short on time so it might take a while.

As for a bug report on this. I don't known of any existing bug for this. I
haven't filed one.

[commit-bot: I haz the power, 2016-10-19 12:01:05]

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

[commit-bot: I haz the power, 2016-10-19 12:01:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kinuko, 2016-10-19 13:35:53]

+1 to have some tests.  How did you find the problem or do you have any repro? 
If you can file a bug with the repro (and link to this patch) someone  will be
able to help you write a test or may be able to take over (if you prefer).

https://codereview.chromium.org/2425173003/diff/1/content/child/resource_disp...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2425173003/diff/1/content/child/resource_disp...
content/child/resource_dispatcher.cc:176: // Dispatches the messages only if the
request is no longer deferred.
nit: to make it clearer that FlushDeferredMessages don't flush things when
is_deferred is true can we have if (!is_deferred) here too or make the comment
even clearer, e.g. "it dispatches only when request_info->is_deferred is false"

https://codereview.chromium.org/2425173003/diff/1/content/child/resource_disp...
content/child/resource_dispatcher.cc:596: ReleaseResourcesInMessageQueue(&q);
I think if GetPendingRequestInfo returns null ReleaseResourcesInMessageQueue
should be also already called by RemovePendingRequest?

[jb, 2016-10-20 13:06:50]

https://codereview.chromium.org/2425173003/diff/1/content/child/resource_disp...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2425173003/diff/1/content/child/resource_disp...
content/child/resource_dispatcher.cc:176: // Dispatches the messages only if the
request is no longer deferred.
On 2016/10/19 13:35:53, kinuko (slowish) wrote:
> nit: to make it clearer that FlushDeferredMessages don't flush things when
> is_deferred is true can we have if (!is_deferred) here too or make the comment
> even clearer, e.g. "it dispatches only when request_info->is_deferred is
false"

Makes sense. I will add an extra if for clarity.

https://codereview.chromium.org/2425173003/diff/1/content/child/resource_disp...
content/child/resource_dispatcher.cc:596: ReleaseResourcesInMessageQueue(&q);
On 2016/10/19 13:35:53, kinuko (slowish) wrote:
> I think if GetPendingRequestInfo returns null ReleaseResourcesInMessageQueue
> should be also already called by RemovePendingRequest?

In this context the queue in the request_info is swapped out for the empty
queue, so it won't be able to clean it.

[jb, 2016-10-20 13:24:35]

On 2016/10/19 13:35:53, kinuko (slowish) wrote:
> +1 to have some tests.  How did you find the problem or do you have any repro?

> If you can file a bug with the repro (and link to this patch) someone  will be
> able to help you write a test or may be able to take over (if you prefer).
> 
One of our tests started to crash and I tracked it down to this problem.
Unfortunately I can't share the test. It is quite Opera specific and uses an
internal testing framework.

The problem can occur when loading of a corrupt image is deffered. The way
corrupt images cancels resource loads changed a while ago. I tracked it down to
https://codereview.chromium.org/2173873003. It was the reason for the crash in
our case, but I guess there could be other ways to get here as well.

I have added two unittests now. The last one
CancelWhileFlushingDeferredRequestsFromOnMessageReceived should be similar to
what happened in the case I looked into.

[kinuko, 2016-10-20 18:10:19]

The tests look great, thanks for adding them! LGTM % nit

https://codereview.chromium.org/2425173003/diff/20001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2425173003/diff/20001/content/child/resource_...
content/child/resource_dispatcher.cc:179: }
nit: Hm, I found having nested if around is_deferred makes the code a bit hard
to follow (though that's what I asked-- sorry :( ) --I think we could just keep
two if's, one for request_info->is_deferred and the other for
!message_queue.empty()?

[jb, 2016-10-21 07:28:48]

https://codereview.chromium.org/2425173003/diff/20001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2425173003/diff/20001/content/child/resource_...
content/child/resource_dispatcher.cc:179: }
On 2016/10/20 18:10:18, kinuko (slowish) wrote:
> nit: Hm, I found having nested if around is_deferred makes the code a bit hard
> to follow (though that's what I asked-- sorry :( ) --I think we could just
keep
> two if's, one for request_info->is_deferred and the other for
> !message_queue.empty()?

NP, I agree. I will fix it.

[kinuko, 2016-10-24 14:12:06]

On 2016/10/21 07:28:48, jb wrote:
>
https://codereview.chromium.org/2425173003/diff/20001/content/child/resource_...
> File content/child/resource_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/2425173003/diff/20001/content/child/resource_...
> content/child/resource_dispatcher.cc:179: }
> On 2016/10/20 18:10:18, kinuko (slowish) wrote:
> > nit: Hm, I found having nested if around is_deferred makes the code a bit
hard
> > to follow (though that's what I asked-- sorry :( ) --I think we could just
> keep
> > two if's, one for request_info->is_deferred and the other for
> > !message_queue.empty()?
> 
> NP, I agree. I will fix it.

(still lgtm)

[jb, 2016-10-25 06:50:02]

The CQ bit was checked by jb@opera.com

[commit-bot: I haz the power, 2016-10-25 06:50:15]

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-25 08:34:46]

Description was changed from

==========
Deal with canceled requests when flushing deferred messages.

Flushing deferred messages might lead to a request being canceled
(e.g. when an ImageResource loads a corrupt image). The code didn't
fully take this into account which would cause crashes (and resource
leaks if it would have survived).

BUG=
==========

to

==========
Deal with canceled requests when flushing deferred messages.

Flushing deferred messages might lead to a request being canceled
(e.g. when an ImageResource loads a corrupt image). The code didn't
fully take this into account which would cause crashes (and resource
leaks if it would have survived).

BUG=
==========

[commit-bot: I haz the power, 2016-10-25 08:34:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-25 08:37:01]

Description was changed from

==========
Deal with canceled requests when flushing deferred messages.

Flushing deferred messages might lead to a request being canceled
(e.g. when an ImageResource loads a corrupt image). The code didn't
fully take this into account which would cause crashes (and resource
leaks if it would have survived).

BUG=
==========

to

==========
Deal with canceled requests when flushing deferred messages.

Flushing deferred messages might lead to a request being canceled
(e.g. when an ImageResource loads a corrupt image). The code didn't
fully take this into account which would cause crashes (and resource
leaks if it would have survived).

BUG=

Committed: https://crrev.com/864cf45b30cce9d7cc15928a3e0b6be5422693c1
Cr-Commit-Position: refs/heads/master@{#427298}
==========

[commit-bot: I haz the power, 2016-10-25 08:37:02]

Patchset 3 (id:??) landed as
https://crrev.com/864cf45b30cce9d7cc15928a3e0b6be5422693c1
Cr-Commit-Position: refs/heads/master@{#427298}