Bug 155637 – [Fetch API] Add basic loading of resources

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

FetchLoader should be made capable of doing some basic fetch.

[youenn fablet]

Created attachment 274433 [details]
WIP

[youenn fablet]

Created attachment 274579 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

(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

[Alex Christensen]

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?

[Darin Adler]

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.

[Alex Christensen]

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?

[youenn fablet]

(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.

[youenn fablet]

(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.

[youenn fablet]

Created attachment 274653 [details]
Patch

[youenn fablet]

Created attachment 274654 [details]
Moving to ActiveDOMObject

[WebKit Commit Bot]

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.

[youenn fablet]

(In reply to comment #13)
> Created attachment 274654 [details]
> Moving to ActiveDOMObject

This patch removes FetchManager.cpp and adds some more tests.

[Darin Adler]

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.

[Darin Adler]

(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.

[youenn fablet]

Created attachment 274834 [details]
Patch for landing

[youenn fablet]

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.

[WebKit Commit Bot]

Comment on attachment 274834 [details]
Patch for landing

Clearing flags on attachment: 274834

Committed r198627: <http://trac.webkit.org/changeset/198627>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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)

[WebKit Commit Bot]

Re-opened since this is blocked by bug 155856

[youenn fablet]

@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.

[youenn fablet]

Created attachment 274902 [details]
Patch for landing

[youenn fablet]

(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.

[WebKit Commit Bot]

Comment on attachment 274902 [details]
Patch for landing

Clearing flags on attachment: 274902

Committed r198665: <http://trac.webkit.org/changeset/198665>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.