Bug 144682 – Network Cache: Layout Test http/tests/xmlhttprequest/range-test.html is failing

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

[Marcos Chavarría Teijeiro (irc: chavaone)]

The http/tests/xmlhttprequest/range-test.html layout test is failing on GTK+ since revision r183865 (https://trac.webkit.org/changeset/r183865).

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/http/tests/xmlhttprequest/range-test-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/http/tests/xmlhttprequest/range-test-actual.txt
@@ -1,9 +1,11 @@
 Test Range support in XMLHttpRequest
 
 getRange(resources/reply.xml, 34, 36, false)
-Length : expected 2 got 2
+Expected a 206 response, got 200
+Length : expected 2 got 68
 getRange(resources/reply.xml, 35, 38, false)
-Length : expected 3 got 3
+Expected a 206 response, got 200
+Length : expected 3 got 68
 getRange(resources/reply.xml, 34, 36, true)
 Length : expected 2 got 2
 getRange(resources/reply.xml, 35, 38, true)

[Marcos Chavarría Teijeiro (irc: chavaone)]

This test is failing due to the new implemented Cache. We make 5 requests for the same file. The first request will get the complete file and the remaining request will get parts of this files specified by the Range header. After the first request the file is cached so  with the next requests providing the url is the same, the reply would be the same and it returns the whole file instead of some part of it.

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 253184 [details]
Patch

[Carlos Garcia Campos]

This is not specific to GTK+, only cross-platform code is changed in the patch.

[Antti Koivisto]

Comment on attachment 253184 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:114
> +    if (range.isEmpty())
> +        range = ASCIILiteral("No range");

This is unnecessary, you can just leave the range string empty.

> LayoutTests/platform/gtk/TestExpectations:-2275
> -webkit.org/b/144682 http/tests/xmlhttprequest/range-test.html [ Failure ]

You should also add a specific test for range header. This should easy to do, check the other tests under LayoutTests/http/tests/cache/disk-cache.

[Antti Koivisto]

Disk cache specific test that is.

[Marcos Chavarría Teijeiro (irc: chavaone)]

Comment on attachment 253184 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:114
>> +        range = ASCIILiteral("No range");
> 
> This is unnecessary, you can just leave the range string empty.

If I leave the string empty, the NetworkProccess crashes :S.

I have to investigate why it does...

[Marcos Chavarría Teijeiro (irc: chavaone)]

The relevant part of the stack trace is the following:

Thread 1 (Thread 0x7fd13c2d4a80 (LWP 16504)):
#0  0x00007fd15506ed64 in WTF::StringImpl::is8Bit (this=0x0) at ../../Source/WTF/wtf/text/StringImpl.h:457
#1  0x00007fd15506f0a4 in WTF::String::is8Bit (this=0x7fff862fd058) at ../../Source/WTF/wtf/text/WTFString.h:180
#2  0x00007fd15544a331 in WebKit::NetworkCache::hashString (md5=..., string=...) at ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.cpp:70
#3  0x00007fd15544a4a0 in WebKit::NetworkCache::Key::computeHash (this=0x7fff862fd040) at ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.cpp:88
#4  0x00007fd15544a127 in WebKit::NetworkCache::Key::Key (this=0x7fff862fd040, method=..., partition=..., range=..., identifier=...) at ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.cpp:53

I guess we need to not leave that string empty.

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 253328 [details]
Patch

[Antti Koivisto]

Comment on attachment 253328 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:112
> +    String range = request.httpHeaderField(WebCore::HTTPHeaderName::Range);

You could add a FIXME comment that explains that this implements minimal Range support where we don't parse the ranges and only the same exact range request will be served from the cache.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:114
> +    if (range.isEmpty())
> +        range = ASCIILiteral("No range");

You should fix hashString() in NetworkCacheKey.cpp to work with null strings instead. if (string.isNull()) return should be sufficient.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:243
> +    case 206: // Partial Content
>      case 204: // No Content

Please keep these in order.

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 253434 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 253434 [details]
Patch

Clearing flags on attachment: 253434

Committed r184690: <http://trac.webkit.org/changeset/184690>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

Shouldn't we bump the cache version for this kind of changes? I guess we will end up trying to decode range from existing entries.

[Antti Koivisto]

I think the existing entries stay valid with this chance. Even if they didn't it is fine to just fail decoding. Version bump is mostly meant for when cache directory structure changes.

[Carlos Garcia Campos]

(In reply to comment #14)
> I think the existing entries stay valid with this chance. Even if they
> didn't it is fine to just fail decoding. Version bump is mostly meant for
> when cache directory structure changes.

I had to remove the cache after this change, but maybe it's because we don't handle decode errors correctly, our IOChannel ended up trying to read a NULL stream and then the network process stopped loading anything.

[Antti Koivisto]

Yeah, sounds like the real bug is elsewhere. The idea is that obsolete or corrupted cache entries are detected and silently removed. No cache state should cause crashes.

[Carlos Garcia Campos]

(In reply to comment #16)
> Yeah, sounds like the real bug is elsewhere. The idea is that obsolete or
> corrupted cache entries are detected and silently removed. No cache state
> should cause crashes.

Ok, I handled errors when creating the IO streams in the end, see bug #145406

[David Kilzer (:ddkilzer)]

*** Bug 82672 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

http/tests/xmlhttprequest/range-test.html is still marked as flaky:

LayoutTests/platform/mac/TestExpectations:294:webkit.org/b/82672 http/tests/xmlhttprequest/range-test.html [ Pass Failure ]
LayoutTests/platform/ios-wk1/TestExpectations:880:webkit.org/b/82672 http/tests/xmlhttprequest/range-test.html [ Failure ]

[Alexey Proskuryakov]

Updated expectations in http://trac.webkit.org/r226375. Now the test is expected to pass with WK2.