Bug 170849 - A blob created from a fetch response does not correctly get type set
Summary: A blob created from a fetch response does not correctly get type set
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: Safari 10
Hardware: Mac macOS 10.12
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-14 09:22 PDT by David Alan Hjelle
Modified: 2017-05-22 07:46 PDT (History)
10 users (show)

See Also:


Attachments
Response.blob() does not set the content-type based on the header value. (1.80 KB, patch)
2017-04-25 19:41 PDT, Ben Kelly
no flags Details | Formatted Diff | Diff
Patch (4.14 KB, patch)
2017-04-26 10:50 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Response.blob() does not set the content-type based on the header value. (4.04 KB, patch)
2017-04-30 19:20 PDT, Ben Kelly
no flags Details | Formatted Diff | Diff
Response.blob() does not set the content-type based on the header value. (4.17 KB, patch)
2017-04-30 19:49 PDT, Ben Kelly
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (839.48 KB, application/zip)
2017-04-30 21:25 PDT, Build Bot
no flags Details
Patch (7.57 KB, patch)
2017-05-03 20:18 PDT, Ben Kelly
no flags Details | Formatted Diff | Diff
Patch (8.13 KB, patch)
2017-05-05 20:25 PDT, Ben Kelly
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (788.22 KB, application/zip)
2017-05-05 21:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (911.33 KB, application/zip)
2017-05-05 21:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.84 MB, application/zip)
2017-05-05 21:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (750.47 KB, application/zip)
2017-05-05 22:06 PDT, Build Bot
no flags Details
Patch (7.59 KB, patch)
2017-05-06 10:05 PDT, Ben Kelly
no flags Details | Formatted Diff | Diff
Patch (7.59 KB, patch)
2017-05-06 10:39 PDT, Ben Kelly
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Alan Hjelle 2017-04-14 09:22:01 PDT
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.
Comment 1 David Alan Hjelle 2017-04-14 10:25:01 PDT
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.
Comment 2 David Alan Hjelle 2017-04-14 10:41:26 PDT
(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)
Comment 3 Radar WebKit Bug Importer 2017-04-15 17:28:40 PDT
<rdar://problem/31646415>
Comment 4 Ben Kelly 2017-04-25 19:41:58 PDT
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.
Comment 5 youenn fablet 2017-04-25 20:23:41 PDT
> 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.
Comment 6 Alex Christensen 2017-04-26 10:50:49 PDT
Created attachment 308267 [details]
Patch
Comment 7 Alex Christensen 2017-04-26 10:54:49 PDT
I got the patch applying and updated our test results.  Thanks for helping us polish our fetch API implementation!
Comment 8 WebKit Commit Bot 2017-04-26 11:14:44 PDT
Comment on attachment 308267 [details]
Patch

Clearing flags on attachment: 308267

Committed r215814: <http://trac.webkit.org/changeset/215814>
Comment 9 WebKit Commit Bot 2017-04-26 11:14:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Alex Christensen 2017-04-26 11:21:30 PDT
Is webkitcontrib@gmail.com the email address for David Alan Hjelle?
Comment 11 David Alan Hjelle 2017-04-26 12:03:10 PDT
No, webkitcontrib@gmail.com is not me.
Comment 12 Ryan Haddad 2017-04-26 16:26:24 PDT
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
Comment 13 Ryan Haddad 2017-04-26 16:30:22 PDT
(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
Comment 14 Ryan Haddad 2017-04-26 16:52:24 PDT
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>
Comment 15 Ben Kelly 2017-04-27 19:18:48 PDT
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.
Comment 16 Ben Kelly 2017-04-27 20:36:22 PDT
I'll try to reproduce and investigate over the weekend if no one else gets to it before then.
Comment 17 youenn fablet 2017-04-27 21:17:41 PDT
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.
Comment 18 Ben Kelly 2017-04-30 19:17:59 PDT
(In reply to Alex Christensen from comment #10)
> Is webkitcontrib@gmail.com 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.
Comment 19 Ben Kelly 2017-04-30 19:20:50 PDT
Created attachment 308707 [details]
Response.blob() does not set the content-type based on the header value.
Comment 20 Build Bot 2017-04-30 19:23:01 PDT
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.
Comment 21 Ben Kelly 2017-04-30 19:45:56 PDT
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.
Comment 22 Ben Kelly 2017-04-30 19:49:00 PDT
Created attachment 308710 [details]
Response.blob() does not set the content-type based on the header value.
Comment 23 Build Bot 2017-04-30 21:25:35 PDT
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
Comment 24 Build Bot 2017-04-30 21:25:37 PDT
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
Comment 25 Ben Kelly 2017-05-02 18:44:41 PDT
Building ios-simulator locally to see if I can reproduce.
Comment 26 Ben Kelly 2017-05-03 17:35:01 PDT
Ok, I can reliably reproduce the failures in the ios-simulator.  Let me see if I can figure what is going on...
Comment 27 Ben Kelly 2017-05-03 20:18:52 PDT
Created attachment 309006 [details]
Patch
Comment 28 Ben Kelly 2017-05-03 20:21:29 PDT
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.
Comment 29 WebKit Commit Bot 2017-05-05 17:09:29 PDT
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
Comment 30 Ben Kelly 2017-05-05 20:23:09 PDT
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.
Comment 31 Ben Kelly 2017-05-05 20:25:04 PDT
Created attachment 309263 [details]
Patch
Comment 32 Build Bot 2017-05-05 21:33:14 PDT
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
Comment 33 Build Bot 2017-05-05 21:33:15 PDT
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
Comment 34 Build Bot 2017-05-05 21:40:56 PDT
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
Comment 35 Build Bot 2017-05-05 21:40:57 PDT
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
Comment 36 Build Bot 2017-05-05 21:56:56 PDT
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
Comment 37 Build Bot 2017-05-05 21:56:58 PDT
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
Comment 38 Build Bot 2017-05-05 22:06:06 PDT
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
Comment 39 Build Bot 2017-05-05 22:06:07 PDT
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
Comment 40 Ben Kelly 2017-05-06 10:04:24 PDT
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.
Comment 41 Ben Kelly 2017-05-06 10:05:43 PDT
Created attachment 309283 [details]
Patch
Comment 42 youenn fablet 2017-05-06 10:31:19 PDT
"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.
Comment 43 WebKit Commit Bot 2017-05-06 10:33:11 PDT
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
Comment 44 Ben Kelly 2017-05-06 10:36:55 PDT
I'm an idiot.  The fix to the changelog was not in the commit.  I'm really sorry.  Still figuring out my workflow here.
Comment 45 Ben Kelly 2017-05-06 10:39:45 PDT
Created attachment 309285 [details]
Patch
Comment 46 youenn fablet 2017-05-06 11:10:59 PDT
change logs are a bit painful at first...
https://trac.webkit.org/wiki/UsingGitWithWebKit has some useful information.
Comment 47 Alexey Proskuryakov 2017-05-06 12:07:42 PDT
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.
Comment 48 Ben Kelly 2017-05-07 10:01:31 PDT
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.
Comment 49 youenn fablet 2017-05-07 16:34:52 PDT
(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.
Comment 50 WebKit Commit Bot 2017-05-07 17:01:10 PDT
Comment on attachment 309285 [details]
Patch

Clearing flags on attachment: 309285

Committed r216353: <http://trac.webkit.org/changeset/216353>
Comment 51 WebKit Commit Bot 2017-05-07 17:01:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 52 Alexey Proskuryakov 2017-05-07 22:54:39 PDT
> 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.
Comment 53 youenn fablet 2017-05-08 00:03:01 PDT
> 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.
Comment 54 Josh Habdas 2017-05-22 05:53:24 PDT
FYI - https://bugs.chromium.org/p/chromium/issues/detail?id=724433

Please let me know if you'd like a new bug.
Comment 55 youenn fablet 2017-05-22 07:36:56 PDT
(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.
Comment 56 Ben Kelly 2017-05-22 07:46:01 PDT
We probably need consensus on this spec issue:

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