Bug 170849 – A blob created from a fetch response does not correctly get type set

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[David Alan Hjelle]

In Safari 10.1 on macOS 10.12.4 (and I believe on iOS as well), the following code shows an empty content-type, rather than 'text/html' as I would expect and as Chrome/Firefox do.

```
var output = document.querySelector('span');
fetch('https://enable-cors.org/').then(function(response) {
  return response.blob();
}).then(function(blob) {
  output.innerText = "'" + blob.type + "'";
}).catch(function(error) {
  output.innerText = 'Error: ' + error.message;
  console.log(error);
});
```

See a runnable version at https://jsbin.com/hubope/edit?js,output.

While this sounds like 161228, which is marked as fixed last year, it's a problem in the most recent version of Safari. 137647 sounds similar, too, but was about XHR and not the Fetch API.

[David Alan Hjelle]

In case anyone comes here with this problem, a workaround:

```
let new_blob = new Blob([blob], {type: 'application/pdf'});
```

Then the new_blob works fine, as it has the correct content-type.

[David Alan Hjelle]

(assuming, of course, you have the correct content-type…perhaps one can get this from the HTTP headers, but I didn't need this in my situation)

[Radar WebKit Bug Importer]

<rdar://problem/31646415>

[Ben Kelly]

Created attachment 308192 [details]
Response.blob() does not set the content-type based on the header value.

This should update the Response content-type after headers are received.  Its covered by a web-platform-test, but I'm not sure if there is something I need to do in webkit to indicate that the test works now.

Also, I apologize if I made this patch incorrectly.  Its my first time submitting here.

[youenn fablet]

> This should update the Response content-type after headers are received. 
> Its covered by a web-platform-test, but I'm not sure if there is something I
> need to do in webkit to indicate that the test works now.
> 
> Also, I apologize if I made this patch incorrectly.  Its my first time
> submitting here.

Welcome!

The patch is almost right, the ChangeLog looks great.
There is a problem with the file paths though, since you should target Source/WebCore/ChangeLog and Source/WebCore/Modules/fetch/FetchResponse.cpp, not ChangeLog and Modules/fetch/FetchResponse.cpp.

If you have a WebKit git clone, you might be able to use the command below, assuming that your changes and ChangeLog are all in the last commit:
Tools/Scripts/webkit-patch upload -g HEAD~1..HEAD
Or if you are in master and your changes are not committed:
Tools/Scripts/webkit-patch upload

http://w3c-test.org/fetch/api/response/response-consume.html should translate to LayoutTests/imported/w3c/web-platform-tests/fetch/api/response/response-consume.html.

You might need to update LayoutTests/imported/w3c/web-platform-tests/fetch/api/response/response-consume-expected.txt as a test might go from FAIL to PASS.

[Alex Christensen]

Created attachment 308267 [details]
Patch

[Alex Christensen]

I got the patch applying and updated our test results.  Thanks for helping us polish our fetch API implementation!

[WebKit Commit Bot]

Comment on attachment 308267 [details]
Patch

Clearing flags on attachment: 308267

Committed r215814: <http://trac.webkit.org/changeset/215814>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

Is [e-mail] the email address for David Alan Hjelle?

[David Alan Hjelle]

No, [e-mail] is not me.

[Ryan Haddad]

LayoutTest imported/w3c/web-platform-tests/fetch/api/response/response-consume.html is failing on ios-simulator after this change.

Is the test expected to pass there?

https://build.webkit.org/results/Apple%20iOS%2010%20Simulator%20Release%20WK2%20(Tests)/r215832%20(967)/results.html

[Ryan Haddad]

(In reply to Ryan Haddad from comment #12)
> LayoutTest
> imported/w3c/web-platform-tests/fetch/api/response/response-consume.html is
> failing on ios-simulator after this change.
> 
> Is the test expected to pass there?
> 
> https://build.webkit.org/results/
> Apple%20iOS%2010%20Simulator%20Release%20WK2%20(Tests)/r215832%20(967)/
> results.html

It appears that the test may be a flaky failure on macOS as well:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Ffetch%2Fapi%2Fresponse%2Fresponse-consume.html

[Ryan Haddad]

Reverted r215814 for reason:

The LayoutTest for this change is failing on ios-simulator and is flaky on macOS.

Committed r215842: <http://trac.webkit.org/changeset/215842>

[Ben Kelly]

Thanks for trying to land this.

I think the test should be stable.  It is essentially this (from upstream WPT repo):

    checkResponseBody(fetch("../resources/top.txt"), "top", checkBodyBlob, "from fetch to blob");

    function checkResponseBody(responsePromise, expectedBody, checkFunction, bodyTypes) {
      promise_test(function(test) {
        return responsePromise.then(function(response) {
          assert_false(response.bodyUsed, "bodyUsed is false at init");
          return checkFunction(test, response, expectedBody);
        });
      }, "Consume response's body: " + bodyTypes);
    }

    function checkBodyBlob(test, response, expectedBody, expectedType) {
      return response.blob().then(function(bodyAsBlob) {
        assert_equals(bodyAsBlob.type, expectedType || "text/plain", "Blob body type should be computed from the response Content-Type");
        var promise = new Promise( function (resolve, reject) {
          var reader = new FileReader();
          reader.onload = function(evt) {
            resolve(reader.result)
          };
          reader.onerror = function () {
            reject("Blob's reader failed");
          };
          reader.readAsText(bodyAsBlob);
        });
        return promise.then(function(body) {
          assert_equals(body, expectedBody, "Retrieve and verify response's body");
          assert_true(response.bodyUsed, "body as blob: bodyUsed turned true");
        });
      });
    }

For the type check this basically boils down to:

  fetch(url).then(r => r.blob()).then(b => assert_equals(b.type, 'text/plain'));

I don't think that should be racy.

I wonder if the errors occur if FetchResponse::BodyLoader::didReceiveResponse() is called after the Response.blob() call.  Perhaps there is a race where usually didReceiveResponse() completes before resolving the fetch promise, but not always.

[Ben Kelly]

I'll try to reproduce and investigate over the weekend if no one else gets to it before then.

[youenn fablet]

Response::blob() cannot be called before the promise is resolved. The patch looks good.

Looking at https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Ffetch%2Fapi%2Fresponse%2Fresponse-consume.html, it seems the iOS results are fine now. The Mac results are for the tests are flaky but are not specific to the updated subtest.

[Ben Kelly]

(In reply to Alex Christensen from comment #10)
> Is [e-mail] the email address for David Alan Hjelle?

Sorry for the confusion.  I missed this question before.  I went ahead and updated my account to a better name/email.  I'll upload the patch with these corrections as well.

I also couldn't reproduce any flakyness in these tests (and I can't see the dashboard for some reason).  Given comment 17 I don't know what the next step here is.

[Ben Kelly]

Created attachment 308707 [details]
Response.blob() does not set the content-type based on the header value.

[Build Bot]

Attachment 308707 [details] did not pass style-queue:


Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ben Kelly]

Comment on attachment 308707 [details]
Response.blob() does not set the content-type based on the header value.

Sorry, I messed up the export of the patch from git.

[Ben Kelly]

Created attachment 308710 [details]
Response.blob() does not set the content-type based on the header value.

[Build Bot]

Comment on attachment 308710 [details]
Response.blob() does not set the content-type based on the header value.

Attachment 308710 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3648225

New failing tests:
imported/w3c/web-platform-tests/fetch/api/response/response-consume.html

[Build Bot]

Created attachment 308711 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Ben Kelly]

Building ios-simulator locally to see if I can reproduce.

[Ben Kelly]

Ok, I can reliably reproduce the failures in the ios-simulator.  Let me see if I can figure what is going on...

[Ben Kelly]

Created attachment 309006 [details]
Patch

[Ben Kelly]

The failures were due to another issue similar to bug 171489.  The type was not getting set on the FetchBodyConsumer if we hit the consumeOnceLoadingFinished() path.  Apparently only the ios-simulator hits this path consistently.

[WebKit Commit Bot]

Comment on attachment 309006 [details]
Patch

Rejecting attachment 309006 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 309006, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/LayoutTests/imported/w3c/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/3682090

[Ben Kelly]

Ah, sorry.  I hand-edited the Changelog and used "Oops" instead of "OOPS".  I guess this threw off the autoland scripts.

I'll upload the patch again.

[Ben Kelly]

Created attachment 309263 [details]
Patch

[Build Bot]

Comment on attachment 309263 [details]
Patch

Attachment 309263 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3683396

New failing tests:
js/dom/global-constructors-attributes-dedicated-worker.html
imported/w3c/web-platform-tests/fetch/api/basic/request-headers.any.worker.html

[Build Bot]

Created attachment 309265 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309263 [details]
Patch

Attachment 309263 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3683457

New failing tests:
js/dom/global-constructors-attributes-dedicated-worker.html
imported/w3c/web-platform-tests/fetch/api/basic/request-headers.any.worker.html

[Build Bot]

Created attachment 309266 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309263 [details]
Patch

Attachment 309263 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3683475

New failing tests:
js/dom/global-constructors-attributes-dedicated-worker.html
imported/w3c/web-platform-tests/fetch/api/basic/request-headers.any.worker.html

[Build Bot]

Created attachment 309267 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309263 [details]
Patch

Attachment 309263 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3683513

New failing tests:
js/dom/global-constructors-attributes-dedicated-worker.html
imported/w3c/web-platform-tests/fetch/api/basic/request-headers.any.worker.html

[Build Bot]

Created attachment 309269 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Ben Kelly]

Ugh, sorry again.  Apparently I'm exploring every way to screw up a patch in this bug.

This time I ran `webkit-patch upload` while I had an unrelated change in my working area.  I'll try again with --git_commit.

[Ben Kelly]

Created attachment 309283 [details]
Patch

[youenn fablet]

"webkit-patch upload -g" is nice.
--no-review can be useful as well if one wants to upload a WIP or just test a patch against the bots.

[WebKit Commit Bot]

Comment on attachment 309283 [details]
Patch

Rejecting attachment 309283 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 309283, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/LayoutTests/imported/w3c/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/3688541

[Ben Kelly]

I'm an idiot.  The fix to the changelog was not in the commit.  I'm really sorry.  Still figuring out my workflow here.

[Ben Kelly]

Created attachment 309285 [details]
Patch

[youenn fablet]

change logs are a bit painful at first...
https://trac.webkit.org/wiki/UsingGitWithWebKit has some useful information.

[Alexey Proskuryakov]

The workflow I recommend is to ignore webkit-patch completely, and to generate patches with svn-create-patch (and to use svn-apply/svn-unapply). With direct manipulation like this, it's much easier to verify each step.

[Ben Kelly]

Comment on attachment 309285 [details]
Patch

Ok.  EWS is green.  I double checked the "Reviewed by NOBODY (OOPS!)" lines.  I think this should finally be good to review and land.

[youenn fablet]

(In reply to Alexey Proskuryakov from comment #47)
> The workflow I recommend is to ignore webkit-patch completely, and to
> generate patches with svn-create-patch (and to use svn-apply/svn-unapply).
> With direct manipulation like this, it's much easier to verify each step.

We might want to go away from svn at some point. If there is anything suboptimal in WebKit-patch, we should try to fix it.

[WebKit Commit Bot]

Comment on attachment 309285 [details]
Patch

Clearing flags on attachment: 309285

Committed r216353: <http://trac.webkit.org/changeset/216353>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> We might want to go away from svn at some point. If there is anything suboptimal in WebKit-patch, we should try to fix it.

The scripts I mentioned work with git-svn and pure git checkouts just as well (perhaps even better than with svn, as there are a few incompatibilities with modern svn versions that are yet to be fixed).

I'm not sure if there is anything to fix with webkit-patch. What I'm saying is that it makes patch operations more opaque and thus harder to get right, but combining several steps together is the whole reason for its existence.

[youenn fablet]

> The scripts I mentioned work with git-svn and pure git checkouts just as
> well (perhaps even better than with svn, as there are a few
> incompatibilities with modern svn versions that are yet to be fixed).

Interesting, I'll give it a try!
Maybe we should rename these scripts.

[Josh Habdas]

FYI - https://bugs.chromium.org/p/chromium/issues/detail?id=724433

Please let me know if you'd like a new bug.

[youenn fablet]

(In reply to Josh Habdas from comment #54)
> FYI - https://bugs.chromium.org/p/chromium/issues/detail?id=724433
> 
> Please let me know if you'd like a new bug.

A new bug would be good.
Clarification by the fetch spec and related tests in W3C web-platform-tests would be even better. Maybe the latter would trigger the former.

[Ben Kelly]

We probably need consensus on this spec issue:

https://github.com/whatwg/fetch/issues/540