Bug 144682 - Network Cache: Layout Test http/tests/xmlhttprequest/range-test.html is failing
Summary: Network Cache: Layout Test http/tests/xmlhttprequest/range-test.html is failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 82672 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-05-06 05:10 PDT by Marcos Chavarría Teijeiro (irc: chavaone)
Modified: 2018-01-03 15:55 PST (History)
8 users (show)

See Also:


Attachments
Patch (7.27 KB, patch)
2015-05-15 02:27 PDT, Marcos Chavarría Teijeiro (irc: chavaone)
no flags Details | Formatted Diff | Diff
Patch (11.32 KB, patch)
2015-05-18 09:20 PDT, Marcos Chavarría Teijeiro (irc: chavaone)
no flags Details | Formatted Diff | Diff
Patch (11.76 KB, patch)
2015-05-20 01:23 PDT, Marcos Chavarría Teijeiro (irc: chavaone)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marcos Chavarría Teijeiro (irc: chavaone) 2015-05-06 05:10:50 PDT
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)
Comment 1 Marcos Chavarría Teijeiro (irc: chavaone) 2015-05-14 02:10:36 PDT
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.
Comment 2 Marcos Chavarría Teijeiro (irc: chavaone) 2015-05-15 02:27:53 PDT
Created attachment 253184 [details]
Patch
Comment 3 Carlos Garcia Campos 2015-05-15 05:45:45 PDT
This is not specific to GTK+, only cross-platform code is changed in the patch.
Comment 4 Antti Koivisto 2015-05-15 05:59:02 PDT
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.
Comment 5 Antti Koivisto 2015-05-15 06:00:18 PDT
Disk cache specific test that is.
Comment 6 Marcos Chavarría Teijeiro (irc: chavaone) 2015-05-18 04:51:25 PDT
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...
Comment 7 Marcos Chavarría Teijeiro (irc: chavaone) 2015-05-18 08:59:15 PDT
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.
Comment 8 Marcos Chavarría Teijeiro (irc: chavaone) 2015-05-18 09:20:20 PDT
Created attachment 253328 [details]
Patch
Comment 9 Antti Koivisto 2015-05-19 09:03:09 PDT
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.
Comment 10 Marcos Chavarría Teijeiro (irc: chavaone) 2015-05-20 01:23:17 PDT
Created attachment 253434 [details]
Patch
Comment 11 WebKit Commit Bot 2015-05-20 19:09:24 PDT
Comment on attachment 253434 [details]
Patch

Clearing flags on attachment: 253434

Committed r184690: <http://trac.webkit.org/changeset/184690>
Comment 12 WebKit Commit Bot 2015-05-20 19:09:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Carlos Garcia Campos 2015-05-26 00:22:35 PDT
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.
Comment 14 Antti Koivisto 2015-05-26 07:57:02 PDT
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.
Comment 15 Carlos Garcia Campos 2015-05-26 08:01:15 PDT
(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.
Comment 16 Antti Koivisto 2015-05-26 11:35:37 PDT
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.
Comment 17 Carlos Garcia Campos 2015-05-27 01:05:59 PDT
(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
Comment 18 David Kilzer (:ddkilzer) 2015-07-03 08:46:14 PDT
*** Bug 82672 has been marked as a duplicate of this bug. ***
Comment 19 Alexey Proskuryakov 2018-01-03 13:35:38 PST
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 ]
Comment 20 Alexey Proskuryakov 2018-01-03 15:55:22 PST
Updated expectations in http://trac.webkit.org/r226375. Now the test is expected to pass with WK2.