[link preload] Double downloads of preloaded content when it's in MemoryCache
authoryoav@yoav.ws <yoav@yoav.ws@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 11 Apr 2017 15:10:10 +0000 (15:10 +0000)
committeryoav@yoav.ws <yoav@yoav.ws@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 11 Apr 2017 15:10:10 +0000 (15:10 +0000)
https://bugs.webkit.org/show_bug.cgi?id=170122

Reviewed by Antti Koivisto.

Source/WebCore:

No new tests, but unflaked http/tests/preload/single_download_preload_headers_charset.html.

The test was flaky because it appears as if MemoryCache is not being evicted between runs,
and running multiple iterations of the test resulted in preloaded being taken out of MemoryCache
and not having the unknown encoding flag. In those cases, the result was a double download and
a failed test.

* loader/TextResourceDecoder.cpp:
(WebCore::TextResourceDecoder::setEncoding): Set the m_encodingSet flag.
* loader/TextResourceDecoder.h: Added an m_encodingSet flag initialized to false.
* loader/cache/CachedCSSStyleSheet.cpp:
(WebCore::CachedCSSStyleSheet::setEncoding): Assert that stylesheets don't maintain decoded text.
* loader/cache/CachedResource.cpp:
(WebCore::CachedResource::CachedResource): Remove initialization of hasUnknownEncoding flag.
* loader/cache/CachedResource.h:
(WebCore::CachedResource::hasUnknownEncoding): Remove.
(WebCore::CachedResource::setHasUnknownEncoding): Remove.
(WebCore::CachedResource::CachedResource): Remove initialization of hasUnknownEncoding flag.
* loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::determineRevalidationPolicy): Set the encoding in case it changed.

LayoutTests:

* TestExpectations: Removed flakiness label from the header preload charset test.
* fast/loader/cache-encoding-expected.txt: Changed expectation.
* fast/loader/cache-encoding.html: Modified behavior to stick with the first decoded string.
* http/tests/preload/preload-encoding-expected.txt: Changed expectation.
* http/tests/preload/preload-encoding.html: Modified behavior to stick with the first decoded string.
* imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01-expected.txt: This test refers to the same file
twice and expects different decoding for it each time. This is the behavior that we modified, and therefore the test expectation is changed as well.

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/fast/loader/cache-encoding-expected.txt
LayoutTests/fast/loader/cache-encoding.html
LayoutTests/http/tests/preload/preload-encoding-expected.txt
LayoutTests/http/tests/preload/preload-encoding.php
LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/loader/TextResourceDecoder.cpp
Source/WebCore/loader/TextResourceDecoder.h
Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp
Source/WebCore/loader/cache/CachedResource.cpp
Source/WebCore/loader/cache/CachedResource.h
Source/WebCore/loader/cache/CachedResourceLoader.cpp

index 23a4f26..3d17d18 100644 (file)
@@ -1,3 +1,18 @@
+2017-04-11  Yoav Weiss  <yoav@yoav.ws>
+
+        [link preload] Double downloads of preloaded content when it's in MemoryCache
+        https://bugs.webkit.org/show_bug.cgi?id=170122
+
+        Reviewed by Antti Koivisto.
+
+        * TestExpectations: Removed flakiness label from the header preload charset test.
+        * fast/loader/cache-encoding-expected.txt: Changed expectation.
+        * fast/loader/cache-encoding.html: Modified behavior to stick with the first decoded string.
+        * http/tests/preload/preload-encoding-expected.txt: Changed expectation.
+        * http/tests/preload/preload-encoding.html: Modified behavior to stick with the first decoded string.
+        * imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01-expected.txt: This test refers to the same file
+        twice and expects different decoding for it each time. This is the behavior that we modified, and therefore the test expectation is changed as well.
+
 2017-04-11  Manuel Rego Casasnovas  <rego@igalia.com>
 
         [css-grid] Fix fast/css-grid-layout/grid-simplified-layout-positioned.html
index 4852aa8..fc9fb77 100644 (file)
@@ -1092,8 +1092,6 @@ fast/history/page-cache-with-opener.html [ Skip ]
 
 webkit.org/b/168238 imported/w3c/web-platform-tests/dom/events/EventListener-invoke-legacy.html [ Pass Failure ]
 
-webkit.org/b/170122 http/tests/preload/single_download_preload_headers_charset.php [ Pass Failure ]
-
 ########################################
 ### START OF -disabled tests
 
index 785d97d..50962df 100644 (file)
@@ -1,11 +1,11 @@
-CONSOLE MESSAGE: line 1: SyntaxError: Invalid character '\u8307'
 First load a script with a wrong charset then again with the right one. Second attempt should work and 'scriptSuccess' should be set to true. 'successfullyParsed' will be undefined.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
 PASS scriptSuccess is true
-FAIL successfullyParsed should be true (of type boolean). Was undefined (of type undefined).
+PASS scriptSuccess is true
+PASS successfullyParsed is true
 
 TEST COMPLETE
 
index 0bbb60a..c4758cd 100644 (file)
@@ -16,8 +16,9 @@ function appendScriptWithCharset(charset, onload)
 
 function test()
 {
-    appendScriptWithCharset("utf-16", function () {
-        appendScriptWithCharset("utf-8", function () {
+    appendScriptWithCharset("utf-8", function () {
+        shouldBeTrue("scriptSuccess");
+        appendScriptWithCharset("utf-16", function () {
             shouldBeTrue("scriptSuccess");
             finishJSTest();
         });
index 785d97d..50962df 100644 (file)
@@ -1,11 +1,11 @@
-CONSOLE MESSAGE: line 1: SyntaxError: Invalid character '\u8307'
 First load a script with a wrong charset then again with the right one. Second attempt should work and 'scriptSuccess' should be set to true. 'successfullyParsed' will be undefined.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
 PASS scriptSuccess is true
-FAIL successfullyParsed should be true (of type boolean). Was undefined (of type undefined).
+PASS scriptSuccess is true
+PASS successfullyParsed is true
 
 TEST COMPLETE
 
index 38e8398..4dddf33 100644 (file)
@@ -19,8 +19,10 @@ function appendScriptWithCharset(charset, onload)
 
 function test()
 {
-    appendScriptWithCharset("utf-16", function () {
-        appendScriptWithCharset("utf-8", function () {
+    appendScriptWithCharset("utf-8", function () {
+        shouldBeTrue("scriptSuccess");
+        scriptSuccess = false;
+        appendScriptWithCharset("utf-16", function () {
             shouldBeTrue("scriptSuccess");
             finishJSTest();
         });
index cc4a5b0..fe3fdb7 100644 (file)
@@ -2,7 +2,7 @@
 PASS Script @type: unknown parameters 
 PASS Script @type: unknown parameters 1 
 PASS Script @type: unknown parameters 2 
-PASS Script @type: unknown parameters 3 
+FAIL Script @type: unknown parameters 3 assert_equals: expected "śćążź" but got "\ufffd湿\ufffd"
 PASS Script @type: unknown parameters 4 
 PASS Script @type: unknown parameters 5 
 
index 7ff37ed..3b18013 100644 (file)
@@ -1,3 +1,31 @@
+2017-04-11  Yoav Weiss  <yoav@yoav.ws>
+
+        [link preload] Double downloads of preloaded content when it's in MemoryCache
+        https://bugs.webkit.org/show_bug.cgi?id=170122
+
+        Reviewed by Antti Koivisto.
+
+        No new tests, but unflaked http/tests/preload/single_download_preload_headers_charset.html.
+
+        The test was flaky because it appears as if MemoryCache is not being evicted between runs,
+        and running multiple iterations of the test resulted in preloaded being taken out of MemoryCache
+        and not having the unknown encoding flag. In those cases, the result was a double download and
+        a failed test.
+
+        * loader/TextResourceDecoder.cpp:
+        (WebCore::TextResourceDecoder::setEncoding): Set the m_encodingSet flag.
+        * loader/TextResourceDecoder.h: Added an m_encodingSet flag initialized to false.
+        * loader/cache/CachedCSSStyleSheet.cpp:
+        (WebCore::CachedCSSStyleSheet::setEncoding): Assert that stylesheets don't maintain decoded text.
+        * loader/cache/CachedResource.cpp:
+        (WebCore::CachedResource::CachedResource): Remove initialization of hasUnknownEncoding flag.
+        * loader/cache/CachedResource.h:
+        (WebCore::CachedResource::hasUnknownEncoding): Remove.
+        (WebCore::CachedResource::setHasUnknownEncoding): Remove.
+        (WebCore::CachedResource::CachedResource): Remove initialization of hasUnknownEncoding flag.
+        * loader/cache/CachedResourceLoader.cpp:
+        (WebCore::CachedResourceLoader::determineRevalidationPolicy): Set the encoding in case it changed.
+
 2017-04-11  Miguel Gomez  <magomez@igalia.com>
 
         REGRESSION(r215211): [GTK] Lots of image related tests are timing out, causing the test bot to exit early
index fe94b7f..afc8205 100644 (file)
@@ -355,6 +355,7 @@ void TextResourceDecoder::setEncoding(const TextEncoding& encoding, EncodingSour
     else
         m_encoding = encoding;
 
+    m_encodingSet = true;
     m_codec = nullptr;
     m_source = source;
 }
index 98ec925..6e11d9f 100644 (file)
@@ -68,6 +68,7 @@ public:
    
     void useLenientXMLDecoding() { m_useLenientXMLDecoding = true; }
     bool sawError() const { return m_sawError; }
+    bool encodingSet() const { return m_encodingSet; }
 
 private:
     WEBCORE_EXPORT TextResourceDecoder(const String& mimeType, const TextEncoding& defaultEncoding, bool usesEncodingDetector);
@@ -95,6 +96,7 @@ private:
     bool m_useLenientXMLDecoding; // Don't stop on XML decoding errors.
     bool m_sawError;
     bool m_usesEncodingDetector;
+    bool m_encodingSet { false };
 
     std::unique_ptr<HTMLMetaCharsetParser> m_charsetParser;
 };
index af40cc2..bd160af 100644 (file)
@@ -64,6 +64,7 @@ void CachedCSSStyleSheet::didAddClient(CachedResourceClient& client)
 
 void CachedCSSStyleSheet::setEncoding(const String& chs)
 {
+    ASSERT(m_decodedSheetText.isNull());
     m_decoder->setEncoding(chs, TextResourceDecoder::EncodingFromHTTPHeader);
 }
 
index c8c47f0..eee2033 100644 (file)
@@ -123,7 +123,6 @@ CachedResource::CachedResource(CachedResourceRequest&& request, Type type, Sessi
     , m_origin(request.releaseOrigin())
     , m_initiatorName(request.initiatorName())
     , m_isLinkPreload(request.isLinkPreload())
-    , m_hasUnknownEncoding(request.isLinkPreload())
     , m_type(type)
 {
     ASSERT(sessionID.isValid());
index a8bd811..380b546 100644 (file)
@@ -232,8 +232,6 @@ public:
     void decreasePreloadCount() { ASSERT(m_preloadCount); --m_preloadCount; }
     bool isLinkPreload() { return m_isLinkPreload; }
     void setLinkPreload() { m_isLinkPreload = true; }
-    bool hasUnknownEncoding() { return m_hasUnknownEncoding; }
-    void setHasUnknownEncoding(bool hasUnknownEncoding) { m_hasUnknownEncoding = hasUnknownEncoding; }
 
     void registerHandle(CachedResourceHandleBase*);
     WEBCORE_EXPORT void unregisterHandle(CachedResourceHandleBase*);
@@ -336,7 +334,6 @@ private:
     bool m_inCache { false };
     bool m_loading { false };
     bool m_isLinkPreload { false };
-    bool m_hasUnknownEncoding { false };
 
     bool m_switchingClientsToRevalidatedResource { false };
 
index e89065b..c199d26 100644 (file)
@@ -935,12 +935,8 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida
         return Reload;
 
     auto* textDecoder = existingResource->textResourceDecoder();
-    if (textDecoder && !textDecoder->hasEqualEncodingForCharset(cachedResourceRequest.charset())) {
-        if (!existingResource->hasUnknownEncoding())
-            return Reload;
-        existingResource->setHasUnknownEncoding(false);
+    if (textDecoder && !textDecoder->hasEqualEncodingForCharset(cachedResourceRequest.charset()) && !textDecoder->encodingSet())
         existingResource->setEncoding(cachedResourceRequest.charset());
-    }
 
     // FIXME: We should use the same cache policy for all resource types. The raw resource policy is overly strict
     //        while the normal subresource policy is too loose.