FetchResponse should close its stream when loading finishes

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Currently it is not doing so until pulling happens.

[youenn fablet]

Created attachment 345299 [details]
Patch

[youenn fablet]

<rdar://problem/42311832>

[Chris Dumez]

Comment on attachment 345299 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345299&action=review

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:50
> +    if (m_readableStreamSource)

m_readableStreamSource gets nulled out in several methods in this class, in which case, the source may be kept alive (due to refcounting) but will still have a pointer to this owner. I think we want to clear the source's body owner in all cases where m_readableStreamSource gets cleared, not used in the BodyOwner destructor.

> Source/WebCore/Modules/fetch/FetchBodySource.cpp:89
> +void FetchBodySource::cleanBodyOwner()

I do not like "clean", I'd prefer "clear" or "detachFrom"

[Chris Dumez]

Comment on attachment 345299 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345299&action=review

>> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:50
>> +    if (m_readableStreamSource)
> 
> m_readableStreamSource gets nulled out in several methods in this class, in which case, the source may be kept alive (due to refcounting) but will still have a pointer to this owner. I think we want to clear the source's body owner in all cases where m_readableStreamSource gets cleared, not used in the BodyOwner destructor.

not *just* in the BodyOwner destructor.

[youenn fablet]

> m_readableStreamSource gets nulled out in several methods in this class, in
> which case, the source may be kept alive (due to refcounting) but will still
> have a pointer to this owner. I think we want to clear the source's body
> owner in all cases where m_readableStreamSource gets cleared, not used in
> the BodyOwner destructor.

I do not think this is actually needed and we will probably want to either ASSERT or do the same thing in BodyOwner destructor.
I agree though that we could ensure that the source is in a closed/errored state whenever FetchBodyOwner nullify its source. Doing all of this at about the same place in code might be more future proof.
I'll update the patch accordingly.

> > Source/WebCore/Modules/fetch/FetchBodySource.cpp:89
> > +void FetchBodySource::cleanBodyOwner()
> 
> I do not like "clean", I'd prefer "clear" or "detachFrom"

OK

[youenn fablet]

Created attachment 345353 [details]
Patch

[youenn fablet]

Created attachment 345354 [details]
Patch

[Chris Dumez]

Comment on attachment 345353 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345353&action=review

> Source/WebCore/Modules/fetch/FetchResponse.cpp:259
> +            m_response.m_readableStreamSource->error(makeString("Loading failed"_s, error.localizedDescription()));

Don't we need a ": " between "Loading failed" and the description?

[youenn fablet]

Created attachment 345355 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 345355 [details]
Patch for landing

Clearing flags on attachment: 345355

Committed r233994: <https://trac.webkit.org/changeset/233994>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

mac-wk1 debug EWS bots are failing one of the assertions added in this change:

Thread 29 Crashed:: WebCore: Worker
0   com.apple.JavaScriptCore            0x0000000107d20ff0 WTFCrash + 16 (Assertions.cpp:267)
1   com.apple.WebCore                   0x000000011491d157 WebCore::FetchBodySource::setInactive() + 71 (FetchBodySource.cpp:50)
2   com.apple.WebCore                   0x00000001142c230e WebCore::ReadableStreamSource::clean() + 78 (ReadableStreamSource.h:117)
3   com.apple.WebCore                   0x0000000114918c4b WebCore::FetchBodySource::close() + 59 (FetchBodySource.cpp:85)
4   com.apple.WebCore                   0x0000000114918354 WebCore::FetchBody::consumeAsStream(WebCore::FetchBodyOwner&, WebCore::FetchBodySource&) + 1300 (FetchBody.cpp:197)
5   com.apple.WebCore                   0x000000011491ce84 WebCore::FetchBodyOwner::consumeBodyAsStream() + 340 (FetchBodyOwner.cpp:328)
6   com.apple.WebCore                   0x00000001149343f9 WebCore::FetchResponse::consumeBodyAsStream() + 121 (FetchResponse.cpp:422)
7   com.apple.WebCore                   0x000000011491d1fc WebCore::FetchBodySource::doStart() + 108 (FetchBodySource.cpp:60)

https://webkit-queues.webkit.org/results/8589680

[Ryan Haddad]

Same on WK2:
https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r233994%20(5483)/http/tests/fetch/bodyUsed-crash-log.txt

[Dawei Fenton (:realdawei)]

Reverted r233994 for reason:

Caused EWS and bot failures due to assertions added

Committed r234003: <https://trac.webkit.org/changeset/234003>

[youenn fablet]

Thanks David!

[Chris Dumez]

Comment on attachment 345355 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=345355&action=review

> Source/WebCore/Modules/fetch/FetchBodySource.cpp:82
> +    m_bodyOwner = nullptr;

Maybe we should clear this at the *end* of the method?

> Source/WebCore/Modules/fetch/FetchBodySource.cpp:89
> +    m_bodyOwner = nullptr;

ditto.

[youenn fablet]

Created attachment 345446 [details]
Fixing ASSERTs

[Chris Dumez]

Comment on attachment 345446 [details]
Fixing ASSERTs

LGTM, no crashes here in stress gc or debug.

[youenn fablet]

Did two changes to fix ASSERTS:
- clear m_bodyOwner at the end of the method
- handle the specific case of cancelling a stream that is closing but not closed. In that case, even though controller.close() is called, the stream is not called until all chunks are read. Cancel can be used in between. I added a m_isClosed debug boolean to ensure this.

[youenn fablet]

s/end of the method/end of the methods/

[WebKit Commit Bot]

Comment on attachment 345446 [details]
Fixing ASSERTs

Clearing flags on attachment: 345446

Committed r234045: <https://trac.webkit.org/changeset/234045>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

Comment on attachment 345446 [details]
Fixing ASSERTs

View in context: https://bugs.webkit.org/attachment.cgi?id=345446&action=review

> Source/WebCore/Modules/fetch/FetchBodySource.cpp:82
> +#ifndef NDEBUG

This should be:

#if !ASSERT_DISABLED

Because it is possible to compile a Release build with all asserts enabled.

> Source/WebCore/Modules/fetch/FetchBodySource.h:63
> +#ifndef NDEBUG

This should be:

#if !ASSERT_DISABLED

Because it is possible to compile a Release build with all asserts enabled.