Bug 225057 - charset in contentType used in Blob.prototype.slice(start, end, contentType) is lost
Summary: charset in contentType used in Blob.prototype.slice(start, end, contentType) ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari 14
Hardware: Mac (Intel) macOS 10.15
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-26 09:28 PDT by Rick Waldron
Modified: 2021-04-27 16:01 PDT (History)
6 users (show)

See Also:


Attachments
Patch (18.71 KB, patch)
2021-04-26 14:32 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (18.60 KB, patch)
2021-04-26 14:42 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (19.28 KB, patch)
2021-04-26 16:36 PDT, Alex Christensen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (19.48 KB, patch)
2021-04-26 17:02 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (23.27 KB, patch)
2021-04-27 10:07 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rick Waldron 2021-04-26 09:28:55 PDT
When running the following case in any Safari release, the portion of the contentType argument ";charset=utf-8" is missing from the content-type header when the response is received: 


    const blob = new Blob([`<style></style>`], { type: 'text/html' });
    const normalizedBlob = blob.slice(0, blob.size, `text/html;charset=utf-8`);
    const url = URL.createObjectURL(normalizedBlob);
    const xhr = new XMLHttpRequest();
    xhr.addEventListener('load', () => {
      console.log('(xhr-get) content-type', xhr.getResponseHeader('content-type'));
    });
    xhr.open('GET', url);
    xhr.send();


    fetch(url)
      .then(response => {
        console.log('(fetch) content-type', response.headers.get('content-type'))
      });


Expected output: 

    "(xhr-get) content-type", "text/html;charset=utf-8"
    "(fetch) content-type", "text/html;charset=utf-8"


Actual output: 

    "(xhr-get) content-type", "text/html"
    "(fetch) content-type", "text/html"


The expected output appears correct in Chrome (+ Edge) and Firefox 


I've made the test case available here: https://jsfiddle.net/rwaldron/acnxz7sv/
Comment 1 Radar WebKit Bug Importer 2021-04-26 14:18:07 PDT
<rdar://problem/77175697>
Comment 2 Alex Christensen 2021-04-26 14:32:08 PDT
Created attachment 427092 [details]
Patch
Comment 3 Chris Dumez 2021-04-26 14:35:32 PDT
Comment on attachment 427092 [details]
Patch

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

> LayoutTests/fast/files/blob-content-type-expected.txt:1
> +ALERT: content-type text/html2;charset=utf-8

Please write a test with actual PASS / FAIL lines. This output really does not make it obvious if the test is failing or passing. If I were to make a change breaking this, I could easily rebaseline this test by mistake and still think it is fine.
Comment 4 Alex Christensen 2021-04-26 14:42:04 PDT
Created attachment 427093 [details]
Patch
Comment 5 Chris Dumez 2021-04-26 14:46:46 PDT
Comment on attachment 427093 [details]
Patch

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

> LayoutTests/fast/files/blob-content-type.html:14
> +    alert(response.headers.get('content-type') == "" ? "PASS" : "FAIL")

This fails in Chrome. response.headers.get('content-type') returns null. Who is correct?
Comment 6 Chris Dumez 2021-04-26 14:49:41 PDT
Comment on attachment 427093 [details]
Patch

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

>> LayoutTests/fast/files/blob-content-type.html:14
>> +    alert(response.headers.get('content-type') == "" ? "PASS" : "FAIL")
> 
> This fails in Chrome. response.headers.get('content-type') returns null. Who is correct?

And Firefox gives "text/html" lol :) Seems browsers really don't agree on this.
Comment 7 Alexey Proskuryakov 2021-04-26 14:52:53 PDT
Comment on attachment 427093 [details]
Patch

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

> LayoutTests/fast/files/blob-content-type-expected.txt:2
> +ALERT: PASS
> +ALERT: PASS

Would you mind making the test more conventional in having some output that explains what is expected, what bug was fixed etc? There isn't always the time to do a blame to see what a test was added for.

Can this use the usual harness to avoid the need for manual testRunner.dumpAsText, and to have the "test completed" line?
Comment 8 Alex Christensen 2021-04-26 14:54:27 PDT
Oh, no.

I'm glad I added that test.  In the simple case where a content type is provided, all browsers agree that that should be used.  Where a content type is not provided, since browsers don't agree maybe we should continue our existing behavior and inherit from the sliced-from blob.

I think there is always enough time to git blame, but I can indeed make the test nicer.
Comment 9 Alex Christensen 2021-04-26 16:36:25 PDT
Created attachment 427103 [details]
Patch
Comment 10 Alexey Proskuryakov 2021-04-26 16:43:25 PDT
Comment on attachment 427103 [details]
Patch

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

> LayoutTests/fast/files/blob-content-type-expected.txt:3
> +TEST COMPLETE

Well this is wrong!
Comment 11 Chris Dumez 2021-04-26 16:50:18 PDT
Comment on attachment 427103 [details]
Patch

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

>> LayoutTests/fast/files/blob-content-type-expected.txt:3
>> +TEST COMPLETE
> 
> Well this is wrong!

Indeed.

> LayoutTests/fast/files/blob-content-type.html:1
> +<script src="../../resources/js-test-pre.js"></script>

You can just use js-test.js

> LayoutTests/fast/files/blob-content-type.html:3
> +async function runTest() {

This test is missing a description("What the test does") call and `jsTestIsAsync = true;`

> LayoutTests/fast/files/blob-content-type.html:26
> +<script src="../../resources/js-test-post.js"></script>

Not needed.
Comment 12 Chris Dumez 2021-04-26 16:51:04 PDT
Comment on attachment 427103 [details]
Patch

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

> LayoutTests/fast/files/blob-content-type.html:10
> +    shouldBe("response.headers.get('content-type')", "'text/html2;charset=utf-8'")

You can use shouldBeEqualToString() for this.

> LayoutTests/fast/files/blob-content-type.html:15
> +    shouldBe("response.headers.get('content-type')", "'text/original-blob-content-type;'")

this

> LayoutTests/fast/files/blob-content-type.html:20
> +    shouldBe("response.headers.get('content-type')", "'text/original-blob-content-type;'")

and this.
Comment 13 Chris Dumez 2021-04-26 16:52:10 PDT
Comment on attachment 427103 [details]
Patch

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

> LayoutTests/fast/files/blob-content-type.html:22
> +    asyncTestPassed();

finishJSTest();
Comment 14 Alex Christensen 2021-04-26 17:02:11 PDT
Created attachment 427108 [details]
Patch
Comment 15 Chris Dumez 2021-04-26 18:06:21 PDT
Comment on attachment 427108 [details]
Patch

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

> LayoutTests/fast/files/blob-content-type.html:5
> +    const blob = new Blob([`<style></style>`], { type: 'text/original-blob-content-type;' });

Why the weird ` quotes?

> LayoutTests/fast/files/blob-content-type.html:8
> +        const slice = blob.slice(0, blob.size, `text/html2;charset=utf-8`);

ditto.

> LayoutTests/fast/files/blob-content-type.html:22
> +        const slice = blob.slice(0, blob.size, "");

and then here double quotes, even though the rest of the file seems to be using single quotes.
Comment 16 Rick Waldron 2021-04-27 07:58:22 PDT
(In reply to Chris Dumez from comment #3)
> Comment on attachment 427092 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427092&action=review
> 
> > LayoutTests/fast/files/blob-content-type-expected.txt:1
> > +ALERT: content-type text/html2;charset=utf-8
> 
> Please write a test with actual PASS / FAIL lines. This output really does
> not make it obvious if the test is failing or passing. If I were to make a
> change breaking this, I could easily rebaseline this test by mistake and
> still think it is fine.

I've updated the jsfiddle, but here is the test code: 


    const blob = new Blob([`<style></style>`], { type: 'text/html' });
    const normalizedBlob = blob.slice(0, blob.size, `text/html;charset=utf-8`);
    const url = URL.createObjectURL(normalizedBlob);
    const xhr = new XMLHttpRequest();
    xhr.addEventListener('load', () => {
      console.log(xhr.getResponseHeader('content-type') === 'text/html;charset=utf-8' ? 'PASS' : 'FAIL');
    });
    xhr.open('GET', url);
    xhr.send();


Chrome and Firefox: PASS
Safari: FAIL
Comment 17 Chris Dumez 2021-04-27 08:03:44 PDT
(In reply to Rick Waldron from comment #16)
> (In reply to Chris Dumez from comment #3)
> > Comment on attachment 427092 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=427092&action=review
> > 
> > > LayoutTests/fast/files/blob-content-type-expected.txt:1
> > > +ALERT: content-type text/html2;charset=utf-8
> > 
> > Please write a test with actual PASS / FAIL lines. This output really does
> > not make it obvious if the test is failing or passing. If I were to make a
> > change breaking this, I could easily rebaseline this test by mistake and
> > still think it is fine.
> 
> I've updated the jsfiddle, but here is the test code: 
> 
> 
>     const blob = new Blob([`<style></style>`], { type: 'text/html' });
>     const normalizedBlob = blob.slice(0, blob.size,
> `text/html;charset=utf-8`);
>     const url = URL.createObjectURL(normalizedBlob);
>     const xhr = new XMLHttpRequest();
>     xhr.addEventListener('load', () => {
>       console.log(xhr.getResponseHeader('content-type') ===
> 'text/html;charset=utf-8' ? 'PASS' : 'FAIL');
>     });
>     xhr.open('GET', url);
>     xhr.send();
> 
> 
> Chrome and Firefox: PASS
> Safari: FAIL

Thanks Rick. I was actually talking to Alex who wrote the patch. Our code changes always need to be committed with a corresponding test. Alex did include a test in his latest iteration and the fix should get committed soon.
Comment 18 Alex Christensen 2021-04-27 10:07:37 PDT
Created attachment 427160 [details]
Patch
Comment 19 EWS 2021-04-27 16:01:51 PDT
Committed r276677 (237094@main): <https://commits.webkit.org/237094@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427160 [details].