Bug 161137 - [Fetch API] Ensure response cloning works when data is loading
Summary: [Fetch API] Ensure response cloning works when data is loading
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: 151937
  Show dependency treegraph
 
Reported: 2016-08-24 06:52 PDT by youenn fablet
Modified: 2016-08-28 23:51 PDT (History)
6 users (show)

See Also:


Attachments
Patch (12.79 KB, patch)
2016-08-24 06:59 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Improving test style (12.70 KB, patch)
2016-08-25 07:18 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (12.69 KB, patch)
2016-08-28 23:20 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-08-24 06:52:45 PDT
When data is loading, cloning is not working as FetchBody contains no data.
Comment 1 youenn fablet 2016-08-24 06:59:37 PDT
Created attachment 286852 [details]
Patch
Comment 2 youenn fablet 2016-08-25 07:18:54 PDT
Created attachment 286971 [details]
Improving test style
Comment 3 Darin Adler 2016-08-27 17:12:37 PDT
Comment on attachment 286971 [details]
Improving test style

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

> Source/WebCore/Modules/fetch/FetchResponse.cpp:140
>      if (m_response.m_readableStreamSource) {
> -        m_response.m_readableStreamSource->close();
> -        m_response.m_readableStreamSource = nullptr;
> +        // Let's close the stream except if we have data to enqueue.
> +        if (m_response.m_body.type() != FetchBody::Type::Loaded) {
> +            m_response.m_readableStreamSource->close();
> +            m_response.m_readableStreamSource = nullptr;
> +        }
>      }

In the case where we don’t close the stream here, what closes the stream?

Why not use && instead of nested if statements?
Comment 4 youenn fablet 2016-08-28 23:18:11 PDT
(In reply to comment #3)
> Comment on attachment 286971 [details]
> Improving test style
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=286971&action=review
> 
> > Source/WebCore/Modules/fetch/FetchResponse.cpp:140
> >      if (m_response.m_readableStreamSource) {
> > -        m_response.m_readableStreamSource->close();
> > -        m_response.m_readableStreamSource = nullptr;
> > +        // Let's close the stream except if we have data to enqueue.
> > +        if (m_response.m_body.type() != FetchBody::Type::Loaded) {
> > +            m_response.m_readableStreamSource->close();
> > +            m_response.m_readableStreamSource = nullptr;
> > +        }
> >      }
> 
> In the case where we don’t close the stream here, what closes the stream?

When type is Loaded, FetchBody has some data, which means the data was not enqueued in the stream. On the first read request, FetchBody::consumeAsStream will enqueue the data and close the stream.
I'll update the comment.

Overall, there is some complexity added by the idea of enqueuing data only on the first read call. Maybe we should simplify this and enqueue data as soon as the stream is created.

> Why not use && instead of nested if statements?

Right, I'll fix that.
Comment 5 youenn fablet 2016-08-28 23:20:35 PDT
Created attachment 287249 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2016-08-28 23:51:32 PDT
Comment on attachment 287249 [details]
Patch for landing

Clearing flags on attachment: 287249

Committed r205110: <http://trac.webkit.org/changeset/205110>
Comment 7 WebKit Commit Bot 2016-08-28 23:51:36 PDT
All reviewed patches have been landed.  Closing bug.