Bug 169274 – [link preload] Double downloads of preloaded CSS

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

[Yoav Weiss]

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

[Yoav Weiss]

Created attachment 303647 [details]
Patch

[Yoav Weiss]

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.

[Antti Koivisto]

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>

[Yoav Weiss]

(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?

[Antti Koivisto]

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.

[Yoav Weiss]

Created attachment 303664 [details]
Patch

[Yoav Weiss]

(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.

[Yoav Weiss]

(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.

[Antti Koivisto]

Comment on attachment 303664 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResource.h:235
> -    void setLinkPreload() { m_isLinkPreload = true; }
> +    enum class IsLinkPreload { LinkPreload, NotLinkPreload };
> +    void setLinkPreload(IsLinkPreload isLinkPreload) { m_isLinkPreload = ((isLinkPreload == IsLinkPreload::LinkPreload) ? true : false); }

Enum seem unnecessary. Maybe just setIsLinkPreload(bool)?

[Antti Koivisto]

> 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.

[Yoav Weiss]

Created attachment 303689 [details]
Patch

[Yoav Weiss]

(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.

[Yoav Weiss]

Looks like test failures are related. Looking into it.

[Build Bot]

Comment on attachment 303689 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01.html

[Build Bot]

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

[Build Bot]

Comment on attachment 303689 [details]
Patch

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

New failing tests:
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-001.html

[Build Bot]

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

[Build Bot]

Comment on attachment 303689 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/syntax/parsing-html-fragments/the-input-byte-stream-009.html
http/tests/preload/preload-encoding.php
fast/loader/cache-encoding.html
imported/w3c/web-platform-tests/html/syntax/parsing-html-fragments/the-input-byte-stream-015.html
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-016.html

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Yoav Weiss]

Created attachment 303768 [details]
Patch

[Build Bot]

Comment on attachment 303768 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01.html

[Build Bot]

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

[Build Bot]

Comment on attachment 303768 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01.html

[Build Bot]

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

[Build Bot]

Comment on attachment 303768 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01.html
fast/loader/cache-encoding.html
http/tests/preload/preload-encoding.php

[Build Bot]

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

[Yoav Weiss]

Created attachment 303786 [details]
Patch

[Build Bot]

Comment on attachment 303786 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/script-charset-01.html
fast/loader/cache-encoding.html
http/tests/preload/preload-encoding.php

[Build Bot]

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

[Yoav Weiss]

Created attachment 303806 [details]
Patch

[Yoav Weiss]

Antti - The tests pass now without reloading CachedResources and performing charset conversions on them when needed. PTAL?

[Yoav Weiss]

Created attachment 303948 [details]
Patch

[Antti Koivisto]

Comment on attachment 303948 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResource.h:235
> +    bool isUnknownCharset() { return m_unknownCharset; }

hasUnknownEncoding()/m_hasUnknownEncoding would match setEncoding and WebKit style better.

[Yoav Weiss]

Created attachment 303978 [details]
Patch

[Yoav Weiss]

(In reply to comment #35)
> Comment on attachment 303948 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303948&action=review
> 
> > Source/WebCore/loader/cache/CachedResource.h:235
> > +    bool isUnknownCharset() { return m_unknownCharset; }
> 
> hasUnknownEncoding()/m_hasUnknownEncoding would match setEncoding and WebKit
> style better.

changed

[Yoav Weiss]

Comment on attachment 303978 [details]
Patch

Thanks for reviewing! :)

[WebKit Commit Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 303978 [details]
Patch

Clearing flags on attachment: 303978

Committed r213672: <http://trac.webkit.org/changeset/213672>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.