Closed Bug 1669664 Opened 4 years ago Closed 4 years ago

scripts in template tags are eagerly downloaded

Categories

(Core :: DOM: HTML Parser, defect)

Firefox 82
defect

Tracking

()

RESOLVED FIXED
83 Branch
Tracking Status
firefox83 --- fixed

People

(Reporter: web, Assigned: emilio)

References

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:82.0) Gecko/20100101 Firefox/82.0

Steps to reproduce:

Consider this document, in which the user attempts to use dynamic import() to load an app bundle, but falls back to loading several large polyfills and a legacy bundle for less-capable browsers.

The user makes use of the <template> element, as in her case, it is much more ergonomic than the DOM API (i.e. document.createElement("script")).

<template id="legacy-scripts">
  <script src="polyfill.js"></script>
  <script src="really-large-polyfill.js" onload="doThing()"></script>
  <script src="legacy/app.js"></script>
  <script src="dont-load-on-modern.js"></script>
</template>

<script nomodule>
  // for nomodule browsers, load the polyfills and the large script
  document.head.appendChild(document.getElementById('legacy-scripts').content.cloneNode(true));
  window.isNomoduleBrowser = true;
</script>

<script>
  if (!window.isNomoduleBrowser) {
    import('/app.js');
    window.supportsDynamicImport = true;
  }
</script>

<script>
  // if the previous script fails to parse, load the legacy bundle
  if (!window.supportsDynamicImport && !window.isNomoduleBrowser)
    document.head.appendChild(document.getElementById('legacy-scripts').content.cloneNode(true));
</script>

Actual results:

On Firefox 82, polyfill.js, really-large-polyfill.js, legacy/app.js, and dont-load-on-modern.js will all be downloaded, but not executed. In other words, if dont-load-on-modern.js contains the following:

console.log('loaded legacy scripts');
window.isLegacy = true;

Then the user will observe in the devtools network panel that all those scripts were downloaded, but they will not see 'loaded legacy scripts' in the console, and window.isLegacy will be undefined.

I'm not sure if Firefox also parses the scripts, or if it just downloads them.

Expected results:

The WHATWG spec is mum on whether or not scripts should be loaded

https://html.spec.whatwg.org/multipage/scripting.html#the-template-element

I argue that users should expect the contents of a <template> to be completely inert, having no effect on the state of the browser other than to be parsed and ready to stamp.

Neither Chrome 85 nor Safari 14 downloaded the scripts when I tested on macOS Mojave.

Without the eager-loading behaviour described in this bug, users that wish to preload those scripts could still do so with preload tags.

Component: Untriaged → DOM: Serializers
Product: Firefox → Core

Probably the speculative parser is doing its thing too eagerly.

Status: UNCONFIRMED → NEW
Component: DOM: Serializers → DOM: HTML Parser
Ever confirmed: true

I guess in order to make this 100% sound we should check the whole
template mode stack, but that seemed more expensive than what I'd really
like, and I think it's not likely to be an issue in practice (maybe we
can too-eagerly preload some images inside tables inside templates, or
something of that sort?).

Assignee: nobody → emilio
Status: NEW → ASSIGNED

To be clear... I can see a bunch of places where speculatively loading <template> contents may be useful... But I agree in this case the effect is quite terrible :)

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0191bd914f91
Don't speculatively load scripts, etc inside a template element. r=hsivonen
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/26039 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch
Upstream PR merged by moz-wptsync-bot
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/26054 for changes under testing/web-platform/tests

Backed out changeset 0191bd914f91 (bug 1669664) for test_login_item.html failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&tochange=26394defe88ff5fc9b555ba359c4c68247bd3905&searchStr=Linux%2C18.04%2Cx64%2Cdebug%2CMochitests%2Cwithout%2Ce10s%2Ctest-linux1804-64%2Fdebug-mochitest-chrome-1proc%2Cc2&fromchange=d368b4f167322c1b8e809f13f94e807af6009b64&selectedTaskRun=CGSmSML2QDKinpuKn_Qu8g.0

Backout link: https://hg.mozilla.org/mozilla-central/rev/9f3fcb6752b4e36779e395abef8362f0b0d14e7b

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=318060881&repo=autoland&lineNumber=2594

[task 2020-10-08T19:58:59.331Z] 19:58:59     INFO - TEST-START | browser/components/aboutlogins/tests/chrome/test_login_item.html
[task 2020-10-08T19:58:59.333Z] 19:58:59     INFO - GECKO(1780) | [1780, Main Thread] WARNING: 'NS_FAILED(targetPrincipal->GetAsciiOrigin(targetOrigin))', file /builds/worker/checkouts/gecko/toolkit/components/antitracking/AntiTrackingUtils.cpp:347
[task 2020-10-08T19:58:59.374Z] 19:58:59     INFO - GECKO(1780) | [1780, Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /builds/worker/checkouts/gecko/parser/html/nsHtml5StreamParser.cpp:1132
[task 2020-10-08T19:58:59.376Z] 19:58:59     INFO - GECKO(1780) | [1780, Main Thread] WARNING: 'NS_FAILED(targetPrincipal->GetAsciiOrigin(targetOrigin))', file /builds/worker/checkouts/gecko/toolkit/components/antitracking/AntiTrackingUtils.cpp:347
[task 2020-10-08T19:58:59.377Z] 19:58:59     INFO - GECKO(1780) | Couldn't convert chrome URL: chrome://mochitests/tests/SimpleTest/test.css
[task 2020-10-08T19:58:59.434Z] 19:58:59     INFO - GECKO(1780) | Couldn't convert chrome URL: chrome://mochitests/tests/SimpleTest/test.css
[task 2020-10-08T19:58:59.451Z] 19:58:59     INFO - GECKO(1780) | [1780, Main Thread] WARNING: NS_ENSURE_TRUE(presShell) failed: file /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowOuter.cpp:4278
[task 2020-10-08T19:58:59.455Z] 19:58:59     INFO - GECKO(1780) | [1780, Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /builds/worker/checkouts/gecko/parser/html/nsHtml5StreamParser.cpp:1132
[task 2020-10-08T19:58:59.644Z] 19:58:59     INFO - TEST-INFO | started process screentopng
[task 2020-10-08T19:58:59.886Z] 19:58:59     INFO - TEST-INFO | screentopng: exit 0
[task 2020-10-08T19:58:59.887Z] 19:58:59     INFO - Buffered messages logged at 19:58:59
[task 2020-10-08T19:58:59.888Z] 19:58:59     INFO - add_task | Entering test setup
[task 2020-10-08T19:58:59.888Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Check some templates found 
[task 2020-10-08T19:58:59.895Z] 19:58:59     INFO - add_task | Leaving test setup
[task 2020-10-08T19:58:59.895Z] 19:58:59     INFO - add_task | Entering test test_empty_item
[task 2020-10-08T19:58:59.895Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | loginItem exists 
[task 2020-10-08T19:58:59.895Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | origin should be blank 
[task 2020-10-08T19:58:59.895Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | username should be blank 
[task 2020-10-08T19:58:59.895Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | password should be blank 
[task 2020-10-08T19:58:59.895Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Real password input should be disconnected 
[task 2020-10-08T19:58:59.895Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | password display should be blank 
[task 2020-10-08T19:58:59.895Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Password display input should be visible 
[task 2020-10-08T19:58:59.895Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | time-created should be blank when undefined 
[task 2020-10-08T19:58:59.895Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | time-changed should be blank when undefined 
[task 2020-10-08T19:58:59.896Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | time-used should be blank when undefined 
[task 2020-10-08T19:58:59.896Z] 19:58:59     INFO - add_task | Leaving test test_empty_item
[task 2020-10-08T19:58:59.896Z] 19:58:59     INFO - add_task | Entering test test_set_login
[task 2020-10-08T19:58:59.896Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | loginItem should not be in 'edit' mode 
[task 2020-10-08T19:58:59.896Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | loginItem should not be in 'isNewLogin' mode 
[task 2020-10-08T19:58:59.897Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Origin input should be hidden when not in edit mode 
[task 2020-10-08T19:58:59.897Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Origin display link should be visible when not in edit mode 
[task 2020-10-08T19:58:59.897Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | origin should be populated 
[task 2020-10-08T19:58:59.897Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | username should be populated 
[task 2020-10-08T19:58:59.897Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | username field should have default placeholder when not editing 
[task 2020-10-08T19:58:59.897Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | password should be populated 
[task 2020-10-08T19:58:59.897Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Password shouldn't be exposed in @value 
[task 2020-10-08T19:58:59.897Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Real password input should be disconnected 
[task 2020-10-08T19:58:59.898Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | password display should be populated 
[task 2020-10-08T19:58:59.898Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Password display input should be visible 
[task 2020-10-08T19:58:59.899Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | time-created should be populated 
[task 2020-10-08T19:58:59.902Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | time-changed should be populated 
[task 2020-10-08T19:58:59.904Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | time-used should be populated 
[task 2020-10-08T19:58:59.904Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | The copy buttons should be visible when viewing a login 
[task 2020-10-08T19:58:59.906Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | loginItem should not be in 'edit' mode 
[task 2020-10-08T19:58:59.906Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | username field should have default placeholder when username is not present and not editing 
[task 2020-10-08T19:58:59.908Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | The copy-username-button should be disabled if there is no username 
[task 2020-10-08T19:58:59.909Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | there should be no placeholder id on the username input in edit mode 
[task 2020-10-08T19:58:59.909Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | there should be no placeholder on the username input in edit mode 
[task 2020-10-08T19:58:59.910Z] 19:58:59     INFO - add_task | Leaving test test_set_login
[task 2020-10-08T19:58:59.913Z] 19:58:59     INFO - add_task | Entering test test_update_breaches
[task 2020-10-08T19:58:59.914Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Breach alert should be visible 
[task 2020-10-08T19:58:59.914Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Breach alert link should be equal to the correspondingBreach.breachAlertURL. 
[task 2020-10-08T19:58:59.914Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Link in the text should point to the login origin 
[task 2020-10-08T19:58:59.915Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Vulnerable alert should not be visible on a non-vulnerable login. 
[task 2020-10-08T19:58:59.915Z] 19:58:59     INFO - add_task | Leaving test test_update_breaches
[task 2020-10-08T19:58:59.916Z] 19:58:59     INFO - add_task | Entering test test_breach_alert_is_correctly_hidden
[task 2020-10-08T19:58:59.916Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Breach alert should not be visible on login without an associated breach. 
[task 2020-10-08T19:58:59.917Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Vulnerable alert should not be visible on a non-vulnerable login. 
[task 2020-10-08T19:58:59.917Z] 19:58:59     INFO - add_task | Leaving test test_breach_alert_is_correctly_hidden
[task 2020-10-08T19:58:59.917Z] 19:58:59     INFO - add_task | Entering test test_update_vulnerable
[task 2020-10-08T19:58:59.918Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Breach alert should not be visible on login without an associated breach. 
[task 2020-10-08T19:58:59.919Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Vulnerable alert should be visible 
[task 2020-10-08T19:58:59.920Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Link in the text should point to the login origin 
[task 2020-10-08T19:58:59.921Z] 19:58:59     INFO - add_task | Leaving test test_update_vulnerable
[task 2020-10-08T19:58:59.922Z] 19:58:59     INFO - add_task | Entering test test_vulnerable_alert_is_correctly_hidden
[task 2020-10-08T19:58:59.923Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Breach alert should not be visible on login without an associated breach. 
[task 2020-10-08T19:58:59.924Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Vulnerable alert should not be visible on a non-vulnerable login. 
[task 2020-10-08T19:58:59.925Z] 19:58:59     INFO - add_task | Leaving test test_vulnerable_alert_is_correctly_hidden
[task 2020-10-08T19:58:59.926Z] 19:58:59     INFO - add_task | Entering test test_edit_login
[task 2020-10-08T19:58:59.928Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | loginItem should be in 'edit' mode 
[task 2020-10-08T19:58:59.929Z] 19:58:59     INFO - Buffered messages finished
[task 2020-10-08T19:58:59.930Z] 19:58:59     INFO - TEST-UNEXPECTED-FAIL | browser/components/aboutlogins/tests/chrome/test_login_item.html | edit button should be hidden in 'edit' mode 
[task 2020-10-08T19:58:59.931Z] 19:58:59     INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:417:16
[task 2020-10-08T19:58:59.932Z] 19:58:59     INFO - test_edit_login@chrome://mochitests/content/chrome/browser/components/aboutlogins/tests/chrome/test_login_item.html:191:5
[task 2020-10-08T19:58:59.933Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | loginItem should not be in 'isNewLogin' mode 
[task 2020-10-08T19:58:59.934Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Delete button should be enabled when editing a login 
[task 2020-10-08T19:58:59.936Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Origin input should be hidden in edit mode 
[task 2020-10-08T19:58:59.937Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Origin display link should be visible in edit mode 
[task 2020-10-08T19:58:59.938Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | origin should be populated 
[task 2020-10-08T19:58:59.939Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | username should be populated 
[task 2020-10-08T19:58:59.941Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | there should be no placeholder id on the username input in edit mode 
[task 2020-10-08T19:58:59.941Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | there should be no placeholder on the username input in edit mode 
[task 2020-10-08T19:58:59.942Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | password should be populated 
[task 2020-10-08T19:58:59.943Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | password display should be populated 
[task 2020-10-08T19:58:59.943Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | time-created should be populated 
[task 2020-10-08T19:58:59.944Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | time-changed should be populated 
[task 2020-10-08T19:58:59.945Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | time-used should be populated 
[task 2020-10-08T19:58:59.945Z] 19:58:59     INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-10-08T19:58:59.946Z] 19:58:59     INFO - TEST-UNEXPECTED-FAIL | browser/components/aboutlogins/tests/chrome/test_login_item.html | The copy buttons should be hidden when editing a login 
[task 2020-10-08T19:58:59.946Z] 19:58:59     INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:417:16
[task 2020-10-08T19:58:59.946Z] 19:58:59     INFO - test_edit_login@chrome://mochitests/content/chrome/browser/components/aboutlogins/tests/chrome/test_login_item.html:211:5
[task 2020-10-08T19:58:59.947Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | event should include guid 
[task 2020-10-08T19:58:59.948Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | event should include origin 
[task 2020-10-08T19:58:59.949Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | event should include new username 
[task 2020-10-08T19:58:59.949Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | event should include new password 
[task 2020-10-08T19:58:59.950Z] 19:58:59     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_item.html | Clicking the .save-changes-button should dispatch the AboutLoginsUpdateLogin event 
[task 2020-10-08T19:58:59.951Z] 19:58:59     INFO - add_task | Leaving test test_edit_login
...
Status: RESOLVED → REOPENED
Flags: needinfo?(emilio)
Resolution: FIXED → ---
Target Milestone: 83 Branch → ---
Upstream PR merged by moz-wptsync-bot

These were relying on the <link> elements being preloaded inside the
<template> which is something we are going to try changing.

In order to ensure the stylesheets apply synchronously and not cause
test failures, preload them explicitly when importing them from the page
to the test.

Flags: needinfo?(emilio)

:emilio, I left a comment/question on the patch which I'd like to understand better before reviewing.

Flags: needinfo?(emilio)

Thanks, I had missed it. Replied, let me know if you want to chat sync about how this all worked :)

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dce7c0a3f2fd
Explicitly preload stylesheets inside templates in about:logins tests. r=sfoster
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e551d2c638d4
Don't speculatively load scripts, etc inside a template element. r=hsivonen
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/26216 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: