Bug 170122 - [link preload] Double downloads of preloaded content when it's in MemoryCache
Summary: [link preload] Double downloads of preloaded content when it's in MemoryCache
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 171091
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-27 11:58 PDT by Yoav Weiss
Modified: 2017-05-31 06:55 PDT (History)
9 users (show)

See Also:


Attachments
Patch (10.14 KB, patch)
2017-03-29 07:46 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (783.13 KB, application/zip)
2017-03-29 09:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (889.54 KB, application/zip)
2017-03-29 09:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.56 MB, application/zip)
2017-03-29 09:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (777.76 KB, application/zip)
2017-03-29 09:26 PDT, Build Bot
no flags Details
Patch (10.30 KB, patch)
2017-03-29 14:30 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (10.77 KB, patch)
2017-04-06 06:19 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (918.12 KB, application/zip)
2017-04-06 07:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (926.60 KB, application/zip)
2017-04-06 07:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.69 MB, application/zip)
2017-04-06 07:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (872.53 KB, application/zip)
2017-04-06 08:00 PDT, Build Bot
no flags Details
Patch (12.52 KB, patch)
2017-04-06 08:28 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (12.83 KB, patch)
2017-04-11 06:35 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yoav Weiss 2017-03-27 11:58:50 PDT
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.
Comment 1 Yoav Weiss 2017-03-29 07:46:47 PDT
Created attachment 305733 [details]
Patch
Comment 2 Yoav Weiss 2017-03-29 07:50:12 PDT
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?
Comment 3 Build Bot 2017-03-29 09:02:33 PDT
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
Comment 4 Build Bot 2017-03-29 09:02:36 PDT
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
Comment 5 Build Bot 2017-03-29 09:05:16 PDT
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
Comment 6 Build Bot 2017-03-29 09:05:19 PDT
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
Comment 7 Build Bot 2017-03-29 09:10:19 PDT
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
Comment 8 Build Bot 2017-03-29 09:10:22 PDT
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
Comment 9 Build Bot 2017-03-29 09:26:08 PDT
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
Comment 10 Build Bot 2017-03-29 09:26:10 PDT
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
Comment 11 Yoav Weiss 2017-03-29 14:30:44 PDT
Created attachment 305793 [details]
Patch
Comment 12 Antti Koivisto 2017-04-05 12:08:42 PDT
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.
Comment 13 Yoav Weiss 2017-04-06 06:19:15 PDT
Created attachment 306378 [details]
Patch
Comment 14 Build Bot 2017-04-06 07:33:08 PDT
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
Comment 15 Build Bot 2017-04-06 07:33:10 PDT
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
Comment 16 Build Bot 2017-04-06 07:41:48 PDT
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
Comment 17 Build Bot 2017-04-06 07:41:49 PDT
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
Comment 18 Build Bot 2017-04-06 07:46:01 PDT
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
Comment 19 Build Bot 2017-04-06 07:46:03 PDT
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
Comment 20 Build Bot 2017-04-06 08:00:35 PDT
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
Comment 21 Build Bot 2017-04-06 08:00:36 PDT
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
Comment 22 Yoav Weiss 2017-04-06 08:28:17 PDT
Created attachment 306391 [details]
Patch
Comment 23 Yoav Weiss 2017-04-10 15:10:25 PDT
The patch was updated to avoid encoding updates. PTAL?
Comment 24 Antti Koivisto 2017-04-11 04:49:13 PDT
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?
Comment 25 Yoav Weiss 2017-04-11 06:35:41 PDT
Created attachment 306809 [details]
Patch
Comment 26 Yoav Weiss 2017-04-11 06:37:51 PDT
(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.
Comment 27 Yoav Weiss 2017-04-11 07:42:54 PDT
Comment on attachment 306809 [details]
Patch

Thanks for reviewing! :)
Comment 28 Yoav Weiss 2017-04-11 07:56:37 PDT
Related upstream PR for WPT change: https://github.com/w3c/web-platform-tests/pull/5532
Comment 29 WebKit Commit Bot 2017-04-11 08:10:13 PDT
Comment on attachment 306809 [details]
Patch

Clearing flags on attachment: 306809

Committed r215229: <http://trac.webkit.org/changeset/215229>
Comment 30 WebKit Commit Bot 2017-04-11 08:10:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Chris Dumez 2017-05-23 12:03:37 PDT
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.
Comment 32 Chris Dumez 2017-05-23 12:30:22 PDT
(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.
Comment 33 Chris Dumez 2017-05-23 12:40:11 PDT
Rolled out in https://trac.webkit.org/r217289.

Will add more testing via Bug 171091.
Comment 34 Yoav Weiss 2017-05-24 00:01:08 PDT
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.
Comment 35 Antti Koivisto 2017-05-24 00:40:05 PDT
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.
Comment 36 Yoav Weiss 2017-05-31 06:55:40 PDT
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.