Notify when resource with error code >= 400 is finished.

Notify when resource with error code >= 400 is finished.

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

[eustas, 2014-01-31 15:47:40]

jochen@chromium.org: Please review changes in core/fetch/

vsevik@chromium.org: Please review changes in devtools/

[jochen (gone - plz use gerrit), 2014-01-31 15:51:18]

I'm not really a good reviewer here. Adding folks that touch fetch more often
than me

[eustas, 2014-01-31 15:56:23]

On 2014/01/31 15:51:18, jochen wrote:
> I'm not really a good reviewer here. Adding folks that touch fetch more often
> than me

Thanks

[Nate Chapin, 2014-01-31 17:25:51]

https://codereview.chromium.org/138843009/diff/1/Source/core/fetch/ResourceLo...
File Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/138843009/diff/1/Source/core/fetch/ResourceLo...
Source/core/fetch/ResourceLoader.cpp:362: if (!m_notifiedLoadComplete) {
I'm not sure this is safe. Specifically, if m_resource->error() triggers an
event, it's possible that this ResourceLoader will get cancelled synchronously
inside. If that happens, because m_state is already Finishing, cancel() will
call releaseResources() and clear m_host without setting m_notifiedLoadComplete
to true.

I suspect that here and throughout, we should make sure the
m_host->didFinishLoading() or m_host->didFailLoading() call happens before the
m_resource->finish() or m_resource->error().

[eustas, 2014-02-03 12:56:05]

PTAL

https://codereview.chromium.org/138843009/diff/1/Source/core/fetch/ResourceLo...
File Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/138843009/diff/1/Source/core/fetch/ResourceLo...
Source/core/fetch/ResourceLoader.cpp:362: if (!m_notifiedLoadComplete) {
On 2014/01/31 17:25:52, Nate Chapin wrote:
> I'm not sure this is safe. Specifically, if m_resource->error() triggers an
> event, it's possible that this ResourceLoader will get cancelled synchronously
> inside. If that happens, because m_state is already Finishing, cancel() will
> call releaseResources() and clear m_host without setting
m_notifiedLoadComplete
> to true.
> 
> I suspect that here and throughout, we should make sure the
> m_host->didFinishLoading() or m_host->didFailLoading() call happens before the
> m_resource->finish() or m_resource->error().

Done.

[Nate Chapin, 2014-02-03 18:01:49]

LGTM, thanks!

https://codereview.chromium.org/138843009/diff/70001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/138843009/diff/70001/Source/core/fetch/Resour...
Source/core/fetch/ResourceLoader.cpp:90: if (!m_notifiedLoadComplete) {
I *think* it's impossible to get here without m_notifiedLoadComplete being true
now. The only way it could happen is if cancel() can get called when (m_state ==
Finishing && !m_notifiedLoadComplete), and I think you've prevented that
thoroughly.

If you agree, feel free to change this back to an ASSERT before landing.

https://codereview.chromium.org/138843009/diff/70001/Source/core/fetch/Resour...
Source/core/fetch/ResourceLoader.cpp:369:
m_resource->error(Resource::LoadError);
Hmm...I wonder if cancel() could just take care of the didFailLoading() and
error() calls here.

That's a patch for another time, though. :)

[vsevik, 2014-02-04 13:02:09]

devtools lgtm

[eustas, 2014-02-05 11:58:16]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-02-05 11:58:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eustas@chromium.org/138843009/110001

[commit-bot: I haz the power, 2014-02-05 14:50:17]

Change committed as 166502