Bug 172357 - Access-Control-Expose-Headers parsed incorrectly
Summary: Access-Control-Expose-Headers parsed incorrectly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-19 04:40 PDT by Anne van Kesteren
Modified: 2019-09-17 23:04 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.95 KB, patch)
2019-09-15 04:42 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (23.95 KB, patch)
2019-09-15 07:47 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (24.14 KB, patch)
2019-09-15 08:57 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (28.51 KB, patch)
2019-09-15 12:32 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (28.47 KB, patch)
2019-09-16 23:48 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (28.47 KB, patch)
2019-09-17 00:18 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anne van Kesteren 2017-05-19 04:40:57 PDT
Test: https://github.com/w3c/web-platform-tests/pull/6000.

In https://bugzilla.mozilla.org/show_bug.cgi?id=1364598 it was discovered that only Firefox handles this correctly out of the four browser engines. We'd appreciate if you could fix this. If you don't feel like you could fix this, please propose a change to the Fetch standard (and what change you'd like that to be) instead.
Comment 1 Rob Buis 2019-09-15 04:42:39 PDT
Created attachment 378808 [details]
Patch
Comment 2 Rob Buis 2019-09-15 07:47:11 PDT
Created attachment 378813 [details]
Patch
Comment 3 Rob Buis 2019-09-15 08:57:07 PDT
Created attachment 378814 [details]
Patch
Comment 4 Rob Buis 2019-09-15 12:32:52 PDT
Created attachment 378819 [details]
Patch
Comment 5 Rob Buis 2019-09-15 14:05:24 PDT
Note that the test is all PASSes on Catalina, I assume because of a CFNetwork fix/change.
Comment 6 youenn fablet 2019-09-16 20:07:52 PDT
Comment on attachment 378819 [details]
Patch

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

> Source/WebCore/platform/network/HTTPParsers.h:148
>      set.add(string.substring(start, end - start + 1));

We are creating a string twice, can we optimise this?
We could try to use a StringView for the isValidHTTPToken case.

> Source/WebCore/platform/network/HTTPParsers.h:162
> +                return set;

Why doing set.clear() and return Set.
Can we just do: return { }
Comment 7 Rob Buis 2019-09-16 23:48:28 PDT
Created attachment 378942 [details]
Patch
Comment 8 Rob Buis 2019-09-16 23:50:10 PDT
Comment on attachment 378819 [details]
Patch

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

>> Source/WebCore/platform/network/HTTPParsers.h:148
>>      set.add(string.substring(start, end - start + 1));
> 
> We are creating a string twice, can we optimise this?
> We could try to use a StringView for the isValidHTTPToken case.

Fixed.

>> Source/WebCore/platform/network/HTTPParsers.h:162
>> +                return set;
> 
> Why doing set.clear() and return Set.
> Can we just do: return { }

Right, I was not happy with this before, done.
Comment 9 youenn fablet 2019-09-17 00:08:58 PDT
Comment on attachment 378942 [details]
Patch

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

> Source/WebCore/platform/network/HTTPParsers.h:149
> +    set.add(token);

WTFMove()
Comment 10 Rob Buis 2019-09-17 00:18:31 PDT
Created attachment 378944 [details]
Patch
Comment 11 WebKit Commit Bot 2019-09-17 00:56:15 PDT
Comment on attachment 378944 [details]
Patch

Clearing flags on attachment: 378944

Committed r249946: <https://trac.webkit.org/changeset/249946>
Comment 12 WebKit Commit Bot 2019-09-17 00:56:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-09-17 00:57:17 PDT
<rdar://problem/55430697>
Comment 14 Russell Epstein 2019-09-17 13:57:23 PDT
(In reply to Rob Buis from comment #5)
> Note that the test is all PASSes on Catalina, I assume because of a
> CFNetwork fix/change.

Rebaselined for Catalina and iOS 13 in r249981.
Comment 15 Rob Buis 2019-09-17 23:04:34 PDT
(In reply to Russell Epstein from comment #14)
> (In reply to Rob Buis from comment #5)
> > Note that the test is all PASSes on Catalina, I assume because of a
> > CFNetwork fix/change.
> 
> Rebaselined for Catalina and iOS 13 in r249981.

Thnx! I did not know platform/mac was already meant for Catalina.