From 45d8e094975c825a9be2be947d4b75ee1b37dd9b Mon Sep 17 00:00:00 2001
From: Benjamin Franzke <ben@bnf.dev>
Date: Tue, 18 Jun 2024 05:59:35 +0200
Subject: [PATCH] [BUGFIX] Avoid race condition in DocumentService.ready()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Improve document-service responsiveness by relying on
`DOMContentLoaded` and `document.readyState` >= `interactive`.
1) Handle non-loading state as "ready" to avoid waiting for `complete`.
`document.readyState` has three states:
* `loading`
The document is still loading.
* `interactive`
The document has finished loading and the document
has been parsed but sub-resources such as scripts, images,
stylesheets and frames are still loading. The state indicates
that the DOMContentLoaded event is about to fire.
* `complete`
The document and all sub-resources have finished loading.
The state indicates that the load event is about to fire.
If DocumentService.ready was called in "interactive" state we have
been skipping this state as we only considered `complete` to be
the "ready" state in this case.
This is wrong as we actually wait for the DOMContentLoaded if we
are launched in the initial readyState (`loading`), that means
an initial `interactive` must be understood as:
`DOMContentLoaded` has already been fired as state is `interactive`
We should actually avoid waiting for `complete` entirely, as document
readyState `interactive` means "ready" in terms of document parsing.
We're not interested in images or async scripts that are still
loading and want event listeners to be registered as early as
possible.
With this improvement applied we can also drop the (now) unneeded
`load` event-listener, as we are no longer skipping interactive
state (this made the `load`-listener necessary previously).
Note that the `load` workaround was previously needed for IE<=10 as
those versions set `document.readyState` to `interactive` prior to
the `DOMContentLoaded` event. This applied workaround becomes a race
condition once `load` never happens and that is the case with a
recent google chrome regression which causes iframe documents to
never "complete": https://issues.chromium.org/issues/347724924
2) Drop the timeout logic. It is no longer needed now that we only need
to wait for `DOMContentLoaded` – which will always fire (or already
fired) – instead of the `load` event.
3) Avoid creation of new promises for every invocation of the
ready-method. A promise can be reused without side effects by
multiple consumers. This avoids creating a lot of event listeners
(and previously timers).
4) Remove unneeded document and window references.
Releases: main, 12.4, 11.5
Resolves: #104135
Related: #104139
Change-Id: I42c86961405f8a5c346c17ea429a288a85d58f8a
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/84776
Tested-by: core-ci <typo3@b13.com>
Reviewed-by: Benjamin Franzke <ben@bnf.dev>
Tested-by: Benjamin Franzke <ben@bnf.dev>
---
.../Public/TypeScript/DocumentService.ts | 42 ++++---------------
.../Public/JavaScript/DocumentService.js | 2 +-
2 files changed, 10 insertions(+), 34 deletions(-)
diff --git a/Build/Sources/TypeScript/core/Resources/Public/TypeScript/DocumentService.ts b/Build/Sources/TypeScript/core/Resources/Public/TypeScript/DocumentService.ts
index 694c794fac82..80b9f5f655f7 100644
--- a/Build/Sources/TypeScript/core/Resources/Public/TypeScript/DocumentService.ts
+++ b/Build/Sources/TypeScript/core/Resources/Public/TypeScript/DocumentService.ts
@@ -16,42 +16,18 @@
* @exports TYPO3/CMS/Core/DocumentService
*/
class DocumentService {
- private readonly windowRef: Window;
- private readonly documentRef: Document;
+ private promise: Promise<Document> = null;
- /**
- * @param {Window} windowRef
- * @param {Document} documentRef
- */
- constructor(windowRef: Window = window, documentRef: Document = document) {
- this.windowRef = windowRef;
- this.documentRef = documentRef;
+ public ready(): Promise<Document> {
+ return this.promise ?? (this.promise = this.createPromise());
}
- ready(): Promise<Document> {
- return new Promise<Document>((resolve: Function, reject: Function) => {
- if (this.documentRef.readyState === 'complete') {
- resolve(this.documentRef);
- } else {
- // timeout & reject after 30 seconds
- const timer = setTimeout((): void => {
- clearListeners();
- reject(this.documentRef);
- }, 30000);
- const clearListeners = (): void => {
- this.windowRef.removeEventListener('load', delegate);
- this.documentRef.removeEventListener('DOMContentLoaded', delegate);
- };
- const delegate = (): void => {
- clearListeners();
- clearTimeout(timer);
- resolve(this.documentRef);
- };
- this.windowRef.addEventListener('load', delegate);
- this.documentRef.addEventListener('DOMContentLoaded', delegate);
- }
-
- });
+ private async createPromise(): Promise<Document> {
+ if (document.readyState !== 'loading') {
+ return document;
+ }
+ await new Promise<void>(resolve => document.addEventListener('DOMContentLoaded', () => resolve(), { once: true }));
+ return document;
}
}
diff --git a/typo3/sysext/core/Resources/Public/JavaScript/DocumentService.js b/typo3/sysext/core/Resources/Public/JavaScript/DocumentService.js
index 1a5be706fbe1..fbcdfdec606d 100644
--- a/typo3/sysext/core/Resources/Public/JavaScript/DocumentService.js
+++ b/typo3/sysext/core/Resources/Public/JavaScript/DocumentService.js
@@ -10,4 +10,4 @@
*
* The TYPO3 project - inspiring people to share!
*/
-define(["require","exports"],(function(e,t){"use strict";return new class{constructor(e=window,t=document){this.windowRef=e,this.documentRef=t}ready(){return new Promise((e,t)=>{if("complete"===this.documentRef.readyState)e(this.documentRef);else{const n=setTimeout(()=>{o(),t(this.documentRef)},3e4),o=()=>{this.windowRef.removeEventListener("load",i),this.documentRef.removeEventListener("DOMContentLoaded",i)},i=()=>{o(),clearTimeout(n),e(this.documentRef)};this.windowRef.addEventListener("load",i),this.documentRef.addEventListener("DOMContentLoaded",i)}})}}}));
\ No newline at end of file
+define(["require","exports"],(function(e,t){"use strict";return new class{constructor(){this.promise=null}ready(){var e;return null!==(e=this.promise)&&void 0!==e?e:this.promise=this.createPromise()}async createPromise(){return"loading"!==document.readyState||await new Promise(e=>document.addEventListener("DOMContentLoaded",()=>e(),{once:!0})),document}}}));
\ No newline at end of file
--
GitLab