Bug 256456

Summary: Fix bug in preload scanner with nested templates
Product: WebKit Reporter: Ahmad Saleem <ahmad.saleem792>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: annevk, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   

Description Ahmad Saleem 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!
Comment 1 Anne van Kesteren 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.
Comment 2 Ahmad Saleem 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. :-(
Comment 3 Anne van Kesteren 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.
Comment 4 Ahmad Saleem 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?
Comment 5 Anne van Kesteren 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.
Comment 6 EWS 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.
Comment 7 Radar WebKit Bug Importer 2023-05-08 23:47:20 PDT
<rdar://problem/109081051>
Comment 8 Ahmad Saleem 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.