Bug 236263 – http/tests/cache-storage/cache-origins.https.html is flaky

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Dumez]

http/tests/cache-storage/cache-origins.https.html is flaky because it relies on a particular HashMap ordering.

[Chris Dumez]

Created attachment 451157 [details]
Patch

[Darin Adler]

Comment on attachment 451157 [details]
Patch

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

We should do this sorting inside WebCore, I think.

> LayoutTests/http/tests/cache-storage/cache-origins.https.html:23
> +        representation.sort((a, b) => { return a.origin.clientOrigin > b.origin.clientOrigin; });

This isn’t right. It’s supposed to be a three way sort function, and needs to return -1, 0, +1 based on the ordering. Not like C++ standard library where they used boolean less than style sorting.

> LayoutTests/http/tests/cache-storage/cache-origins.https.html:36
> +        representation.sort((a, b) => { return a.origin.clientOrigin > b.origin.clientOrigin; });

Ditto.

[Chris Dumez]

Created attachment 451163 [details]
Patch

[Darin Adler]

Comment on attachment 451163 [details]
Patch

I still think we should fix this inside WebCore, but this is a good fix. We should also take out the "value_or(0)" we added to work around it.

[Chris Dumez]

Created attachment 451167 [details]
Patch

[Chris Dumez]

Alternative approach, fixing it on native side, as suggested.

[Chris Dumez]

Created attachment 451168 [details]
Patch

[Darin Adler]

Comment on attachment 451168 [details]
Patch

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

I like this as is, but I have some thoughts on little style tweaks.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:796
>      builder.append("{ \"path\": \"");
>      builder.append(m_rootPath);
>      builder.append("\", \"origins\": [");

We can merge these all into one line.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:797
> +    Vector<String> origins = WTF::map(m_caches, [](auto& keyValue) {

Could use auto here instead of Vector<String>. I think it would look a little better if it’s built even before we define StringBuilder.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:803
> +        originBuilder.append("\n{ \"origin\" : { \"topOrigin\" : \"");
> +        originBuilder.append(keyValue.key.topOrigin.toString());
> +        originBuilder.append("\", \"clientOrigin\": \"");
> +        originBuilder.append(keyValue.key.clientOrigin.toString());
> +        originBuilder.append("\" }, \"caches\" : ");

We can merge these all into fewer lines.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:811
> +    for (auto& origin : origins) {

Another way to write this:

    const char* divider = "";
    for (auto& origin : origins) {
        builder.append(divider, origin);
        divider = ",";
    }

A little less wordy than the isFirst version.

[Chris Dumez]

Created attachment 451243 [details]
Patch

[EWS]

Committed r289419 (246982@main): <https://commits.webkit.org/246982@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/88648506>