REGRESSION(r158333): http/tests/xmlhttprequest/response-encoding.html and xmlhttprequ...
authorap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Oct 2013 16:50:03 +0000 (16:50 +0000)
committerap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Oct 2013 16:50:03 +0000 (16:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=123548

Reviewed by Brady Eidson.

Source/WebCore:

We had code that made sure that cached 200 responses weren't used for conditional
requests. But it didn't work the other way - cached 304 responses got reused for
subsequent unconditional requests!

Adding the test uncovered this bug.

* loader/cache/CachedRawResource.cpp: (WebCore::shouldIgnoreHeaderForCacheReuse):
Should never ignore conditional headers. Code in determineRevalidationPolicy
was already undoing this for conditional requests, but we also shouldn't use
WebCore cache if it holds a 304 response to conditional request.

* loader/cache/CachedResourceLoader.cpp: (WebCore::CachedResourceLoader::determineRevalidationPolicy):
Even though the changed code is only for raw resources, I think that we can never
get a conditional request here any more.

LayoutTests:

* TestExpectations: Unskip tests that used to be affected by response-empty-arraybuffer.html

* http/tests/xmlhttprequest/response-empty-arraybuffer-expected.txt:
* http/tests/xmlhttprequest/response-empty-arraybuffer.html:
Fix a stupid typo. This test actually fully passes.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@158362 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/http/tests/xmlhttprequest/response-empty-arraybuffer-expected.txt
LayoutTests/http/tests/xmlhttprequest/response-empty-arraybuffer.html
Source/WebCore/ChangeLog
Source/WebCore/loader/cache/CachedRawResource.cpp
Source/WebCore/loader/cache/CachedResourceLoader.cpp

index a465614..36de5f6 100644 (file)
@@ -1,3 +1,16 @@
+2013-10-31  Alexey Proskuryakov  <ap@apple.com>
+
+        REGRESSION(r158333): http/tests/xmlhttprequest/response-encoding.html and xmlhttprequest-overridemimetype-content-type-header.html are failing
+        https://bugs.webkit.org/show_bug.cgi?id=123548
+
+        Reviewed by Brady Eidson.
+
+        * TestExpectations: Unskip tests that used to be affected by response-empty-arraybuffer.html
+
+        * http/tests/xmlhttprequest/response-empty-arraybuffer-expected.txt:
+        * http/tests/xmlhttprequest/response-empty-arraybuffer.html:
+        Fix a stupid typo. This test actually fully passes.
+
 2013-10-31  Krzysztof Wolanski  <k.wolanski@samsung.com>
 
         [EFL] Rebaselining after r158186
index e72828b..193f036 100644 (file)
@@ -76,8 +76,5 @@ webkit.org/b/122679 security/crypto-subtle-gc.html [ Skip ]
 webkit.org/b/122679 security/crypto-subtle-gc-2.html [ Skip ]
 webkit.org/b/122679 security/crypto-subtle-gc-3.html [ Skip ]
 
-webkit.org/b/123548 http/tests/xmlhttprequest/response-encoding.html [ Failure ]
-webkit.org/b/123548 http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-content-type-header.html [ Failure ]
-
 webkit.org/b/123555 [ Debug ] media/media-fragments/TC0054.html [ Crash ]
 webkit.org/b/123555 [ Debug ] media/media-fragments/TC0061.html [ Crash ]
index 41e786e..8e49dbe 100644 (file)
@@ -11,9 +11,9 @@ Test that XMLHttpRequest.response returns an empty ArrayBuffer when received sta
 PASS request.status is 200
 PASS Object.prototype.toString.call(request.response) is '[object ArrayBuffer]'
 PASS request.response.byteLength is 68
-FAIL request2.status should be 304. Was 200.
+PASS request2.status is 304
 PASS Object.prototype.toString.call(request2.response) is '[object ArrayBuffer]'
-FAIL request2.response.byteLength should be 0. Was 68.
+PASS request2.response.byteLength is 0
 PASS successfullyParsed is true
 
 TEST COMPLETE
index 04a7b37..cd1bfc3 100644 (file)
@@ -58,7 +58,7 @@ function notModifiedTest()
             if (req2.readyState != 4)
                 return;
 
-            request2 = req;
+            request2 = req2;
 
             shouldBe("request2.status", "304");
             shouldBe("Object.prototype.toString.call(request2.response)", "'[object ArrayBuffer]'");
index ab7e052..86e801c 100644 (file)
@@ -1,3 +1,25 @@
+2013-10-31  Alexey Proskuryakov  <ap@apple.com>
+
+        REGRESSION(r158333): http/tests/xmlhttprequest/response-encoding.html and xmlhttprequest-overridemimetype-content-type-header.html are failing
+        https://bugs.webkit.org/show_bug.cgi?id=123548
+
+        Reviewed by Brady Eidson.
+
+        We had code that made sure that cached 200 responses weren't used for conditional
+        requests. But it didn't work the other way - cached 304 responses got reused for
+        subsequent unconditional requests!
+
+        Adding the test uncovered this bug.
+
+        * loader/cache/CachedRawResource.cpp: (WebCore::shouldIgnoreHeaderForCacheReuse):
+        Should never ignore conditional headers. Code in determineRevalidationPolicy
+        was already undoing this for conditional requests, but we also shouldn't use
+        WebCore cache if it holds a 304 response to conditional request.
+
+        * loader/cache/CachedResourceLoader.cpp: (WebCore::CachedResourceLoader::determineRevalidationPolicy):
+        Even though the changed code is only for raw resources, I think that we can never
+        get a conditional request here any more.
+
 2013-10-30  Alexey Proskuryakov  <ap@apple.com>
 
         CryptoAlgorithmDescriptionBuilder should support producing nested algorithms
index 44128d1..a6eff5e 100644 (file)
@@ -203,8 +203,6 @@ static bool shouldIgnoreHeaderForCacheReuse(AtomicString headerName)
     if (m_headers.isEmpty()) {
         m_headers.add("Accept");
         m_headers.add("Cache-Control");
-        m_headers.add("If-Modified-Since");
-        m_headers.add("If-None-Match");
         m_headers.add("Origin");
         m_headers.add("Pragma");
         m_headers.add("Purpose");
index b5b2fcf..9f9e457 100644 (file)
@@ -584,11 +584,8 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida
     if (!existingResource->canReuse(request))
         return Reload;
 
-    // Certain requests (e.g., XHRs) might have manually set headers that require revalidation.
-    // FIXME: In theory, this should be a Revalidate case. In practice, the MemoryCache revalidation path assumes a whole bunch
-    // of things about how revalidation works that manual headers violate, so punt to Reload instead.
-    if (request.isConditional())
-        return Reload;
+    // Conditional requests should have failed canReuse check.
+    ASSERT(!request.isConditional());
 
     // Do not load from cache if images are not enabled. The load for this image will be blocked
     // in CachedImage::load.