Bug 155637

Created attachment 274433 [details] WIP

Created attachment 274579 [details] Patch

Attachment 274579 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/fetch/FetchBody.h:80: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 274579 [details] Patch Attachment 274579 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1015339 New failing tests: imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection-2.html

Created attachment 274580 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

(In reply to comment #4) > Comment on attachment 274579 [details] > Patch > > Attachment 274579 [details] did not pass mac-debug-ews (mac): > Output: http://webkit-queues.webkit.org/results/1015339 > > New failing tests: > imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection- > 2.html Marking this test as flaky in https://trac.webkit.org/changeset/198487

Comment on attachment 274579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274579&action=review This looks like great progress, but I think it needs some significant changes before going in. > Source/WebCore/Modules/fetch/FetchManager.cpp:74 > + m_pendingResponses.removeFirstMatching([pointer](const RefPtr<FetchResponse>& item) { I don't think this is a good design. If we have lots of pending responses, we would have to iterate all of them just to remove one of them. Please use a HashSet or a HashMap instead of a Vector. > Source/WebCore/Modules/fetch/FetchManager.h:51 > + void removePendingResponse(FetchResponse&); Right now this is called in BodyLoader::didReceiveResponse and BodyLoader::didFail. Can a request be cancelled/aborted? That might just not be implemented yet, which is fine for now. > Source/WebCore/Modules/fetch/FetchResponse.h:99 > + FetchResponse& response; > + FetchManager& manager; > + FetchPromise promise; > + std::unique_ptr<FetchLoader> loader; Please use the m_ prefix for member variable names. It makes it easier to see where things are coming from. Also, what happens when a FetchManager is destroyed with pending fetch responses? As it stands right now, there is still a FetchManager& here that could lead to use-after-free bugs. > Source/WebCore/platform/network/BlobResourceHandle.cpp:585 > + response.setHTTPHeaderField(HTTPHeaderName::ContentLength, String::format("%lld", m_totalRemainingSize)); Should this be m_totalSize?

Comment on attachment 274579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274579&action=review >> Source/WebCore/Modules/fetch/FetchManager.cpp:74 >> + m_pendingResponses.removeFirstMatching([pointer](const RefPtr<FetchResponse>& item) { > > I don't think this is a good design. If we have lots of pending responses, we would have to iterate all of them just to remove one of them. Please use a HashSet or a HashMap instead of a Vector. I assume that random order is not OK, so it needs to be ListHashSet.

Comment on attachment 274579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274579&action=review > Source/WebCore/ChangeLog:10 > + Introducing FetchManager which stores the list of pending FetchResponse objects. I don't understand the need for this. Right now there is only one FetchManager per window. Will there be one for each worker? The loading code I'm familiar with just gives the network stack (in my case CFNetwork) a ResourceRequest and it "magically" gets a ResourceResponse without having to manage the pending responses in WebKit. Do we need to do this now for the fetch api?

(In reply to comment #7) > Comment on attachment 274579 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=274579&action=review > > This looks like great progress, but I think it needs some significant > changes before going in. > > > Source/WebCore/Modules/fetch/FetchManager.cpp:74 > > + m_pendingResponses.removeFirstMatching([pointer](const RefPtr<FetchResponse>& item) { > > I don't think this is a good design. If we have lots of pending responses, > we would have to iterate all of them just to remove one of them. Please use > a HashSet or a HashMap instead of a Vector. > > > Source/WebCore/Modules/fetch/FetchManager.h:51 > > + void removePendingResponse(FetchResponse&); > > Right now this is called in BodyLoader::didReceiveResponse and > BodyLoader::didFail. Can a request be cancelled/aborted? That might just > not be implemented yet, which is fine for now. A request cannot be aborted right now, except if the document is destroyed. > > Source/WebCore/Modules/fetch/FetchResponse.h:99 > > + FetchResponse& response; > > + FetchManager& manager; > > + FetchPromise promise; > > + std::unique_ptr<FetchLoader> loader; > > Please use the m_ prefix for member variable names. It makes it easier to > see where things are coming from. I also prefer using m_prefixes, but I was thinking WebKit style for struct was omitting the m_ prefix. > Also, what happens when a FetchManager is destroyed with pending fetch > responses? As it stands right now, there is still a FetchManager& here that > could lead to use-after-free bugs. The responses would also get destroyed. > > Source/WebCore/platform/network/BlobResourceHandle.cpp:585 > > + response.setHTTPHeaderField(HTTPHeaderName::ContentLength, String::format("%lld", m_totalRemainingSize)); > > Should this be m_totalSize? m_totalRemainingSize is used as the expected content length for the ResourceResponse constructor, hence why I used it.

(In reply to comment #9) > Comment on attachment 274579 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=274579&action=review > > > Source/WebCore/ChangeLog:10 > > + Introducing FetchManager which stores the list of pending FetchResponse objects. > > I don't understand the need for this. Right now there is only one > FetchManager per window. Will there be one for each worker? The loading > code I'm familiar with just gives the network stack (in my case CFNetwork) a > ResourceRequest and it "magically" gets a ResourceResponse without having to > manage the pending responses in WebKit. Do we need to do this now for the > fetch api? Let's switch to ActiveDOMObject setPendingActivity.

Created attachment 274653 [details] Patch

Created attachment 274654 [details] Moving to ActiveDOMObject

Attachment 274654 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/fetch/FetchBody.h:80: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #13) > Created attachment 274654 [details] > Moving to ActiveDOMObject This patch removes FetchManager.cpp and adds some more tests.

Comment on attachment 274654 [details] Moving to ActiveDOMObject View in context: https://bugs.webkit.org/attachment.cgi?id=274654&action=review > Source/WebCore/Modules/fetch/DOMWindowFetch.cpp:40 > +void DOMWindowFetch::fetch(DOMWindow& window, FetchRequest* request, const Dictionary& dictionary, DeferredWrapper&& promise) I don’t think the name “request” here is great. It’s like an “underlying” request? It’s strange that this fetch request gets wrapped in another. Is that really right? > Source/WebCore/Modules/fetch/DOMWindowFetch.cpp:48 > + if (ec || !fetchRequest) { Is it right to check both things here? Is it really possible to have a failure with exception code zero? What does that mean exactly? Is it possible to have a failure that returns non-null? I think we should only be checking one of these two things. > Source/WebCore/Modules/fetch/DOMWindowFetch.cpp:64 > + if (ec || !fetchRequest) { Ditto. > Source/WebCore/Modules/fetch/DOMWindowFetch.h:44 > + static void fetch(DOMWindow&, FetchRequest*, const Dictionary&, DeferredWrapper&&); How can we change this FetchRequest* into a FetchRequest&? At the moment, if we pass a null in here it will crash later. Are we using a pointer here because of some limitation of the bindings generator? > Source/WebCore/Modules/fetch/FetchBody.cpp:45 > +static inline RefPtr<Blob> blobFromArrayBuffer(const RefPtr<ArrayBuffer>&, const String&); Putting “inline” here has no effect. Please don’t. Argument type should be ArrayBuffer* or ArrayBuffer&. Almost never helpful to pass a const RefPtr& to something. > Source/WebCore/Modules/fetch/FetchBody.cpp:175 > + m_consumer = Consumer({type, WTFMove(promise)}); Syntax here seems peculiar. Normally we use space between braces. Also strange that we need to specific a type explicitly if using argument list syntax. > Source/WebCore/Modules/fetch/FetchBody.cpp:195 > + fulfillTextPromise(type, TextResourceDecoder::create(ASCIILiteral("text/plain"), "UTF-8")->decodeAndFlush(static_cast<const char*>(m_data->data()), m_data->byteLength()), promise); Is there no more efficient way to properly decode UTF-8 than creating a text resource decoder with these string arguments? We should investigate that. > Source/WebCore/Modules/fetch/FetchBody.cpp:252 > + data.reserveCapacity(buffer->byteLength()); > + data.append(static_cast<const char*>(buffer->data()), buffer->byteLength()); reserveInitialCapacity, uncheckedAppend But also, I think we can just construct with the initial values more directly using the Vector constructor that takes two pointers. > Source/WebCore/Modules/fetch/FetchBody.cpp:315 > + // FIXME:: Support FormData. Extra colon here. Also not sure we need the FIXME at all. > Source/WebCore/Modules/fetch/FetchResponse.cpp:163 > + response->m_bodyLoader = BodyLoader(response.get(), WTFMove(promise)); Can we use initializer list syntax instead here? response->m_bodyLoader = { response.get(), WTFMove(promise) }; > Source/WebCore/Modules/fetch/FetchResponse.cpp:184 > +FetchResponse::BodyLoader::BodyLoader(FetchResponse& response, FetchPromise&& promise) Extra space here after the &&. > Source/WebCore/Modules/fetch/FetchResponse.cpp:208 > + m_loader = std::make_unique<FetchLoader>(FetchLoader::Type::ArrayBuffer, *this); > + ASSERT(m_loader); This assertion isn’t needed. make_unique never returns null by definition; in WebKit it crashes the process if it fails to allocate, in other C++ context it throws an exception, but never null > Source/WebCore/Modules/fetch/FetchResponse.h:99 > + class BodyLoader final : public FetchLoaderClient { > + public: > + BodyLoader(FetchResponse&, FetchPromise&&); > + > + void start(ScriptExecutionContext&, const FetchRequest&); > + void stop(); > + > + private: > + // FetchLoaderClient API > + void didFail() final; > + void didSucceed() final; > + void didFinishLoadingAsArrayBuffer(RefPtr<ArrayBuffer>&&) final; > + void didReceiveResponse(const ResourceResponse&); > + > + FetchResponse& m_response; > + Optional<FetchPromise> m_promise; > + std::unique_ptr<FetchLoader> m_loader; > + }; Do we need this class definition in the header? Try just forward declaring in the header and moving the definition to the cpp file. Should work. > Source/WebCore/platform/network/BlobResourceHandle.cpp:585 > + response.setHTTPHeaderField(HTTPHeaderName::ContentLength, String::format("%lld", m_totalRemainingSize)); String::number(m_totalRemainingSize) is the right way to do this; String::format should be deprecated, and is especially wrong when we just want to do a single number like this.

(In reply to comment #10) > > > Source/WebCore/Modules/fetch/FetchResponse.h:99 > > > + FetchResponse& response; > > > + FetchManager& manager; > > > + FetchPromise promise; > > > + std::unique_ptr<FetchLoader> loader; > > > > Please use the m_ prefix for member variable names. It makes it easier to > > see where things are coming from. > > I also prefer using m_prefixes, but I was thinking WebKit style for struct > was omitting the m_ prefix. The general guideline is that we use the "m_" prefix for private data members, and we also prefer to make all data members private and use accessor functions if we have to make things public. The exception is for a struct where we intentionally use public data members as the interface to the struct. In that case we prefer not to use the "m_" prefix.

Created attachment 274834 [details] Patch for landing

Thanks for the review. More comments inline. (In reply to comment #16) > Comment on attachment 274654 [details] > Moving to ActiveDOMObject > > View in context: > https://bugs.webkit.org/attachment.cgi?id=274654&action=review > > > Source/WebCore/Modules/fetch/DOMWindowFetch.cpp:40 > > +void DOMWindowFetch::fetch(DOMWindow& window, FetchRequest* request, const Dictionary& dictionary, DeferredWrapper&& promise) > > I don’t think the name “request” here is great. It’s like an “underlying” > request? It’s strange that this fetch request gets wrapped in another. Is > that really right? This is step 2 of https://fetch.spec.whatwg.org/#fetch-method. The FetchRequest constructor can take a FetchRequest as input, which content is copied in the newly created FetchRequest. Let's change the name to "input" then. > > > Source/WebCore/Modules/fetch/DOMWindowFetch.cpp:48 > > + if (ec || !fetchRequest) { > > Is it right to check both things here? Is it really possible to have a > failure with exception code zero? What does that mean exactly? Is it > possible to have a failure that returns non-null? I think we should only be > checking one of these two things. Right, let's check "ec" and put an ASSERT on fetchRequest. > > Source/WebCore/Modules/fetch/DOMWindowFetch.h:44 > > + static void fetch(DOMWindow&, FetchRequest*, const Dictionary&, DeferredWrapper&&); > > How can we change this FetchRequest* into a FetchRequest&? At the moment, if > we pass a null in here it will crash later. Are we using a pointer here > because of some limitation of the bindings generator? Yes, the binding generator should probably be improved to pass a FetchRequest& but it currently passes a FetchRequest*. There is an assert in FetchRequest::create() to ensure that the FetchRequest pointer is not null. > > Source/WebCore/Modules/fetch/FetchBody.cpp:45 > > +static inline RefPtr<Blob> blobFromArrayBuffer(const RefPtr<ArrayBuffer>&, const String&); > > Putting “inline” here has no effect. Please don’t. > > Argument type should be ArrayBuffer* or ArrayBuffer&. Almost never helpful > to pass a const RefPtr& to something. OK. > > Source/WebCore/Modules/fetch/FetchBody.cpp:175 > > + m_consumer = Consumer({type, WTFMove(promise)}); > > Syntax here seems peculiar. Normally we use space between braces. Also > strange that we need to specific a type explicitly if using argument list > syntax. This is a current limitation of WTF::Optional. I am not sure how WTF::Optional could be improved with that respect. > > > Source/WebCore/Modules/fetch/FetchBody.cpp:195 > > + fulfillTextPromise(type, TextResourceDecoder::create(ASCIILiteral("text/plain"), "UTF-8")->decodeAndFlush(static_cast<const char*>(m_data->data()), m_data->byteLength()), promise); > > Is there no more efficient way to properly decode UTF-8 than creating a text > resource decoder with these string arguments? We should investigate that. > > > Source/WebCore/Modules/fetch/FetchBody.cpp:252 > > + data.reserveCapacity(buffer->byteLength()); > > + data.append(static_cast<const char*>(buffer->data()), buffer->byteLength()); > > reserveInitialCapacity, uncheckedAppend > But also, I think we can just construct with the initial values more > directly using the Vector constructor that takes two pointers. I switched to Vector(size_t) followed by a memcpy. > > Source/WebCore/Modules/fetch/FetchBody.cpp:315 > > + // FIXME:: Support FormData. > > Extra colon here. Also not sure we need the FIXME at all. Removed. > > Source/WebCore/Modules/fetch/FetchResponse.cpp:163 > > + response->m_bodyLoader = BodyLoader(response.get(), WTFMove(promise)); > > Can we use initializer list syntax instead here? > > response->m_bodyLoader = { response.get(), WTFMove(promise) }; This is not working unfortunately. > > Source/WebCore/Modules/fetch/FetchResponse.cpp:184 > > +FetchResponse::BodyLoader::BodyLoader(FetchResponse& response, FetchPromise&& promise) > > Extra space here after the &&. > OK > > Source/WebCore/Modules/fetch/FetchResponse.cpp:208 > > + m_loader = std::make_unique<FetchLoader>(FetchLoader::Type::ArrayBuffer, *this); > > + ASSERT(m_loader); > > This assertion isn’t needed. make_unique never returns null by definition; > in WebKit it crashes the process if it fails to allocate, in other C++ > context it throws an exception, but never null OK > > Source/WebCore/Modules/fetch/FetchResponse.h:99 > > + class BodyLoader final : public FetchLoaderClient { > > + public: > > + BodyLoader(FetchResponse&, FetchPromise&&); > > + > > + void start(ScriptExecutionContext&, const FetchRequest&); > > + void stop(); > > + > > + private: > > + // FetchLoaderClient API > > + void didFail() final; > > + void didSucceed() final; > > + void didFinishLoadingAsArrayBuffer(RefPtr<ArrayBuffer>&&) final; > > + void didReceiveResponse(const ResourceResponse&); > > + > > + FetchResponse& m_response; > > + Optional<FetchPromise> m_promise; > > + std::unique_ptr<FetchLoader> m_loader; > > + }; > > Do we need this class definition in the header? Try just forward declaring > in the header and moving the definition to the cpp file. Should work. BodyLoader is a WTF::Optional which requires to know FetchPromise, at least its size I think. > > > Source/WebCore/platform/network/BlobResourceHandle.cpp:585 > > + response.setHTTPHeaderField(HTTPHeaderName::ContentLength, String::format("%lld", m_totalRemainingSize)); > > String::number(m_totalRemainingSize) is the right way to do this; > String::format should be deprecated, and is especially wrong when we just > want to do a single number like this. OK.

Comment on attachment 274834 [details] Patch for landing Clearing flags on attachment: 274834 Committed r198627: <http://trac.webkit.org/changeset/198627>

All reviewed patches have been landed. Closing bug.

This has caused use-after-free crashes, rolling out for now. imported/w3c/web-platform-tests/fetch/api/basic/integrity.html imported/w3c/web-platform-tests/fetch/api/basic/mode-no-cors.html imported/w3c/web-platform-tests/fetch/api/basic/mode-same-origin.html imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.html imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob.html imported/w3c/web-platform-tests/fetch/api/basic/scheme-data.html imported/w3c/web-platform-tests/fetch/api/basic/scheme-others.html ==32639==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000b23068 at pc 0x0001118fd8a4 bp 0x7fff53ade5a0 sp 0x7fff53ade598 READ of size 8 at 0x604000b23068 thread T0 #0 0x1118fd8a3 in WTF::RefPtr<WebCore::ThreadableLoader>::swap(WTF::RefPtr<WebCore::ThreadableLoader>&) (WebCore.framework/Versions/A/WebCore+0xcde8a3) #1 0x1118fc3c5 in WTF::RefPtr<WebCore::ThreadableLoader>::operator=(WTF::RefPtr<WebCore::ThreadableLoader>&&) (WebCore.framework/Versions/A/WebCore+0xcdd3c5) #2 0x113333c5c in WebCore::FetchLoader::start(WebCore::ScriptExecutionContext&, WebCore::FetchRequest const&) (WebCore.framework/Versions/A/WebCore+0x2714c5c) #3 0x1119389e0 in WebCore::FetchResponse::BodyLoader::start(WebCore::ScriptExecutionContext&, WebCore::FetchRequest const&) (WebCore.framework/Versions/A/WebCore+0xd199e0) #4 0x111938715 in WebCore::FetchResponse::fetch(WebCore::ScriptExecutionContext&, WebCore::FetchRequest const&, WebCore::DOMPromise<WTF::RefPtr<WebCore::FetchResponse>, int>&&) (WebCore.framework/Versions/A/WebCore+0xd19715) #5 0x1111cd097 in WebCore::DOMWindowFetch::fetch(WebCore::DOMWindow&, WTF::String const&, WebCore::Dictionary const&, WebCore::DeferredWrapper&&) (WebCore.framework/Versions/A/WebCore+0x5ae097) #6 0x1121353e7 in WebCore::jsDOMWindowInstanceFunctionFetch1Promise(JSC::ExecState*, JSC::JSPromiseDeferred*) (WebCore.framework/Versions/A/WebCore+0x15163e7) #7 0x111f380cf in WebCore::callPromiseFunction(JSC::ExecState&, long long (*)(JSC::ExecState*, JSC::JSPromiseDeferred*)) (WebCore.framework/Versions/A/WebCore+0x13190cf) #8 0x11213519f in WebCore::jsDOMWindowInstanceFunctionFetch1(JSC::ExecState*) (WebCore.framework/Versions/A/WebCore+0x151619f) #9 0x112132dc2 in WebCore::jsDOMWindowInstanceFunctionFetch(JSC::ExecState*) (WebCore.framework/Versions/A/WebCore+0x1513dc2) #10 0x3fb5fbc01027 (<unknown module>) #11 0x10fa9f91a in llint_entry (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x10c491a) #12 0x10fa9fd86 in llint_entry (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x10c4d86) #13 0x10fa9f91a in llint_entry (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x10c491a) #14 0x3fb5fbcc5d7b (<unknown module>) #15 0x10fa99c2a in vmEntryToJavaScript (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x10bec2a) #16 0x10f8067fd in JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xe2b7fd) #17 0x10ea43594 in JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x68594) #18 0x10ea431f1 in JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x681f1) #19 0x10f04a85a in JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x66f85a) #20 0x10f8f05f1 in JSC::JSJobMicrotask::run(JSC::ExecState*) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xf155f1) #21 0x11205b83d in WebCore::JSMainThreadExecState::runTask(JSC::ExecState*, JSC::Microtask&) (WebCore.framework/Versions/A/WebCore+0x143c83d) #22 0x112153cf5 in WebCore::JSDOMWindowMicrotaskCallback::call() (WebCore.framework/Versions/A/WebCore+0x1534cf5) #23 0x1110fb568 in WebCore::ActiveDOMCallbackMicrotask::run() (WebCore.framework/Versions/A/WebCore+0x4dc568) #24 0x112936805 in WebCore::MicrotaskQueue::performMicrotaskCheckpoint() (WebCore.framework/Versions/A/WebCore+0x1d17805) #25 0x110c3f484 in WebCore::ThreadTimers::sharedTimerFiredInternal() (WebCore.framework/Versions/A/WebCore+0x20484) #26 0x110c3f29f in WebCore::timerFired(__CFRunLoopTimer*, void*) (WebCore.framework/Versions/A/WebCore+0x2029f) ... 0x604000b23068 is located 24 bytes inside of 40-byte region [0x604000b23050,0x604000b23078) freed by thread T0 here: #0 0x10d773109 in wrap_free (/Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.11.xctoolchain/usr/lib/clang/7.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib+0x43109) #1 0x10fe3e950 in bmalloc::Deallocator::deallocateSlowCase(void*) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x1463950) #2 0x11193a249 in WebCore::FetchResponse::BodyLoader::~BodyLoader() (WebCore.framework/Versions/A/WebCore+0xd1b249) #3 0x11193a399 in WTF::Optional<WebCore::FetchResponse::BodyLoader>::destroy() (WebCore.framework/Versions/A/WebCore+0xd1b399) #4 0x11193968d in WTF::Optional<WebCore::FetchResponse::BodyLoader>::operator=(WTF::NulloptTag) (WebCore.framework/Versions/A/WebCore+0xd1a68d) #5 0x111938c60 in WebCore::FetchResponse::BodyLoader::didFail() (WebCore.framework/Versions/A/WebCore+0xd19c60) #6 0x11170b2b0 in WebCore::DocumentThreadableLoader::DocumentThreadableLoader(WebCore::Document&, WebCore::ThreadableLoaderClient&, WebCore::DocumentThreadableLoader::BlockingBehavior, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderOptions const&, std::__1::unique_ptr<WebCore::ContentSecurityPolicy, std::__1::default_delete<WebCore::ContentSecurityPolicy> >&&) (WebCore.framework/Versions/A/WebCore+0xaec2b0) #7 0x11170ac98 in WebCore::DocumentThreadableLoader::create(WebCore::Document&, WebCore::ThreadableLoaderClient&, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderOptions const&, std::__1::unique_ptr<WebCore::ContentSecurityPolicy, std::__1::default_delete<WebCore::ContentSecurityPolicy> >&&) (WebCore.framework/Versions/A/WebCore+0xaebc98) #8 0x11170ae87 in WebCore::DocumentThreadableLoader::create(WebCore::Document&, WebCore::ThreadableLoaderClient&, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderOptions const&) (WebCore.framework/Versions/A/WebCore+0xaebe87) #9 0x110dcb89c in WebCore::ThreadableLoader::create(WebCore::ScriptExecutionContext*, WebCore::ThreadableLoaderClient*, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderOptions const&) (WebCore.framework/Versions/A/WebCore+0x1ac89c) #10 0x113333c50 in WebCore::FetchLoader::start(WebCore::ScriptExecutionContext&, WebCore::FetchRequest const&) (WebCore.framework/Versions/A/WebCore+0x2714c50) #11 0x1119389e0 in WebCore::FetchResponse::BodyLoader::start(WebCore::ScriptExecutionContext&, WebCore::FetchRequest const&) (WebCore.framework/Versions/A/WebCore+0xd199e0) #12 0x111938715 in WebCore::FetchResponse::fetch(WebCore::ScriptExecutionContext&, WebCore::FetchRequest const&, WebCore::DOMPromise<WTF::RefPtr<WebCore::FetchResponse>, int>&&) (WebCore.framework/Versions/A/WebCore+0xd19715) #13 0x1111cd097 in WebCore::DOMWindowFetch::fetch(WebCore::DOMWindow&, WTF::String const&, WebCore::Dictionary const&, WebCore::DeferredWrapper&&) (WebCore.framework/Versions/A/WebCore+0x5ae097) #14 0x1121353e7 in WebCore::jsDOMWindowInstanceFunctionFetch1Promise(JSC::ExecState*, JSC::JSPromiseDeferred*) (WebCore.framework/Versions/A/WebCore+0x15163e7) #15 0x111f380cf in WebCore::callPromiseFunction(JSC::ExecState&, long long (*)(JSC::ExecState*, JSC::JSPromiseDeferred*)) (WebCore.framework/Versions/A/WebCore+0x13190cf) #16 0x11213519f in WebCore::jsDOMWindowInstanceFunctionFetch1(JSC::ExecState*) (WebCore.framework/Versions/A/WebCore+0x151619f) #17 0x112132dc2 in WebCore::jsDOMWindowInstanceFunctionFetch(JSC::ExecState*) (WebCore.framework/Versions/A/WebCore+0x1513dc2) #18 0x3fb5fbc01027 (<unknown module>) #19 0x10fa9f91a in llint_entry (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x10c491a) #20 0x10fa9fd86 in llint_entry (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x10c4d86) #21 0x10fa9f91a in llint_entry (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x10c491a) #22 0x3fb5fbcc5d7b (<unknown module>) #23 0x10fa99c2a in vmEntryToJavaScript (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x10bec2a) #24 0x10f8067fd in JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xe2b7fd) #25 0x10ea43594 in JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x68594) #26 0x10ea431f1 in JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x681f1) #27 0x10f04a85a in JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x66f85a) #28 0x10f8f05f1 in JSC::JSJobMicrotask::run(JSC::ExecState*) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xf155f1) #29 0x11205b83d in WebCore::JSMainThreadExecState::runTask(JSC::ExecState*, JSC::Microtask&) (WebCore.framework/Versions/A/WebCore+0x143c83d)

Re-opened since this is blocked by bug 155856

@ap, thanks for rolling out the patch. The issue is probably due to the loader client (FetchResponse::BodyLoader) being deallocated as part of didFail, didFail being sometimes called within ThreadableLoader::create(). I will update the patch accordingly.

Created attachment 274902 [details] Patch for landing

(In reply to comment #25) > Created attachment 274902 [details] > Patch for landing Patch is updated accordingly. If clients of FetchLoader can be freed when didFail is called, they should make sure to ensure that freeing in didFail is not happening if the ThreadableLoader is null.

Comment on attachment 274902 [details] Patch for landing Clearing flags on attachment: 274902 Committed r198665: <http://trac.webkit.org/changeset/198665>

All reviewed patches have been landed. Closing bug.