Bug 155490 – [Fetch API] response-consume.html is crashing on Mac WK1 Debug builds

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

As shown in https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK1%20(Tests)/r198134%20(11545)/results.html, response-consume.html is often crashing.
This can be reproduced locally.

Crash log is:
ASSERTION FAILED: m_heap->vm()->currentThreadIsHoldingAPILock()
/Users/youenn/Documents/WebKit/Source/JavaScriptCore/heap/MarkedAllocator.cpp(146) : void *JSC::MarkedAllocator::allocateSlowCase(size_t)
1   0x10306b490 WTFCrash
2   0x102ca4ac9 JSC::MarkedAllocator::allocateSlowCase(unsigned long)
3   0x102122bea JSC::MarkedAllocator::allocate(unsigned long)
4   0x10220e839 JSC::MarkedSpace::allocateWithDestructor(unsigned long)
5   0x10220e7fc JSC::Heap::allocateWithDestructor(unsigned long)
6   0x102217e6a void* JSC::Heap::allocateObjectOfType<JSC::JSString>(unsigned long)
7   0x102217dc4 void* JSC::allocateCell<JSC::JSString>(JSC::Heap&, unsigned long)
8   0x102217b1c void* JSC::allocateCell<JSC::JSString>(JSC::Heap&)
9   0x1022177d0 JSC::JSString::create(JSC::VM&, WTF::PassRefPtr<WTF::StringImpl>)
10  0x102217496 JSC::jsString(JSC::VM*, WTF::String const&)
11  0x10240cc55 JSC::jsString(JSC::ExecState*, WTF::String const&)
12  0x102c756ee JSC::LiteralParser<unsigned char>::parse(JSC::ParserState)
13  0x102a46b87 JSC::LiteralParser<unsigned char>::tryLiteralParse()
14  0x102bcd6bd JSC::JSONParse(JSC::ExecState*, WTF::String const&)
15  0x108413220 WebCore::FetchBody::resolveAsJSON(WebCore::ScriptExecutionContext&, WTF::String const&, WebCore::DeferredWrapper&&)
16  0x108413b74 WebCore::FetchBody::loadedAsText(WebCore::ScriptExecutionContext&, WTF::String&&)
17  0x108f62150 WebCore::FetchBodyOwner::loadedBlobAsText(WTF::String&&)
18  0x108f62704 WebCore::FetchBodyOwner::BlobLoader::didFinishLoadingAsText(WTF::String&&)
19  0x109f48c07 WebCore::FetchLoader::didFinishLoading(unsigned long, double)
...

[youenn fablet]

Created attachment 274087 [details]
Patch

[Alex Christensen]

Comment on attachment 274087 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchBody.cpp:214
> +    DOMRequestState::Scope scope(state);

This locks state.exec().  The other use of DOMRequestState in deserializeIDBValueDataToJSValue calls exec.vm().apiLock().lock() and exec.vm().apiLock().unlock() manually.  That sounds like it would fix the assertion better than this.

[youenn fablet]

(In reply to comment #2)
> Comment on attachment 274087 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274087&action=review
> 
> > Source/WebCore/Modules/fetch/FetchBody.cpp:214
> > +    DOMRequestState::Scope scope(state);
> 
> This locks state.exec().  The other use of DOMRequestState in
> deserializeIDBValueDataToJSValue calls exec.vm().apiLock().lock() and
> exec.vm().apiLock().unlock() manually.  That sounds like it would fix the
> assertion better than this.

I am not sure to see the difference between the two approaches.
Can you elaborate?

[Alex Christensen]

Comment on attachment 274087 [details]
Patch

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

>>> Source/WebCore/Modules/fetch/FetchBody.cpp:214
>>> +    DOMRequestState::Scope scope(state);
>> 
>> This locks state.exec().  The other use of DOMRequestState in deserializeIDBValueDataToJSValue calls exec.vm().apiLock().lock() and exec.vm().apiLock().unlock() manually.  That sounds like it would fix the assertion better than this.
> 
> I am not sure to see the difference between the two approaches.
> Can you elaborate?

I was thinking a JSLockHolder locked something else, but looking into JSLockHolder::init I see these are indeed the same thing.

> Source/WebCore/Modules/fetch/FetchBody.cpp:217
>      if (!value)
>          promise.reject<ExceptionCode>(SYNTAX_ERR);

Don't we want to release the lock before this code?  I think scope needs its own scope.

[youenn fablet]

Created attachment 274188 [details]
Removing use of DOMRequestState

[youenn fablet]

> Don't we want to release the lock before this code?  I think scope needs its
> own scope.

Right, it improves the code a bit.

Looking at DOMRequestState, it seems DOMRequestState::Scope is used nowhere in WebKit. I migrated JSON parsing in a dedicated promise resolution routine.

[Alex Christensen]

(In reply to comment #6)
> Looking at DOMRequestState, it seems DOMRequestState::Scope is used nowhere
> in WebKit. 
Let's delete it, then.

[Darin Adler]

Comment on attachment 274188 [details]
Removing use of DOMRequestState

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

> Source/WebCore/Modules/fetch/FetchBody.cpp:249
> +void FetchBody::loadedAsText(String&& text)
>  {
>      ASSERT(m_consumer);
>      ASSERT(m_consumer->type == Consumer::Type::Text || m_consumer->type == Consumer::Type::JSON);
>      if (m_consumer->type == Consumer::Type::Text)
>          m_consumer->promise.resolve(text);
>      else
> -        resolveAsJSON(context, text, WTFMove(m_consumer->promise));
> +        fulfillPromiseWithJSON(m_consumer->promise, text);
>      m_consumer = Nullopt;
>  }

The value is passed in as an rvalue reference, but no code paths move it; they all seem to copy it instead. Do we need to add some calls to WTFMove here?

> Source/WebCore/bindings/js/JSDOMPromise.cpp:85
> +static inline JSC::JSValue parseAsJSON(JSC::ExecState* state, const String& data)
> +{
> +    JSC::JSLockHolder lock(state);
> +    return JSC::JSONParse(state, data);
> +}

I would have used a reference here instead of a pointer.

[youenn fablet]

(In reply to comment #8)
> Comment on attachment 274188 [details]
> Removing use of DOMRequestState
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274188&action=review
> 
> > Source/WebCore/Modules/fetch/FetchBody.cpp:249
> > +void FetchBody::loadedAsText(String&& text)
> >  {
> >      ASSERT(m_consumer);
> >      ASSERT(m_consumer->type == Consumer::Type::Text || m_consumer->type == Consumer::Type::JSON);
> >      if (m_consumer->type == Consumer::Type::Text)
> >          m_consumer->promise.resolve(text);
> >      else
> > -        resolveAsJSON(context, text, WTFMove(m_consumer->promise));
> > +        fulfillPromiseWithJSON(m_consumer->promise, text);
> >      m_consumer = Nullopt;
> >  }
> 
> The value is passed in as an rvalue reference, but no code paths move it;
> they all seem to copy it instead. Do we need to add some calls to WTFMove
> here?

Ideally promises should be able to take rvalues but they are not doing so.
loadedAsArrayBuffer is taking an rvalue as it will store the body data as ArrayBuffer if there is no pending promise.
loadedAsText takes an rvalue for consistency here, although other future clients may actually take ownership of text.

[WebKit Commit Bot]

Comment on attachment 274188 [details]
Removing use of DOMRequestState

Clearing flags on attachment: 274188

Committed r198326: <http://trac.webkit.org/changeset/198326>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.