Prevent ThreadableLoaderClient methods from being called after failure notification

Prevent ThreadableLoaderClient methods from being called after failure notification

Currently there's no clear guarantee about ThreadableLoaderClient method
call. Clean up the class to make didFail.* family methods called only
once and no other methods called after them and didFinishLoading().

This includes:
- clear m_client when calling didFail.* family methods to make it clear
  that we won't callback to the client any more
- call clearResource() to prevent callback from the Resource

Since didFailAccessControlCheck() and didFailRedirectCheck() call no
longer require cancelling the loader in them,
cancel() call has been removed from
EventSource::abortConnectionAttempt().

R=hiroshige
BUG=515850
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=202445

[tyoshino (SeeGerritForStatus), 2015-07-31 07:36:51]

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

[tyoshino (SeeGerritForStatus), 2015-07-31 08:24:51]

Fixed AssociatedURLLoaderTest

[hiroshige, 2015-07-31 09:41:15]

On 2015/07/31 08:24:51, tyoshino wrote:
> Fixed AssociatedURLLoaderTest

Also browser_tests are failing (See test results of Patch Set 2).

FYI my similar previous CL was reverted due to browser_tests failure:
https://codereview.chromium.org/891263002.
See also https://code.google.com/p/chromium/issues/detail?id=421627.

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

On 2015/07/31 09:41:15, hiroshige wrote:
> On 2015/07/31 08:24:51, tyoshino wrote:
> > Fixed AssociatedURLLoaderTest
> 
> Also browser_tests are failing (See test results of Patch Set 2).
> 
> FYI my similar previous CL was reverted due to browser_tests failure:
> https://codereview.chromium.org/891263002.
> See also https://code.google.com/p/chromium/issues/detail?id=421627.

There were two issues:
- didFinishLoading() must not clear the resource. Clearing the resource would
make the downloaded file released. For successful cases (didFinishLoad() being
called without didFail()), we must not clear it.
- Some of NaCl/Pepper code expecting didFinishLoading() to be called after
didFail(). To be fixed by https://codereview.chromium.org/1267713003/

[hiroshige, 2015-08-03 13:22:19]

https://codereview.chromium.org/1262593004/diff/60001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1262593004/diff/60001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:553:
handleSuccessfulFinish(resource->identifier(), resource->loadFinishTime());
m_timeoutTimer.stop() is not executed in this branch?

https://codereview.chromium.org/1262593004/diff/60001/Source/core/page/EventS...
File Source/core/page/EventSource.cpp (left):

https://codereview.chromium.org/1262593004/diff/60001/Source/core/page/EventS...
Source/core/page/EventSource.cpp:319: m_loader->cancel();
What is the reason for omitting cancel() in this CL?

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

https://codereview.chromium.org/1262593004/diff/60001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1262593004/diff/60001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:553:
handleSuccessfulFinish(resource->identifier(), resource->loadFinishTime());
On 2015/08/03 13:22:19, hiroshige wrote:
> m_timeoutTimer.stop() is not executed in this branch?

handleSuccessfulFinish does it. If it's just completion of the preflight
request, we need to keep the timer active. So, handleSuccessfulFinish doesn't
call stop() when m_actualRequest is set.

https://codereview.chromium.org/1262593004/diff/60001/Source/core/page/EventS...
File Source/core/page/EventSource.cpp (left):

https://codereview.chromium.org/1262593004/diff/60001/Source/core/page/EventS...
Source/core/page/EventSource.cpp:319: m_loader->cancel();
On 2015/08/03 13:22:19, hiroshige wrote:
> What is the reason for omitting cancel() in this CL?

With the main change of this CL, the loader stops working after calling any of
the didFail.* family functions. So, we no longer need to call cancel() in them.

[hiroshige, 2015-08-04 10:20:16]

If we assume any ThreadableLoaderClient's methods can call
ThreadableLoader::cancel() or deref ThreadableLoader, then the following
ThreadableLoader functions might call client's didFail.*() or
didFinishLoading(), and thus
- we can't assume |this| is still alive after calling them, and
- if we do something after calling them, we must hold RefPtr to |this| and need
check that clear() is not called.
ctor
cancel
cancelWithError
dataDownloaded *
dataSent *
didReceiveResourceTiming *
didTimeout
dispatchInitialRequest
handlePreflightFailure
handlePreflightResponse
handleReceivedData *
handleResponse
handleSuccessfulFinish
loadActualRequest
loadFallbackRequestForServiceWorker
loadRequest
makeCrossOriginAccessRequest
notifyFinished
redirectReceived
responseReceived
setSerializedCachedMetadata *

('*' indicates that didFail.*()/didFinishLoading() are not called from the '*'
methods directly but can be called indirectly via ThreadableLoaderClient's
methods other than didFail.*()/didFinishLoading() that is directly called from
the '*' methods)

(I know this list will be modified by the splitting create-start CL. this list
reflects the current status)

It is better to leave this information in a comment; I think if we
reorder/regroup the methods in the header, I expect this can be done with a
couple of comments.

https://codereview.chromium.org/1262593004/diff/60001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1262593004/diff/60001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:553:
handleSuccessfulFinish(resource->identifier(), resource->loadFinishTime());
> handleSuccessfulFinish does it.
I see, thanks.

> If it's just completion of the preflight
> request, we need to keep the timer active.
If so, does this CL also fix a bug of stopping m_timeoutTimer here?

But the timer is activated again in loadRequest() called from
loadActualRequest(), and thus we don't need to keep the timer active here?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:571: }
nit: please add a comment that the code sequence above does the same thing as
clear() except for clearResource() because the resource is needed in
didFinishLoading().

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:699: 
Should we also check |m_client|?
e.g. for the case where a user calls ThreadableLoader::cancel() from
didReceiveData()?

[hiroshige, 2015-08-04 10:30:59]

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.h:78: // ResourceClient
The methods I listed in #8 corresponds
- ctor
- cancel()
- all methods between here and loadRequest() (L78-L118, inclusive) except for
reportResponseReceived().

(dataReceived() should be also included but I missed in #8)

[hiroshige, 2015-08-06 05:25:03]

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:290:
RefPtr<DocumentThreadableLoader> protect(this);
Can we remove |protect| here as well? (I'm not so sure though)

[hiroshige, 2015-08-10 15:15:13]

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Thre...
File Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Thre...
Source/core/loader/ThreadableLoader.h:136: // - didFailRedirectCheck()
How about the following?

    // Loading completes when one of the following methods are called:
    // - didFinishLoading()
    // - didFail()
    // - didFailAccessControlCheck()
    // - didFailRedirectCheck()
    // After one of these methods is called, no ThreadableLoaderClient
    // methods are called.
    //
    // When ThreadableLoader is destructed, ThreadableLoaderClient methods
    // are NOT called in response to the destruction or after destruction.
    //
    // When ThreadableLoader::cancel() is called, ThreadableLoaderClient::
    // didFail() is called with ResourceError with isCancellation() is true,
    // if any of didFinishLoading() or didFail.*() methods is not called yet.
    //
    // |ThreadableLoaderClient| methods can call ThreadableLoader::cancel()
    // or can destruct ThreadableLoader by clearing RefPtr<ThreadableLoader>.

[tyoshino (SeeGerritForStatus), 2015-08-25 10:23:18]

On 2015/08/04 10:20:16, unavailable (hiroshige) wrote:
> If we assume any ThreadableLoaderClient's methods can call
> ThreadableLoader::cancel() or deref ThreadableLoader, then the following
> ThreadableLoader functions might call client's didFail.*() or
> didFinishLoading(), and thus
> - we can't assume |this| is still alive after calling them, and
> - if we do something after calling them, we must hold RefPtr to |this| and
need
> check that clear() is not called.
> ctor
> cancel

done adding comment.

> cancelWithError

done

> dataDownloaded *

done

It's ok to destroy the resource in this method as RawResource::didDownloadData()
uses ResourceClientWalker.

Hmm, needs protect in RawResource::didDownloadData()?

> dataSent *

done

It's ok to destroy the resource. The same reason as above.

Hmm, needs protect in RawResource::didSendData?

> didReceiveResourceTiming *

done

It's ok to destroy the resource. The same reason as above.

> didTimeout

done

> dispatchInitialRequest

done

> handlePreflightFailure

done

> handlePreflightResponse

done

> handleReceivedData *

done

> handleResponse

done

> handleSuccessfulFinish

done

> loadActualRequest

same as loadRequest()

> loadFallbackRequestForServiceWorker

done

> loadRequest

Let's disallow sync ThreadableLoader users to destroy the loader in their
ThreadableLoaderClient methods.

Then, until https://codereview.chromium.org/1253343002/, this doesn't kill the
loader.

> makeCrossOriginAccessRequest

done

> notifyFinished

done

[TODO] Needs more investigation on lifetime.

[TODO] Fix WebURLRequest::FetchRedirectModeManual handling code. We must not
call clear() after notifyFinished() call.

> redirectReceived

done

It's ok to destroy the resource. The same reason as above (ResourceClientWalker
in RawResource::willFollowRedirect() and hasClient() call in
RawResource::didAddClient()).

> responseReceived

done

It's ok to destroy the resource. The same reason as above (ResourceClientWalker
in RawResource::responseReceived() and hasClient() call in
RawResource::didAddClient()).

> setSerializedCachedMetadata *

done

It's ok to destroy the resource. The same reason as above.

> 
> ('*' indicates that didFail.*()/didFinishLoading() are not called from the '*'
> methods directly but can be called indirectly via ThreadableLoaderClient's
> methods other than didFail.*()/didFinishLoading() that is directly called from
> the '*' methods)
> 
> (I know this list will be modified by the splitting create-start CL. this list
> reflects the current status)
> 
> It is better to leave this information in a comment; I think if we
> reorder/regroup the methods in the header, I expect this can be done with a
> couple of comments.

Done

I'd also allow dataReceived() to destroy the loader.

It's ok to destroy the resource. The same reason as above (ResourceClientWalker
in RawResource::appendData() and hasClient() call in
RawResource::didAddClient()).

[tyoshino (SeeGerritForStatus), 2015-08-26 12:57:33]

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:290:
RefPtr<DocumentThreadableLoader> protect(this);
On 2015/08/06 05:25:03, hiroshige (slow) wrote:
> Can we remove |protect| here as well? (I'm not so sure though)

Agreed. Removed.

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.h:78: // ResourceClient
On 2015/08/04 10:30:59, hiroshige (slow) wrote:
> The methods I listed in #8 corresponds
> - ctor
> - cancel()
> - all methods between here and loadRequest() (L78-L118, inclusive) except for
> reportResponseReceived().
> 
> (dataReceived() should be also included but I missed in #8)

Thanks. This is done except for loadActualRequest() and loadRequest(). See my
last post for the reason why these are excluded.

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Thre...
File Source/core/loader/ThreadableLoader.h (right):

https://codereview.chromium.org/1262593004/diff/80001/Source/core/loader/Thre...
Source/core/loader/ThreadableLoader.h:136: // - didFailRedirectCheck()
On 2015/08/10 15:15:13, hiroshige (slow) wrote:
> How about the following?
> 
>     // Loading completes when one of the following methods are called:
>     // - didFinishLoading()
>     // - didFail()
>     // - didFailAccessControlCheck()
>     // - didFailRedirectCheck()
>     // After one of these methods is called, no ThreadableLoaderClient
>     // methods are called.
>     //
>     // When ThreadableLoader is destructed, ThreadableLoaderClient methods
>     // are NOT called in response to the destruction or after destruction.
>     //
>     // When ThreadableLoader::cancel() is called, ThreadableLoaderClient::
>     // didFail() is called with ResourceError with isCancellation() is true,
>     // if any of didFinishLoading() or didFail.*() methods is not called yet.
>     //
>     // |ThreadableLoaderClient| methods can call ThreadableLoader::cancel()
>     // or can destruct ThreadableLoader by clearing RefPtr<ThreadableLoader>.

Done.

[tyoshino (SeeGerritForStatus), 2015-09-01 13:25:03]

On 2015/08/25 10:23:18, tyoshino wrote:
> > > dataDownloaded *
> 
> done
> 
> It's ok to destroy the resource in this method as
RawResource::didDownloadData()
> uses ResourceClientWalker.
> 
> Hmm, needs protect in RawResource::didDownloadData()?
> 
> > dataSent *
> 
> done
> 
> It's ok to destroy the resource. The same reason as above.
> 
> Hmm, needs protect in RawResource::didSendData?
> 

Filed https://code.google.com/p/chromium/issues/detail?id=527046 for these
issues.

[tyoshino (SeeGerritForStatus), 2015-09-15 06:55:49]

On 2015/09/01 13:25:03, tyoshino wrote:
> Filed https://code.google.com/p/chromium/issues/detail?id=527046 for these
> issues.

Has been fixed.

[tyoshino (SeeGerritForStatus), 2015-09-15 06:58:03]

On 2015/08/25 10:23:18, tyoshino wrote:
> > notifyFinished
> 
> done
> 
> [TODO] Needs more investigation on lifetime.
> 
> [TODO] Fix WebURLRequest::FetchRedirectModeManual handling code. We must not
> call clear() after notifyFinished() call.

Prevented preflight from being handled here by
https://chromium.googlesource.com/chromium/blink/+/e8ceecdf6f49da6251a68b264c...

And, done refactoring to call clear() properly.

[tyoshino (SeeGerritForStatus), 2015-09-15 07:15:17]

On 2015/09/15 06:58:03, tyoshino wrote:
> On 2015/08/25 10:23:18, tyoshino wrote:
> > > notifyFinished
> > 
> > done
> > 
> > [TODO] Needs more investigation on lifetime.
> > 

Now checking this.

> > [TODO] Fix WebURLRequest::FetchRedirectModeManual handling code. We must not
> > call clear() after notifyFinished() call.
> 
> Prevented preflight from being handled here by
>
https://chromium.googlesource.com/chromium/blink/+/e8ceecdf6f49da6251a68b264c...
> 
> And, done refactoring to call clear() properly.

Sorry. The new patch set was not yet uploaded.

[tyoshino (SeeGerritForStatus), 2015-09-15 07:15:36]

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

[commit-bot: I haz the power, 2015-09-15 07:15:43]

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

[tyoshino (SeeGerritForStatus), 2015-09-15 07:30:08]

On 2015/09/15 07:15:17, tyoshino wrote:
> On 2015/09/15 06:58:03, tyoshino wrote:
> > On 2015/08/25 10:23:18, tyoshino wrote:
> > > > notifyFinished
> > > 
> > > done
> > > 
> > > [TODO] Needs more investigation on lifetime.
> > > 
> 
> Now checking this.

Seems ok. In call stacks to notifyFinished(), the resource is protected by
callers.

[tyoshino (SeeGerritForStatus), 2015-09-15 07:42:11]

Ready for review again.

[commit-bot: I haz the power, 2015-09-15 08:24:09]

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

[commit-bot: I haz the power, 2015-09-15 08:24:09]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2015-09-15 08:54:17]

https://codereview.chromium.org/1262593004/diff/180001/Source/core/loader/Doc...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1262593004/diff/180001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:340:
RefPtr<DocumentThreadableLoader> protect(this);
Is this |protect| needed?
(This |protect| is removed in previous Patch Set and re-introduced in this CL.
is it intentional?)

https://codereview.chromium.org/1262593004/diff/180001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:366: if
(resource->errorOccurred()) {
What is the reason for calling handleError() /
handleSuccessfulActualRequestFinish() directly here, rather than calling
notifyFinished()?
This block (L366-372) seems to do the same thing as notifyFinished() because
|m_actualRequest| is false.

https://codereview.chromium.org/1262593004/diff/180001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:739: // ResourceError outlive
the resource for convenience.
This sounds as if |copiedError| *always* outlive the resource.
How about "... make it sure that ResourceError is alive during didFail() even
when Resource is destructed during didFail()." or so?

https://codereview.chromium.org/1262593004/diff/180001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:824: 
Do we need "if (!m_client) return" also here for cancel() in didReceiveData()?

[tyoshino (SeeGerritForStatus), 2015-09-15 13:29:35]

https://codereview.chromium.org/1262593004/diff/180001/Source/core/loader/Doc...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1262593004/diff/180001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:340:
RefPtr<DocumentThreadableLoader> protect(this);
On 2015/09/15 08:54:16, hiroshige wrote:
> Is this |protect| needed?
> (This |protect| is removed in previous Patch Set and re-introduced in this CL.
> is it intentional?)

Seems accidentally re-introduced on merge.

But it's actually required since responseReceived() at L364 may let the client
drop references. Moved to L354.

https://codereview.chromium.org/1262593004/diff/180001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:366: if
(resource->errorOccurred()) {
On 2015/09/15 08:54:17, hiroshige wrote:
> What is the reason for calling handleError() /
> handleSuccessfulActualRequestFinish() directly here, rather than calling
> notifyFinished()?
> This block (L366-372) seems to do the same thing as notifyFinished() because
> |m_actualRequest| is false.

Ah, after refactoring they became identical. Reverted the refactoring.

https://codereview.chromium.org/1262593004/diff/180001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:739: // ResourceError outlive
the resource for convenience.
On 2015/09/15 08:54:16, hiroshige wrote:
> This sounds as if |copiedError| *always* outlive the resource.
> How about "... make it sure that ResourceError is alive during didFail() even
> when Resource is destructed during didFail()." or so?

Good point and nice rephrasing. Done

https://codereview.chromium.org/1262593004/diff/180001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:824: 
On 2015/09/15 08:54:17, hiroshige wrote:
> Do we need "if (!m_client) return" also here for cancel() in didReceiveData()?

Right! We need to take care of cancel().

[tyoshino (SeeGerritForStatus), 2015-09-15 13:29:44]

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

[commit-bot: I haz the power, 2015-09-15 13:29:49]

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

[commit-bot: I haz the power, 2015-09-15 13:58:38]

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

[commit-bot: I haz the power, 2015-09-15 13:58:39]

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

[hiroshige, 2015-09-16 04:09:47]

https://codereview.chromium.org/1262593004/diff/200001/Source/core/loader/Doc...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1262593004/diff/200001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:368: ASSERT(!m_actualRequest);
Here |m_client| might be null (i.e. clear() might be already called) because
cancel() can be called in responseReceived(). We should check |if (m_client)|
here. (Found when implementing my unittests)

[tyoshino (SeeGerritForStatus), 2015-09-16 05:41:16]

https://codereview.chromium.org/1262593004/diff/200001/Source/core/loader/Doc...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1262593004/diff/200001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:368: ASSERT(!m_actualRequest);
On 2015/09/16 04:09:47, hiroshige wrote:
> Here |m_client| might be null (i.e. clear() might be already called) because
> cancel() can be called in responseReceived(). We should check |if (m_client)|
> here. (Found when implementing my unittests)

I thought I fixed this too in addition to L821, but wasn't...

Fixed.

[tyoshino (SeeGerritForStatus), 2015-09-16 05:43:04]

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

[commit-bot: I haz the power, 2015-09-16 05:43:30]

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

[commit-bot: I haz the power, 2015-09-16 07:46:55]

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

[commit-bot: I haz the power, 2015-09-16 07:46:56]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, no
build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, no
build URL)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, no build URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hiroshige, 2015-09-16 07:54:51]

lgtm. Thanks for your effort!!!

[tyoshino (SeeGerritForStatus), 2015-09-16 08:28:27]

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

[tyoshino (SeeGerritForStatus), 2015-09-16 08:28:50]

Hi Mike,

Could you please review this?

[Mike West, 2015-09-16 12:38:48]

LGTM.

[tyoshino (SeeGerritForStatus), 2015-09-17 06:43:58]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-09-17 06:43:58]

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

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

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

[commit-bot: I haz the power, 2015-09-17 06:57:18]

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

[commit-bot: I haz the power, 2015-09-17 06:57:19]

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

[hiroshige, 2015-09-17 07:09:51]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2015-09-17 07:09:59]

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

[commit-bot: I haz the power, 2015-09-17 07:29:17]

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

[commit-bot: I haz the power, 2015-09-17 07:29:18]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[hiroshige, 2015-09-17 08:32:31]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2015-09-17 08:32:45]

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

[hiroshige, 2015-09-17 08:39:33]

https://codereview.chromium.org/1262593004/diff/240001/Source/core/loader/Doc...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1262593004/diff/240001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:667:
ASSERT(!m_sameOriginRequest);
This assertion is failing in many browser tests, e.g.
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...

[tyoshino (SeeGerritForStatus), 2015-09-17 08:45:41]

https://codereview.chromium.org/1262593004/diff/240001/Source/core/loader/Doc...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1262593004/diff/240001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:667:
ASSERT(!m_sameOriginRequest);
On 2015/09/17 08:39:33, hiroshige wrote:
> This assertion is failing in many browser tests, e.g.
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...

Sorry. I just forgot to fix the if-clause after reverting
handleSuccessfulActualRequestFinish refactoring.

Fixing

[tyoshino (SeeGerritForStatus), 2015-09-17 08:46:50]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-09-17 08:46:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1262593004/#ps260001
(title: "Fixed handleSuccessfulFinish condition")

[commit-bot: I haz the power, 2015-09-17 08:46:54]

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

[hiroshige, 2015-09-17 08:54:22]

lgtm.

[commit-bot: I haz the power, 2015-09-17 11:43:44]

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

[commit-bot: I haz the power, 2015-09-17 11:43:45]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tyoshino (SeeGerritForStatus), 2015-09-17 11:46:27]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2015-09-17 11:46:39]

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

[commit-bot: I haz the power, 2015-09-17 12:40:29]

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