Make sure dispatchFailLoad is triggered by stopLoading

Make sure dispatchFailLoad is triggered by stopLoading

Calling ResourceFetcher::stopFetching before
DocumentLoader::cancelMainResourceLoad may trigger a
dispatchDidFinishLoad instead of the expected dispatchFailLoad. This
patch reverts to the former ordering of the calls, which solves the
problem.

BUG=501248
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197901

[landell, 2015-06-24 07:34:57]

landell@opera.com changed reviewers:
+ japhet@chromium.org, mkwst@chromium.org, morrita@chromium.org

[landell, 2015-06-24 07:35:30]

@reviewers: PTAL. Spawning this from the bug discussion.

[Mike West, 2015-06-24 14:52:35]

On 2015/06/24 at 07:35:30, landell wrote:
> @reviewers: PTAL. Spawning this from the bug discussion.

I agree with japhet@'s comments on the bug, and the explanation makes sense to
me.

Is it possible to write a unit or layout test that would trigger the error this
patch is fixing?

[Nate Chapin, 2015-06-24 17:13:29]

On 2015/06/24 14:52:35, Mike West wrote:
> On 2015/06/24 at 07:35:30, landell wrote:
> > @reviewers: PTAL. Spawning this from the bug discussion.
> 
> I agree with japhet@'s comments on the bug, and the explanation makes sense to
> me.
> 
> Is it possible to write a unit or layout test that would trigger the error
this
> patch is fixing?

Agreed, if we have a test, that'd be ideal.

I kicked off trybots for the tests that I'd be worried about with this change.

[landell, 2015-06-24 17:27:47]

On 2015/06/24 14:52:35, Mike West wrote:
> On 2015/06/24 at 07:35:30, landell wrote:
> > @reviewers: PTAL. Spawning this from the bug discussion.
> 
> I agree with japhet@'s comments on the bug, and the explanation makes sense to
> me.
> 
> Is it possible to write a unit or layout test that would trigger the error
this
> patch is fixing?

Locally we reproduce it, by loading a web page and calling stop when we are
using a certain amount of memory. Reproduction does not happen all the time and
our observation is simply that we don't get the dispatchFailLoad call
consistently anymore (as we expect) after the order of the mentioned calls was
changed.
I agree that a proper test would be good but I don't think I could come up with
such a test in any reasonable amount of time. How this code fit together is kind
of magic to me and the reproduction seems to be dependent on the timing of
loading the page and when stopLoading is triggered.

[landell, 2015-06-24 17:32:22]

On 2015/06/24 17:13:29, Nate Chapin wrote:
> 
> Agreed, if we have a test, that'd be ideal.
> 

Do you guys have some idea on a test case?

[landell, 2015-06-25 07:02:32]

That trybot result didn't look too good. I am not used to interpret these
results but looks as this patch wont hold.

[Mike West, 2015-06-25 12:23:04]

On 2015/06/25 at 07:02:32, landell wrote:
> That trybot result didn't look too good. I am not used to interpret these
results but looks as this patch wont hold.

The linux and mac bots are incredibly flaky this week. I wouldn't worry about
them too much if they pass on a second run (as they appear to be doing). If
there's no easy way to test this, and no web-visible effect is expected, LGTM to
land it and see if the world explodes.

[Nate Chapin, 2015-06-25 17:22:11]

On 2015/06/25 12:23:04, Mike West wrote:
> On 2015/06/25 at 07:02:32, landell wrote:
> > That trybot result didn't look too good. I am not used to interpret these
> results but looks as this patch wont hold.
> 
> The linux and mac bots are incredibly flaky this week. I wouldn't worry about
> them too much if they pass on a second run (as they appear to be doing). If
> there's no easy way to test this, and no web-visible effect is expected, LGTM
to
> land it and see if the world explodes.

LGTM too.

[landell, 2015-06-26 07:37:48]

Thanks, I land this then.

[landell, 2015-06-26 07:38:16]

The CQ bit was checked by landell@opera.com

[commit-bot: I haz the power, 2015-06-26 07:38:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1205703006/1

[commit-bot: I haz the power, 2015-06-26 09:49:50]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197901