Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web compat: don't run async scripts between adjacent sync <script>s #6761

Closed
zcorpan opened this issue Jun 11, 2021 · 12 comments
Closed

Web compat: don't run async scripts between adjacent sync <script>s #6761

zcorpan opened this issue Jun 11, 2021 · 12 comments
Labels
compat Standard is not web compatible or proprietary feature needs standardizing topic: parser topic: script

Comments

@zcorpan
Copy link
Member

zcorpan commented Jun 11, 2021

See https://bugs.chromium.org/p/chromium/issues/detail?id=1197376

Apparently chromium was changed to not run async scripts between two sync <script>s.

Do other browsers also do this?

As far as I know the spec doesn't say anything about this, but if it's necessary for web compat it probably should.

cc @whatwg/html-parser @richard-townsend-arm @mfreed7

@zcorpan
Copy link
Member Author

zcorpan commented Jun 11, 2021

If there's a network packet boundary right after </script>, does chromium wait for more data to come through?

@hsivonen
Copy link
Member

The Chromium issue superficially looks like they added an new network buffer boundary dependency. That would be very disappointing. Am I misreading?

@richard-townsend-arm
Copy link

richard-townsend-arm commented Jun 11, 2021

So the context of this is that I'm working on some new HTML parser scheduling heuristics, one of the behaviours is that it used to yield after processing every <script> tag (it did this so that layout/paint could happen faster). Unfortunately, this broke some poorly coded websites that use <script async>.

The one that the issue mentions was doing something like this:

<script>
window.vanilla.addJs({"id":"main-js","src":"\/\/vanilla.futurecdn.net\/tomshardware\/267900\/media\/js\/main.min.js"}, null, 0)
</script>  <script>
window.ffte = {"site":"tomshardware", (snip)
</script>

What was happening was that the site author assumed that the second <script> tag would run before the one added in the first (<script> tags added to the document by other scripts are async), but under Chromium's proposed new parser that wasn't necessarily the case. Of course - you're not supposed to assume anything about when async scripts are executed, but this was breaking some sites so we opted for this solution to try and make the new parser act a bit more like the old one while we continue experimenting. There is a follow up issue open to remove the behaviour.

So TL;DR - I don't think this is a spec issue, the new parser actually gets a bit closer to what the spec says, we just have to find the sites out there that break and get them fixed.

@hsivonen
Copy link
Member

I would much prefer contacting prominent sites that do this over introducing buffer boundary dependencies in the majority-market-share engine for all sites (which risks even more Web authors taking dependencies on the behavior).

@zcorpan zcorpan added the compat Standard is not web compatible or proprietary feature needs standardizing label Jun 14, 2021
@richard-townsend-arm
Copy link

richard-townsend-arm commented Jun 15, 2021

So the calculus IMHO is that the new parser has to perform better than the old one before it's worth breaking those sites - we're not quite there yet!

@mfreed7
Copy link
Contributor

mfreed7 commented Jun 16, 2021

I would much prefer contacting prominent sites that do this over introducing buffer boundary dependencies in the majority-market-share engine for all sites (which risks even more Web authors taking dependencies on the behavior).

Just to chime in a bit here - we would obviously prefer that as well. There are a few problems:

  1. We don't know what sites will break. We have the few examples that were reported as bugs from Canary/Dev channel Chrome. This usually means there will be more. Since this seemed like a pattern (assume no async scripts between consecutive sync scripts), it felt like a good heuristic fix for potentially a class of failures.
  2. The spec for async scripts is purposely vague about when these scripts should run. "When they're ready" is up to the UA. Any and all dependencies on the timing of async scripts should be considered to be site bugs. To the extent that we can slowly change this timing, it will both a) ferret out more of these dependencies, and b) reduce (not increase) the level of dependence on a single browsers idiosyncratic behavior. Both are good for interop. So we thought it would still be a good idea to change the timing of async scripts in general (with the new parser code we're working on), but leave the existing timing in place for classes of failures we observe. Without being able to do that, we can never land the new code, and all existing dependencies will stick.
  3. We already don't plan to keep this behavior.

Side note, which @richard-townsend-arm can correct if wrong, the current Chromium code would wait to execute async scripts if a network boundary happened to come right after a single synchronous script. Which isn't great, of course.

@zcorpan
Copy link
Member Author

zcorpan commented Jun 16, 2021

Side note, which @richard-townsend-arm can correct if wrong, the current Chromium code would wait to execute async scripts if a network boundary happened to come right after a single synchronous script. Which isn't great, of course.

OK. I think it is good for predictable behavior (compared to sometimes blocking async scripts). I believe the spec tries its best to avoid different behavior depending on where network boundaries fall.

@zcorpan
Copy link
Member Author

zcorpan commented May 30, 2024

https://chromium-review.googlesource.com/c/chromium/src/+/3033184 was merged. So does Chromium now match the spec?

@mfreed7
Copy link
Contributor

mfreed7 commented May 30, 2024

https://chromium-review.googlesource.com/c/chromium/src/+/3033184 was merged. So does Chromium now match the spec?

Sorry, which part of the spec were we previously not matching? This whole thread is about when async scripts run, and AFAIK that timing is purposely not standardized.

That CL did land, but there have been others since. The behavior should be roughly the same as that CL modulo other heuristic changes. E.g. instead of token counts we’re now using wall time.

@zcorpan
Copy link
Member Author

zcorpan commented Jun 3, 2024

Right, the timing is not specified. But also, the spec doesn't try to prevent async scripts to run right after a sync script. If there's a web compat reason to do so, it would be good to specify something to that effect.

@mfreed7
Copy link
Contributor

mfreed7 commented Jun 4, 2024

But also, the spec doesn't try to prevent async scripts to run right after a sync script. If there's a web compat reason to do so, it would be good to specify something to that effect.

I don't think Blink tries to prevent async scripts running after sync scripts either, though. (On the contrary, after a sync script, it gets more likely that an async script runs soon.)

I don't believe there's a web compat reason to make changes here. When we previously changed this behavior a few years ago, we did run into compat problems, but all were treated as site bugs that eventually got fixed by site owners. Since the timing of async scripts is deliberately non-specified, the best we can do for interop is make it hard to predict, so that it doesn't take inadvertent dependencies in the ecosystem.

@zcorpan
Copy link
Member Author

zcorpan commented Jun 10, 2024

OK, thanks. Let's close this without changing the spec then. 🙂

@zcorpan zcorpan closed this as completed Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compat Standard is not web compatible or proprietary feature needs standardizing topic: parser topic: script
Development

No branches or pull requests

4 participants