Port two more COOP tests from the legacy report-to header to Reportin…

[cdumez]

…g-Endpoints

These two COOP tests were still using the legacy report-to header instead of the modern Reporting-Endpoints one. WebKit is adding support for Reporting-Endpoints but doesn't support report-to.

[ArthurSonzogni]

I see wpt-chrome-dev-stability is failing with:

webdriver.error.InvalidArgumentException: invalid argument (400): invalid argument: No target with given id found
  (Session info: chrome=106.0.5249.21)

I reported it in https://crbug.com/1354482 If this is the only issue, then it means this is independant of your patch, and you can ask for a sheriff to bypass the CQ.

[annevk]

@ArthurSonzogni could you look at the stability issue in Chrome or maybe find someone who can? Seems that blocks landing this.

[ArthurSonzogni]

@ArthurSonzogni could you look at the stability issue in Chrome or maybe find someone who can? Seems that blocks landing this.

I already did. This is a long standing issue. I investigated and reported the issue against WebDriver in August. I pinged 15 days ago, and it got fixed 7 days ago. https://bugs.chromium.org/p/chromedriver/issues/detail?id=4207 https://chromiumdash.appspot.com/commit/c3cdede5c6e0afd8f13d96f5dc5da39f8e9debd2 On Chrome: 107.0.5304.8

The last run was 8 days ago. Maybe you can re-run the tests hoping this is now using the new version? Alternatively, you can ask a sheriff to allow this patch, since it do not introduce the issue.

[ArthurSonzogni]

Hi @cdumez,

There was a minor bug in the commit. The Reporting-Endpoints takes a structured header. The keys must not start with a number. The test is flaky, because token() starts with a number 10 times out of 16.

// Failing cases:
939b07bf-ac3d-41fc-8a44-c59399441ec4="/common/dispatcher/dispatcher.py?uuid=939b07bf-ac3d-41fc-8a44-c59399441ec4"
1b999173-1d29-4ab3-9907-7d34a014214c="/common/dispatcher/dispatcher.py?uuid=1b999173-1d29-4ab3-9907-7d34a014214c"

// Passing cases:
b98eb0b6-9666-4412-95c6-c0327ebbfcb6="/common/dispatcher/dispatcher.py?uuid=b98eb0b6-9666-4412-95c6-c0327ebbfcb6"
c8fde94d-7889-4a8c-b622-5016e11bb986="/common/dispatcher/dispatcher.py?uuid=c8fde94d-7889-4a8c-b622-5016e11bb986"
ad987391-e0cf-4854-b092-a40176c510de="/common/dispatcher/dispatcher.py?uuid=ad987391-e0cf-4854-b092-a40176c510de"

I have seen some tests using:

function reportToken() {
  // Report endpoint name must start with lower case alphabet.
  return token().replace(/./, 'a');
}

No action required from your side. I am preparing a fix. Just in case, please verify Webkit is parsing Reporting-endpoint correctly.

[cdumez]

Hi @cdumez,

There was a minor bug in the commit. The Reporting-Endpoints takes a structured header. The keys must not start with a number. The test is flaky, because token() starts with a number 10 times out of 16.

// Failing cases:
939b07bf-ac3d-41fc-8a44-c59399441ec4="/common/dispatcher/dispatcher.py?uuid=939b07bf-ac3d-41fc-8a44-c59399441ec4"
1b999173-1d29-4ab3-9907-7d34a014214c="/common/dispatcher/dispatcher.py?uuid=1b999173-1d29-4ab3-9907-7d34a014214c"

// Passing cases:
b98eb0b6-9666-4412-95c6-c0327ebbfcb6="/common/dispatcher/dispatcher.py?uuid=b98eb0b6-9666-4412-95c6-c0327ebbfcb6"
c8fde94d-7889-4a8c-b622-5016e11bb986="/common/dispatcher/dispatcher.py?uuid=c8fde94d-7889-4a8c-b622-5016e11bb986"
ad987391-e0cf-4854-b092-a40176c510de="/common/dispatcher/dispatcher.py?uuid=ad987391-e0cf-4854-b092-a40176c510de"

I have seen some tests using:

function reportToken() {
  // Report endpoint name must start with lower case alphabet.
  return token().replace(/./, 'a');
}

No action required from your side. I am preparing a fix. Just in case, please verify Webkit is parsing Reporting-endpoint correctly.

Ok, thank you for following up. We haven't seen flakiness on WebKit side yet.

[annevk]

@cdumez doesn't that mean we might have a bug in the parser?

[ArthurSonzogni]

@cdumez doesn't that mean we might have a bug in the parser?

Yes, that's what I was suspecting if the test was passing consistently previously.

When parsing a dictionary key, the structured header spec says: https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-header-structure-15#section-4.2.3.3

   1.  If the first character of input_string is not lcalpha, fail
       parsing.

with:

   lcalpha       = %x61-7A ; a-z

[cdumez]

@cdumez doesn't that mean we might have a bug in the parser?

I have just looked at our implementation and it looks alright:

        // Parse a key (https://datatracker.ietf.org/doc/html/rfc8941#section-4.2.3.3)
        if (!isASCIILower(header[index]))
            return std::nullopt;
        size_t keyStart = index++;
        while (index < header.length()) {
            UChar c = header[index];
            if (!isASCIILower(c) && !isASCIIDigit(c) && c != '_' && c != '-' && c != '.' && c != '*')
                break;
            ++index;
        }

We do check that isASCIILower() is true for the first key character. With isASCIILower() being defined as:

template<typename CharacterType> constexpr bool isASCIILower(CharacterType character)
{
    return character >= 'a' && character <= 'z';
}

[ArthurSonzogni]

I believe this code snippet was taken from the parseStructuredFieldValue parser: https://github.com/WebKit/WebKit/blob/acb70b8a1be898aca87a6f4c402678b3993f7a4f/Source/WebCore/platform/network/HTTPParsers.cpp#L584-L614

But, I believe the reporting-endpoints might be parsed by a different parser in ReportingScope::parseReportingEndpointsFromHeader: https://github.com/WebKit/WebKit/blob/06d4a7261d9b1c77d142549bd8b41e10eb94972a/Source/WebCore/Modules/reporting/ReportingScope.cpp#L118-L156

This might explain the issue? I am unfamiliar with the code source, so I am probably wrong here.

[cdumez]

I believe this code snippet was taken from the parseStructuredFieldValue parser: https://github.com/WebKit/WebKit/blob/acb70b8a1be898aca87a6f4c402678b3993f7a4f/Source/WebCore/platform/network/HTTPParsers.cpp#L584-L614

But, I believe the reporting-endpoints might be parsed by a different parser in ReportingScope::parseReportingEndpointsFromHeader: https://github.com/WebKit/WebKit/blob/06d4a7261d9b1c77d142549bd8b41e10eb94972a/Source/WebCore/Modules/reporting/ReportingScope.cpp#L118-L156

This might explain the issue? I am unfamiliar with the code source, so I am probably wrong here.

Oh, this may explain it indeed. I will look into it, thanks.

[cdumez]

I believe this code snippet was taken from the parseStructuredFieldValue parser: https://github.com/WebKit/WebKit/blob/acb70b8a1be898aca87a6f4c402678b3993f7a4f/Source/WebCore/platform/network/HTTPParsers.cpp#L584-L614 But, I believe the reporting-endpoints might be parsed by a different parser in ReportingScope::parseReportingEndpointsFromHeader: https://github.com/WebKit/WebKit/blob/06d4a7261d9b1c77d142549bd8b41e10eb94972a/Source/WebCore/Modules/reporting/ReportingScope.cpp#L118-L156 This might explain the issue? I am unfamiliar with the code source, so I am probably wrong here.

Oh, this may explain it indeed. I will look into it, thanks.

Yes, @ArthurSonzogni was totally right. Thanks for pointing this out! I am fixing the WebKit parsing via https://github.com/WebKit/WebKit/pull/4775.