Bug 155490 - [Fetch API] response-consume.html is crashing on Mac WK1 Debug builds
Summary: [Fetch API] response-consume.html is crashing on Mac WK1 Debug builds
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-15 03:57 PDT by youenn fablet
Modified: 2016-03-17 04:25 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.04 KB, patch)
2016-03-15 04:02 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Removing use of DOMRequestState (8.09 KB, patch)
2016-03-16 07:24 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-03-15 03:57:09 PDT
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)
...
Comment 1 youenn fablet 2016-03-15 04:02:29 PDT
Created attachment 274087 [details]
Patch
Comment 2 Alex Christensen 2016-03-15 09:39:02 PDT
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.
Comment 3 youenn fablet 2016-03-15 10:36:14 PDT
(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?
Comment 4 Alex Christensen 2016-03-15 10:49:46 PDT
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.
Comment 5 youenn fablet 2016-03-16 07:24:11 PDT
Created attachment 274188 [details]
Removing use of DOMRequestState
Comment 6 youenn fablet 2016-03-16 07:31:26 PDT
> 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.
Comment 7 Alex Christensen 2016-03-16 10:43:51 PDT
(In reply to comment #6)
> Looking at DOMRequestState, it seems DOMRequestState::Scope is used nowhere
> in WebKit. 
Let's delete it, then.
Comment 8 Darin Adler 2016-03-16 15:20:47 PDT
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.
Comment 9 youenn fablet 2016-03-17 03:41:28 PDT
(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.
Comment 10 WebKit Commit Bot 2016-03-17 04:24:56 PDT
Comment on attachment 274188 [details]
Removing use of DOMRequestState

Clearing flags on attachment: 274188

Committed r198326: <http://trac.webkit.org/changeset/198326>
Comment 11 WebKit Commit Bot 2016-03-17 04:25:01 PDT
All reviewed patches have been landed.  Closing bug.