Following STP's exposure of preload as an experimental feature, I got a report[1] saying that we're double-downloading CSS in some cases.
Looking into that, it seems like the fact that header-based preload requests are sent out before the document's charset was determined is causing issues in matching of resources coming out of the MemoryCache (in CachedResourceLoader::determineRevalidationPolicy).
Is it possible to avoid determining the charset of resources until later on? Requiring that charset at request time is difficult for preloaded resources, unless they are deliberately delayed.
[1] https://twitter.com/voxpelli/status/834485581132480512
I've uploaded a patch which, in case of a preloaded resource, sets the encoding when taking the resource out of MemoryCache instead of reloading it. Let me know what you think of the approach.
Comment on attachment 303647[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=303647&action=review> Source/WebCore/loader/cache/CachedResourceLoader.cpp:929
> + if (textDecoder && !textDecoder->hasEqualEncodingForCharset(cachedResourceRequest.charset())) {
> + if (!existingResource->isLinkPreload())
> + return Reload;
> + existingResource->setEncoding(cachedResourceRequest.charset());
> + }
What happens if there are multiple requests for the same link-preloaded resource with different charsets?
<link rel="preload" href="foo.js" as="script">
...
<script src="foo.js" charset=utf-8></script>
<script src="foo.js" charset=latin1></script>
(In reply to comment #3)
> Comment on attachment 303647[details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303647&action=review
>
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:929
> > + if (textDecoder && !textDecoder->hasEqualEncodingForCharset(cachedResourceRequest.charset())) {
> > + if (!existingResource->isLinkPreload())
> > + return Reload;
> > + existingResource->setEncoding(cachedResourceRequest.charset());
> > + }
>
> What happens if there are multiple requests for the same link-preloaded
> resource with different charsets?
>
> <link rel="preload" href="foo.js" as="script">
> ...
> <script src="foo.js" charset=utf-8></script>
> <script src="foo.js" charset=latin1></script>
I agree that in that case, we'll enforce two different encodings on the same resource, and that can result in us sending the wrongly decoded text to the second resource.
Maybe we also need to discard any cached decoded text in the related CachedResources?
Maybe you can track if the preload has been already been used and if so require full charset match (or reload)? Or maybe just reset the link-preload bit on the first match?
A better, more complicated solution would be to support multiple charsets on CachedResource level. Clients would provide charset as a parameter on all accesses to decoded strings.
(In reply to comment #5)
> Maybe you can track if the preload has been already been used and if so
> require full charset match (or reload)? Or maybe just reset the link-preload
> bit on the first match?
Yeah, took that route and turned off the flag when reusing a charset mismatch.
>
> A better, more complicated solution would be to support multiple charsets on
> CachedResource level. Clients would provide charset as a parameter on all
> accesses to decoded strings.
(In reply to comment #7)
> (In reply to comment #5)
> > Maybe you can track if the preload has been already been used and if so
> > require full charset match (or reload)? Or maybe just reset the link-preload
> > bit on the first match?
>
> Yeah, took that route and turned off the flag when reusing a charset
> mismatch.
>
> >
> > A better, more complicated solution would be to support multiple charsets on
> > CachedResource level. Clients would provide charset as a parameter on all
> > accesses to decoded strings.
I'm thinking about taking the second route, as it seems cleaner (thinking about it some more, turning off the flag in the charset case seems arbitrary).
Looking at the various CachedResource types it seems like:
* CachedCSSStyleSheet never reuses decoded text.
* Scripts do reuse previously decoded text.
* SVG never reuses decoded text.
* XSLT do reuse previously decoded text.
So, I think that simply getting rid of the internal representation of the decoded text whenever there's an encoding change can be enough to maintain charset correctness.
Let me know what you think.
> So, I think that simply getting rid of the internal representation of the
> decoded text whenever there's an encoding change can be enough to maintain
> charset correctness.
>
> Let me know what you think.
Yeah, that would be nice but just fixing the immediate problem is good too. You can try that out next.
(In reply to comment #10)
> > So, I think that simply getting rid of the internal representation of the
> > decoded text whenever there's an encoding change can be enough to maintain
> > charset correctness.
> >
> > Let me know what you think.
>
> Yeah, that would be nice but just fixing the immediate problem is good too.
> You can try that out next.
I uploaded a new patch with that approach. Let me know if that works, or I should revert to the LinkPreload flag approach.
Created attachment 303695[details]
Archive of layout-test-results from ews102 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 303696[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
Created attachment 303701[details]
Archive of layout-test-results from ews116 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 303689[details]
Patch
Attachment 303689[details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3260809
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-009.html
imported/w3c/web-platform-tests/html/syntax/parsing-html-fragments/the-input-byte-stream-018.html
Created attachment 303705[details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 303779[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
Created attachment 303780[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
Created attachment 303781[details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 303793[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
The commit-queue encountered the following flaky tests while processing attachment 303978[details]:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-styles.html bug 168317 (author: graouts@apple.com)
The commit-queue is continuing to process your patch.
2017-03-07 06:18 PST, Yoav Weiss
2017-03-07 09:21 PST, Yoav Weiss
2017-03-07 11:34 PST, Yoav Weiss
2017-03-07 12:14 PST, Build Bot
2017-03-07 12:19 PST, Build Bot
2017-03-07 12:40 PST, Build Bot
2017-03-07 12:55 PST, Build Bot
2017-03-07 20:31 PST, Yoav Weiss
2017-03-07 21:35 PST, Build Bot
2017-03-07 21:47 PST, Build Bot
2017-03-07 22:01 PST, Build Bot
2017-03-07 23:37 PST, Yoav Weiss
2017-03-08 01:06 PST, Build Bot
2017-03-08 03:03 PST, Yoav Weiss
2017-03-09 12:23 PST, Yoav Weiss
2017-03-09 13:29 PST, Yoav Weiss