RESOLVED FIXED256456
Fix bug in preload scanner with nested templates 
https://bugs.webkit.org/show_bug.cgi?id=256456
Summary Fix bug in preload scanner with nested templates
Ahmad Saleem
Reported 2023-05-08 04:10:24 PDT
Hi Team, I am not able to reproduce this bug in WebKit but we have similar code, so just wanted to raise whether it is something affecting us but the testcase (which I used is incorrect). Blink Commit - https://chromium.googlesource.com/chromium/src/+/168fe3ddcae333c7f2073e32b6f9a9f2b343be98 WebKit Source - https://searchfox.org/wubkat/source/Source/WebCore/html/parser/HTMLPreloadScanner.cpp#453 TestCase (tried but passing) - https://jsfiddle.net/wh4g0fpa/ Although we have same code, but without doing any changes, we are still passing. Just wanted to ask, whether we need to merge this and do change or if we are not failing, we don't need to do any change. Thanks!
Attachments
Add attachment proposed patch, testcase, etc.
Anne van Kesteren
Comment 1 2023-05-08 05:11:14 PDT
This seems like a change we should make. Since `<template><template></template>` m_templateCount will be 0, which would cause preloading to happen. Now I haven't been able to observe any preloads either using the inspector, but the logic is certainly wrong.
Ahmad Saleem
Comment 2 2023-05-08 05:31:18 PDT
(In reply to Anne van Kesteren from comment #1) > This seems like a change we should make. Since > > `<template><template></template>` m_templateCount will be 0, which would > cause preloading to happen. Now I haven't been able to observe any preloads > either using the inspector, but the logic is certainly wrong. Coming with layout testcase would be difficult for me. :-(
Anne van Kesteren
Comment 3 2023-05-08 07:31:40 PDT
https://github.com/web-platform-tests/wpt/pull/24521 added a lot of generated tests you could probably add this case to. I don't know if we have imported those yet though.
Ahmad Saleem
Comment 4 2023-05-08 14:46:07 PDT
I took help from Ryosuke and he told me to look into fast/preloader/ directory and I took 'image.html' test and then changed it into: Name - image-in-nested-template.html Test: <body> <script> if (window.testRunner) { testRunner.dumpAsText(); testRunner.dumpResourceResponseMIMETypes(); } </script> This test requires DumpRenderTree to see the log of what resources are loaded. <template> <template></template> <script src=resources/non-existant.js></script> <script>document.write("<plaintext>");</script> <img src=resources/image1.png> </template> _________________ <template> <template></template> <image xxx> </template> ________________ But when I am looking into output of text, it loads expected and does not show 'image' data. @rniwa & @anne - any input, before I do PR?
Anne van Kesteren
Comment 5 2023-05-08 23:45:18 PDT
It's not expected that it loads. That's the bug. Also, you should probably not rely on the HTML parser quirk that rewrites image to img in this test. Although perhaps that's worth testing as well, but in that case more that it does load when there are no encompassing template elements.
EWS
Comment 6 2023-05-08 23:46:46 PDT
Committed 263850@main (0434048daa53): <https://commits.webkit.org/263850@main> Reviewed commits have been landed. Closing PR #13608 and removing active labels.
Radar WebKit Bug Importer
Comment 7 2023-05-08 23:47:20 PDT
<rdar://problem/109081051>
Ahmad Saleem
Comment 8 2023-05-09 03:56:54 PDT
(In reply to Anne van Kesteren from comment #5) > It's not expected that it loads. That's the bug. Also, you should probably > not rely on the HTML parser quirk that rewrites image to img in this test. > Although perhaps that's worth testing as well, but in that case more that it > does load when there are no encompassing template elements. Yes - typing 'image' is incorrect. I confirmed locally as well that it does not preload, which is expected behavior.
Note You need to log in before you can comment on or make changes to this bug.