From bd036b5b6887be430fdd2d56f3175e79baa6b6ad Mon Sep 17 00:00:00 2001 From: Tilman Vatteroth Date: Wed, 13 Oct 2021 22:07:46 +0200 Subject: [PATCH] Refactor iframe load callback to fix race condition (#1536) * Refactor iframe load callback to fix race condition Signed-off-by: Tilman Vatteroth --- ...render-page-url-on-iframe-load-callback.ts | 51 +++++++++++++++++++ .../renderer-pane/hooks/use-on-iframe-load.ts | 40 --------------- .../renderer-pane/render-iframe.tsx | 37 +++++++++----- 3 files changed, 75 insertions(+), 53 deletions(-) create mode 100644 src/components/editor-page/renderer-pane/hooks/use-force-render-page-url-on-iframe-load-callback.ts delete mode 100644 src/components/editor-page/renderer-pane/hooks/use-on-iframe-load.ts diff --git a/src/components/editor-page/renderer-pane/hooks/use-force-render-page-url-on-iframe-load-callback.ts b/src/components/editor-page/renderer-pane/hooks/use-force-render-page-url-on-iframe-load-callback.ts new file mode 100644 index 000000000..0d90c16c6 --- /dev/null +++ b/src/components/editor-page/renderer-pane/hooks/use-force-render-page-url-on-iframe-load-callback.ts @@ -0,0 +1,51 @@ +/* + * SPDX-FileCopyrightText: 2021 The HedgeDoc developers (see AUTHORS file) + * + * SPDX-License-Identifier: AGPL-3.0-only + */ + +import { RefObject, useCallback, useEffect, useMemo, useRef } from 'react' +import { Logger } from '../../../../utils/logger' + +const log = new Logger('IframeLoader') + +/** + * Generates a callback for a iframe load handler, that enforces a given URL if frame navigates away. + * + * @param iFrameReference A reference to the iframe react dom element. + * @param rendererOrigin The base url that should be enforced. + * @param onNavigateAway An optional callback that is executed when the iframe leaves the enforced URL. + */ +export const useForceRenderPageUrlOnIframeLoadCallback = ( + iFrameReference: RefObject, + rendererOrigin: string, + onNavigateAway?: () => void +): (() => void) => { + const forcedUrl = useMemo(() => `${rendererOrigin}render`, [rendererOrigin]) + const redirectionInProgress = useRef(false) + + const loadCallback = useCallback(() => { + const frame = iFrameReference.current + + if (!frame) { + log.debug('No frame in reference') + return + } + + if (redirectionInProgress.current) { + redirectionInProgress.current = false + log.debug('Redirect complete') + } else { + log.warn(`Navigated away from unknown URL. Forcing back to ${forcedUrl}`) + onNavigateAway?.() + redirectionInProgress.current = true + frame.src = forcedUrl + } + }, [iFrameReference, onNavigateAway, forcedUrl]) + + useEffect(() => { + loadCallback() + }, [loadCallback]) + + return loadCallback +} diff --git a/src/components/editor-page/renderer-pane/hooks/use-on-iframe-load.ts b/src/components/editor-page/renderer-pane/hooks/use-on-iframe-load.ts deleted file mode 100644 index 443e1c637..000000000 --- a/src/components/editor-page/renderer-pane/hooks/use-on-iframe-load.ts +++ /dev/null @@ -1,40 +0,0 @@ -/* - * SPDX-FileCopyrightText: 2021 The HedgeDoc developers (see AUTHORS file) - * - * SPDX-License-Identifier: AGPL-3.0-only - */ - -import { RefObject, useCallback, useRef } from 'react' -import { EditorToRendererCommunicator } from '../../../render-page/window-post-message-communicator/editor-to-renderer-communicator' -import { Logger } from '../../../../utils/logger' - -const log = new Logger('IframeLoader') - -export const useOnIframeLoad = ( - frameReference: RefObject, - iframeCommunicator: EditorToRendererCommunicator, - rendererOrigin: string, - renderPageUrl: string, - onNavigateAway: () => void -): (() => void) => { - const sendToRenderPage = useRef(true) - - return useCallback(() => { - const frame = frameReference.current - if (!frame || !frame.contentWindow) { - iframeCommunicator.unsetMessageTarget() - return - } - - if (sendToRenderPage.current) { - iframeCommunicator.setMessageTarget(frame.contentWindow, rendererOrigin) - sendToRenderPage.current = false - return - } else { - onNavigateAway() - log.error('Navigated away from unknown URL') - frame.src = renderPageUrl - sendToRenderPage.current = true - } - }, [frameReference, iframeCommunicator, onNavigateAway, renderPageUrl, rendererOrigin]) -} diff --git a/src/components/editor-page/renderer-pane/render-iframe.tsx b/src/components/editor-page/renderer-pane/render-iframe.tsx index 05cf7850a..509a41ffd 100644 --- a/src/components/editor-page/renderer-pane/render-iframe.tsx +++ b/src/components/editor-page/renderer-pane/render-iframe.tsx @@ -4,7 +4,6 @@ * SPDX-License-Identifier: AGPL-3.0-only */ import React, { Fragment, useCallback, useEffect, useRef, useState } from 'react' -import { useApplicationState } from '../../../hooks/common/use-application-state' import { isTestMode } from '../../../utils/test-modes' import { RendererProps } from '../../render-page/markdown-document' import { @@ -16,7 +15,7 @@ import { SetScrollStateMessage } from '../../render-page/window-post-message-communicator/rendering-message' import { useEditorToRendererCommunicator } from '../render-context/editor-to-renderer-communicator-context-provider' -import { useOnIframeLoad } from './hooks/use-on-iframe-load' +import { useForceRenderPageUrlOnIframeLoadCallback } from './hooks/use-force-render-page-url-on-iframe-load-callback' import { CommunicatorImageLightbox } from './communicator-image-lightbox' import { setRendererStatus } from '../../../redux/renderer-status/methods' import { useEditorReceiveHandler } from '../../render-page/window-post-message-communicator/hooks/use-editor-receive-handler' @@ -24,6 +23,8 @@ import { useIsRendererReady } from '../../render-page/window-post-message-commun import { useSendDarkModeStatusToRenderer } from './hooks/use-send-dark-mode-status-to-renderer' import { useSendMarkdownToRenderer } from './hooks/use-send-markdown-to-renderer' import { useSendScrollState } from './hooks/use-send-scroll-state' +import { useApplicationState } from '../../../hooks/common/use-application-state' +import { Logger } from '../../../utils/logger' export interface RenderIframeProps extends RendererProps { rendererType: RendererType @@ -31,6 +32,8 @@ export interface RenderIframeProps extends RendererProps { frameClasses?: string } +const log = new Logger('RenderIframe') + export const RenderIframe: React.FC = ({ markdownContent, onTaskCheckedChange, @@ -44,17 +47,14 @@ export const RenderIframe: React.FC = ({ }) => { const frameReference = useRef(null) const rendererOrigin = useApplicationState((state) => state.config.iframeCommunication.rendererOrigin) - const renderPageUrl = `${rendererOrigin}render` - const resetRendererReady = useCallback(() => setRendererStatus(false), []) const iframeCommunicator = useEditorToRendererCommunicator() + const resetRendererReady = useCallback(() => { + log.debug('Reset render status') + iframeCommunicator.unsetMessageTarget() + setRendererStatus(false) + }, [iframeCommunicator]) const rendererReady = useIsRendererReady() - const onIframeLoad = useOnIframeLoad( - frameReference, - iframeCommunicator, - rendererOrigin, - renderPageUrl, - resetRendererReady - ) + const onIframeLoad = useForceRenderPageUrlOnIframeLoadCallback(frameReference, rendererOrigin, resetRendererReady) const [frameHeight, setFrameHeight] = useState(0) useEffect( @@ -99,6 +99,18 @@ export const RenderIframe: React.FC = ({ useEditorReceiveHandler( CommunicationMessageType.RENDERER_READY, useCallback(() => { + const frame = frameReference.current + if (!frame) { + log.error('Load triggered without frame in ref') + return + } + const otherWindow = frame.contentWindow + if (!otherWindow) { + log.error('Load triggered without content window') + return + } + log.debug(`Set iframecommunicator window with origin ${rendererOrigin}`) + iframeCommunicator.setMessageTarget(otherWindow, rendererOrigin) iframeCommunicator.enableCommunication() iframeCommunicator.sendMessageToOtherSide({ type: CommunicationMessageType.SET_BASE_CONFIGURATION, @@ -108,7 +120,7 @@ export const RenderIframe: React.FC = ({ } }) setRendererStatus(true) - }, [iframeCommunicator, rendererType]) + }, [iframeCommunicator, rendererOrigin, rendererType]) ) useSendScrollState(scrollState) @@ -123,7 +135,6 @@ export const RenderIframe: React.FC = ({ data-cy={'documentIframe'} onLoad={onIframeLoad} title='render' - src={renderPageUrl} {...(isTestMode() ? {} : { sandbox: 'allow-downloads allow-same-origin allow-scripts allow-popups' })} ref={frameReference} className={`border-0 ${frameClasses ?? ''}`}