Currently it is not doing so until pulling happens.
Created attachment 345299 [details] Patch
<rdar://problem/42311832>
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"
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.
> 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
Created attachment 345353 [details] Patch
Created attachment 345354 [details] Patch
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?
Created attachment 345355 [details] Patch for landing
Comment on attachment 345355 [details] Patch for landing Clearing flags on attachment: 345355 Committed r233994: <https://trac.webkit.org/changeset/233994>
All reviewed patches have been landed. Closing bug.
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
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
Reverted r233994 for reason: Caused EWS and bot failures due to assertions added Committed r234003: <https://trac.webkit.org/changeset/234003>
Thanks David!
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.
Created attachment 345446 [details] Fixing ASSERTs
Comment on attachment 345446 [details] Fixing ASSERTs LGTM, no crashes here in stress gc or debug.
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.
s/end of the method/end of the methods/
Comment on attachment 345446 [details] Fixing ASSERTs Clearing flags on attachment: 345446 Committed r234045: <https://trac.webkit.org/changeset/234045>
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.