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)
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.
Created attachment 253184 [details] Patch
This is not specific to GTK+, only cross-platform code is changed in the patch.
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.
Disk cache specific test that is.
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...
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.
Created attachment 253328 [details] Patch
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.
Created attachment 253434 [details] Patch
Comment on attachment 253434 [details] Patch Clearing flags on attachment: 253434 Committed r184690: <http://trac.webkit.org/changeset/184690>
All reviewed patches have been landed. Closing bug.
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.
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.
(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.
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.
(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
*** Bug 82672 has been marked as a duplicate of this bug. ***
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 ]
Updated expectations in http://trac.webkit.org/r226375. Now the test is expected to pass with WK2.