|
Description
Yoav Weiss
2017-03-07 05:22:49 PST
Created attachment 303647 [details]
Patch
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. Created attachment 303664 [details]
Patch
(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. 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)? > 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.
Created attachment 303689 [details]
Patch
(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. Looks like test failures are related. Looking into it. 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 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
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 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
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 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 303768 [details]
Patch
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 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
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 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
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 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 303786 [details]
Patch
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 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
Created attachment 303806 [details]
Patch
Antti - The tests pass now without reloading CachedResources and performing charset conversions on them when needed. PTAL? Created attachment 303948 [details]
Patch
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. Created attachment 303978 [details]
Patch
(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 Comment on attachment 303978 [details]
Patch
Thanks for reviewing! :)
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. Comment on attachment 303978 [details] Patch Clearing flags on attachment: 303978 Committed r213672: <http://trac.webkit.org/changeset/213672> All reviewed patches have been landed. Closing bug. |