Bug 225057 – charset in contentType used in Blob.prototype.slice(start, end, contentType) is lost

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Rick Waldron]

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/

[Radar WebKit Bug Importer]

<rdar://problem/77175697>

[Alex Christensen]

Created attachment 427092 [details]
Patch

[Chris Dumez]

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.

[Alex Christensen]

Created attachment 427093 [details]
Patch

[Chris Dumez]

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?

[Chris Dumez]

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.

[Alexey Proskuryakov]

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?

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 427103 [details]
Patch

[Alexey Proskuryakov]

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!

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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();

[Alex Christensen]

Created attachment 427108 [details]
Patch

[Chris Dumez]

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.

[Rick Waldron]

(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

[Chris Dumez]

(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.

[Alex Christensen]

Created attachment 427160 [details]
Patch

[EWS]

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].