Bug 170122 – [link preload] Double downloads of preloaded content when it's in MemoryCache

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Page Loading

[Yoav Weiss]

As a follow up to https://bugs.webkit.org/show_bug.cgi?id=169274 I still see double downloads in test scenarios (where I believe the test runner is not necessarily clearing the MemoryCache between runs).

I think there are also other scenarios where the solution we added to 169274 is not sufficient and can cause a double download. Also, in a separate patch where I'm trying to add preloadScanner support to link preload, I see a lot of tests regress due to double downloads.

[Yoav Weiss]

Created attachment 305733 [details]
Patch

[Yoav Weiss]

Antti - this is (more or less) the same patch you reviewed as part of 169274, but did not like.

As I'm now encountering flakiness related to the fix we landed there, I think we should reconsider this approach. As you mentioned there that there might be correctness issues with this approach, maybe we can think of a solution to those that won't require resource reloading?

[Build Bot]

Comment on attachment 305733 [details]
Patch

Attachment 305733 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3434039

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01.html
imported/w3c/web-platform-tests/html/syntax/parsing-html-fragments/the-input-byte-stream-007.html
imported/w3c/web-platform-tests/html/syntax/parsing-html-fragments/the-input-byte-stream-009.html
imported/w3c/web-platform-tests/html/syntax/parsing-html-fragments/the-input-byte-stream-001.html
imported/w3c/web-platform-tests/html/syntax/parsing-html-fragments/the-input-byte-stream-015.html

[Build Bot]

Created attachment 305737 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 305733 [details]
Patch

Attachment 305733 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3434041

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01.html
imported/w3c/web-platform-tests/html/syntax/parsing-html-fragments/the-input-byte-stream-030.html
imported/w3c/web-platform-tests/html/syntax/parsing-html-fragments/the-input-byte-stream-018.html

[Build Bot]

Created attachment 305739 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 305733 [details]
Patch

Attachment 305733 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3434038

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01.html
imported/w3c/web-platform-tests/html/syntax/parsing-html-fragments/the-input-byte-stream-001.html
imported/w3c/web-platform-tests/html/syntax/parsing-html-fragments/the-input-byte-stream-018.html
imported/w3c/web-platform-tests/html/syntax/parsing-html-fragments/the-input-byte-stream-015.html

[Build Bot]

Created attachment 305742 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 305733 [details]
Patch

Attachment 305733 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3434056

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01.html
imported/w3c/web-platform-tests/html/syntax/parsing-html-fragments/the-input-byte-stream-018.html

[Build Bot]

Created attachment 305747 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Yoav Weiss]

Created attachment 305793 [details]
Patch

[Antti Koivisto]

Comment on attachment 305793 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        [link preload] Double downloads of preloaded content when it's in MemoryCache
> +        https://bugs.webkit.org/show_bug.cgi?id=170122
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests, but unflaked http/tests/preload/single_download_preload_headers_charset.html.

You should explain in the ChangeLog what problem this solves (that is causing the flakiness) and how.

> Source/WebCore/loader/cache/CachedResource.cpp:-126
> -    , m_hasUnknownEncoding(request.isLinkPreload())

The existing behaviour is safe because it is limited to preloads. However this patch makes the encoding changes happen in other situations too.

> Source/WebCore/loader/cache/CachedScript.cpp:61
>  void CachedScript::setEncoding(const String& chs)
>  {
> +    TextEncoding previousEncoding = m_decoder->encoding();
>      m_decoder->setEncoding(chs, TextResourceDecoder::EncodingFromHTTPHeader);
> +    if (previousEncoding != m_decoder->encoding()) {
> +        m_script = String();
> +        m_scriptHash = 0;
> +        m_decodingState = NeverDecoded;
> +    }
>  }

I don't think this is safe. CachedScript might have existing clients that expect it to stay immutable. It may be impossible to reconstruct m_script after encoding change.

[Yoav Weiss]

Created attachment 306378 [details]
Patch

[Build Bot]

Comment on attachment 306378 [details]
Patch

Attachment 306378 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3484608

New failing tests:
fast/loader/cache-encoding.html

[Build Bot]

Created attachment 306383 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 306378 [details]
Patch

Attachment 306378 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3484618

New failing tests:
fast/loader/cache-encoding.html

[Build Bot]

Created attachment 306386 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 306378 [details]
Patch

Attachment 306378 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3484615

New failing tests:
fast/loader/cache-encoding.html

[Build Bot]

Created attachment 306387 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 306378 [details]
Patch

Attachment 306378 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3484633

New failing tests:
fast/loader/cache-encoding.html

[Build Bot]

Created attachment 306388 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Yoav Weiss]

Created attachment 306391 [details]
Patch

[Yoav Weiss]

The patch was updated to avoid encoding updates. PTAL?

[Antti Koivisto]

Comment on attachment 306391 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01-expected.txt:5
> -PASS Script @type: unknown parameters 3 
> +FAIL Script @type: unknown parameters 3 assert_equals: expected "ÅÄÄżź" but got "\ufffd湿\ufffd"

This is not mentioned in ChangeLog. Why does this turn into FAIL?

[Yoav Weiss]

Created attachment 306809 [details]
Patch

[Yoav Weiss]

(In reply to Antti Koivisto from comment #24)
> Comment on attachment 306391 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306391&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01-expected.txt:5
> > -PASS Script @type: unknown parameters 3 
> > +FAIL Script @type: unknown parameters 3 assert_equals: expected "ÅÄÄżź" but got "\ufffd湿\ufffd"
> 
> This is not mentioned in ChangeLog. Why does this turn into FAIL?

I added that to the ChangeLog with an explanation.
The test on https://github.com/w3c/web-platform-tests/blob/master/html/semantics/scripting-1/the-script-element/script-charset-01.html is using the same URL twice and expecting it to be decoded differently. That is not material to the test itself (and is failing in Chrome as well), so I'll PR a fix for that upstream.

[Yoav Weiss]

Comment on attachment 306809 [details]
Patch

Thanks for reviewing! :)

[Yoav Weiss]

Related upstream PR for WPT change: https://github.com/w3c/web-platform-tests/pull/5532

[WebKit Commit Bot]

Comment on attachment 306809 [details]
Patch

Clearing flags on attachment: 306809

Committed r215229: <http://trac.webkit.org/changeset/215229>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 306809 [details]
Patch

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

This change caused:
https://bugs.webkit.org/show_bug.cgi?id=171091

> LayoutTests/http/tests/preload/preload-encoding.php:22
> +    appendScriptWithCharset("utf-8", function () {

This essentially disabled the test which was supposed to:
1. Load the script with a bad encoding
2. Get a SyntaxError
3. Load the script with the right encoding
4. Get no error

This test was rightly failing with your change.

[Chris Dumez]

(In reply to Yoav Weiss from comment #26)
> (In reply to Antti Koivisto from comment #24)
> > Comment on attachment 306391 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=306391&action=review
> > 
> > > LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01-expected.txt:5
> > > -PASS Script @type: unknown parameters 3 
> > > +FAIL Script @type: unknown parameters 3 assert_equals: expected "ÅÄÄżź" but got "\ufffd湿\ufffd"
> > 
> > This is not mentioned in ChangeLog. Why does this turn into FAIL?
> 
> I added that to the ChangeLog with an explanation.
> The test on
> https://github.com/w3c/web-platform-tests/blob/master/html/semantics/
> scripting-1/the-script-element/script-charset-01.html is using the same URL
> twice and expecting it to be decoded differently. That is not material to
> the test itself (and is failing in Chrome as well), so I'll PR a fix for
> that upstream.

This test is passing in Firefox and this shows a real bug AFAICT.

[Chris Dumez]

Rolled out in https://trac.webkit.org/r217289.

Will add more testing via Bug 171091.

[Yoav Weiss]

I have discussed that with Antti and avoiding a double download is the right behavior here. We consciously removed support for the "load the script twice with two different encodings" scenario, at it is an edge case. As you can see in previous patches on this issue, I tried to support that case by changing the encoding of the cached resource, but Antti rightfully observed that to be dangerous behavior which may lead to memory issues.

The MemoryCache is underspecced and we should definitely work to improve that, but in the mean time, comparing WebKit to Firefox, which reloads *every* script reference regardless of charset changes, is not an apples to apples comparison (no pun intended).

FWIW, Chrome also fails that edge-case test, for similar reasons.

[Antti Koivisto]

I'm still baffled why this seemingly simple logic (link preload encoding is decided lazily) can't be implemented without regressing anything. Double downloads seemed worse than the edge case bug that chrome also got wrong but that assert shows there are serious issues here.

[Yoav Weiss]

Hmm, looking at the updated repo after this patch got reverted, I no longer see the flakiness that I saw before. Maybe it was somehow fixed by https://bugs.webkit.org/show_bug.cgi?id=170747. Closing as WONTFIX for now, at least until we see the problem again.