From c614a7f4441bfd46c8851d658bc5598e2465fb74 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Mon, 25 May 2015 12:36:33 +0100 Subject: Fix UI Component race condition on start up. Approach: Re-use the existing AsynDataFetcher class to "fetch" and use the iframe message port. Messages are queued until the iframe's contents have loaded and the message port is open. Fixes #1679. --- content_scripts/ui_component.coffee | 58 ++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/content_scripts/ui_component.coffee b/content_scripts/ui_component.coffee index e7cd3f82..31274c3e 100644 --- a/content_scripts/ui_component.coffee +++ b/content_scripts/ui_component.coffee @@ -16,7 +16,6 @@ class UIComponent className: className seamless: "seamless" src: chrome.runtime.getURL iframeUrl - @iframeElement.addEventListener "load", => @openPort() shadowWrapper = document.createElement "div" # PhantomJS doesn't support createShadowRoot, so guard against its non-existance. @shadowDOM = shadowWrapper.createShadowRoot?() ? shadowWrapper @@ -28,6 +27,16 @@ class UIComponent # Hide the iframe, but don't interfere with the focus. @hide false + # Open a port and pass it to the iframe via window.postMessage. + @iframePort = new AsyncDataFetcher (setIframePort) => + @iframeElement.addEventListener "load", => + # Get vimiumSecret so the iframe can determine that our message isn't the page impersonating us. + chrome.storage.local.get "vimiumSecret", ({ vimiumSecret }) => + { port1, port2 } = new MessageChannel + port1.onmessage = (event) => @handleMessage event + @iframeElement.contentWindow.postMessage vimiumSecret, chrome.runtime.getURL(""), [ port2 ] + setIframePort port1 + # If any other frame in the current tab receives the focus, then we hide the UI component. # NOTE(smblott) This is correct for the vomnibar, but might be incorrect (and need to be revisited) for # other UI components. @@ -35,40 +44,31 @@ class UIComponent @postMessage "hide" if @showing and request.name == "frameFocused" and request.focusFrameId != frameId false # Free up the sendResponse handler. - # Open a port and pass it to the iframe via window.postMessage. - openPort: -> - messageChannel = new MessageChannel() - @iframePort = messageChannel.port1 - @iframePort.onmessage = (event) => @handleMessage event - - # Get vimiumSecret so the iframe can determine that our message isn't the page impersonating us. - chrome.storage.local.get "vimiumSecret", ({vimiumSecret: secret}) => - @iframeElement.contentWindow.postMessage secret, chrome.runtime.getURL(""), [messageChannel.port2] - - # Posts a message; returns true if the message was sent, false otherwise. - postMessage: (message) -> - # We use "?" here because the iframe port is initialized asynchronously, and may not yet be ready. - @iframePort?.postMessage message - @iframePort? + # Posts a message (if one is provided), then calls continuation (if provided). The continuation is only + # ever called *after* the message has been posted. + postMessage: (message = null, continuation = null) -> + @iframePort.use (port) => + port.postMessage message if message? + continuation?() activate: (@options) -> - if @postMessage @options + @postMessage @options, => @show() unless @showing @iframeElement.focus() show: (message) -> - @postMessage message if message? - @iframeElement.classList.remove "vimiumUIComponentHidden" - @iframeElement.classList.add "vimiumUIComponentVisible" - # The window may not have the focus. We focus it now, to prevent the "focus" listener below from firing - # immediately. - window.focus() - window.addEventListener "focus", @onFocus = (event) => - if event.target == window - window.removeEventListener "focus", @onFocus - @onFocus = null - @postMessage "hide" - @showing = true + @postMessage message, => + @iframeElement.classList.remove "vimiumUIComponentHidden" + @iframeElement.classList.add "vimiumUIComponentVisible" + # The window may not have the focus. We focus it now, to prevent the "focus" listener below from firing + # immediately. + window.focus() + window.addEventListener "focus", @onFocus = (event) => + if event.target == window + window.removeEventListener "focus", @onFocus + @onFocus = null + @postMessage "hide" + @showing = true hide: (focusWindow = true)-> @refocusSourceFrame @options?.sourceFrameId if focusWindow -- cgit v1.2.3 From 8b57d179a1655dbce081b321445f34a4aad696f4 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Tue, 26 May 2015 06:03:07 +0100 Subject: Add comment noting why we need AsyncDataFetcher. --- content_scripts/ui_component.coffee | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/content_scripts/ui_component.coffee b/content_scripts/ui_component.coffee index 31274c3e..24982fa7 100644 --- a/content_scripts/ui_component.coffee +++ b/content_scripts/ui_component.coffee @@ -27,7 +27,9 @@ class UIComponent # Hide the iframe, but don't interfere with the focus. @hide false - # Open a port and pass it to the iframe via window.postMessage. + # Open a port and pass it to the iframe via window.postMessage. We use an AsyncDataFetcher to handle + # requests which arrive before the frame (and its message handlers) have completed initialization. See + # #1679. @iframePort = new AsyncDataFetcher (setIframePort) => @iframeElement.addEventListener "load", => # Get vimiumSecret so the iframe can determine that our message isn't the page impersonating us. -- cgit v1.2.3 From 6b49dd27cb20ef5f830b1107b39480201796885d Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Tue, 26 May 2015 07:19:52 +0100 Subject: Refactor to avoid potential race condition. I haven't seen this happen, but... It could be possible for the iframe's contents to load before this callback is called (on nextTick), in which case the "load" callback wouldn't be called. So we delay setting the iframe's source until nextTick has elapsed. --- content_scripts/ui_component.coffee | 6 ++++-- tests/dom_tests/phantom_runner.coffee | 29 +++++++++++++++++------------ 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/content_scripts/ui_component.coffee b/content_scripts/ui_component.coffee index 24982fa7..4e26f26a 100644 --- a/content_scripts/ui_component.coffee +++ b/content_scripts/ui_component.coffee @@ -15,7 +15,6 @@ class UIComponent extend @iframeElement, className: className seamless: "seamless" - src: chrome.runtime.getURL iframeUrl shadowWrapper = document.createElement "div" # PhantomJS doesn't support createShadowRoot, so guard against its non-existance. @shadowDOM = shadowWrapper.createShadowRoot?() ? shadowWrapper @@ -28,9 +27,12 @@ class UIComponent @hide false # Open a port and pass it to the iframe via window.postMessage. We use an AsyncDataFetcher to handle - # requests which arrive before the frame (and its message handlers) have completed initialization. See + # requests which arrive before the iframe (and its message handlers) have completed initialization. See # #1679. @iframePort = new AsyncDataFetcher (setIframePort) => + # We set the iframe source here (and not above) to avoid a potential race condition vis-a-vis the "load" + # event (because this callback runs on "nextTick"). + @iframeElement.src = chrome.runtime.getURL iframeUrl @iframeElement.addEventListener "load", => # Get vimiumSecret so the iframe can determine that our message isn't the page impersonating us. chrome.storage.local.get "vimiumSecret", ({ vimiumSecret }) => diff --git a/tests/dom_tests/phantom_runner.coffee b/tests/dom_tests/phantom_runner.coffee index 93218724..e0382a35 100644 --- a/tests/dom_tests/phantom_runner.coffee +++ b/tests/dom_tests/phantom_runner.coffee @@ -37,15 +37,20 @@ page.open testfile, (status) -> console.log 'Unable to load tests.' phantom.exit 1 - testsFailed = page.evaluate -> - Tests.run() - return Tests.testsFailed - - if system.args[1] == '--coverage' - data = page.evaluate -> JSON.stringify _$jscoverage - fs.write dirname + 'dom_tests_coverage.json', data, 'w' - - if testsFailed > 0 - phantom.exit 1 - else - phantom.exit 0 + runTests = -> + testsFailed = page.evaluate -> + Tests.run() + return Tests.testsFailed + + if system.args[1] == '--coverage' + data = page.evaluate -> JSON.stringify _$jscoverage + fs.write dirname + 'dom_tests_coverage.json', data, 'w' + + if testsFailed > 0 + phantom.exit 1 + else + phantom.exit 0 + + # We add a short delay to allow asynchronous initialization (that is, initialization which happens on + # "nextTick") to complete. + setTimeout runTests, 10 -- cgit v1.2.3 From a9f8798f8379a6db5f57eb9bdca9be0f35fae162 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Tue, 26 May 2015 10:07:48 +0100 Subject: Refactor to avoid potential race condition (cont). --- content_scripts/ui_component.coffee | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/content_scripts/ui_component.coffee b/content_scripts/ui_component.coffee index 4e26f26a..ba141b23 100644 --- a/content_scripts/ui_component.coffee +++ b/content_scripts/ui_component.coffee @@ -20,7 +20,6 @@ class UIComponent @shadowDOM = shadowWrapper.createShadowRoot?() ? shadowWrapper @shadowDOM.appendChild styleSheet @shadowDOM.appendChild @iframeElement - document.documentElement.appendChild shadowWrapper @showing = true # The iframe is visible now. # Hide the iframe, but don't interfere with the focus. @@ -30,9 +29,11 @@ class UIComponent # requests which arrive before the iframe (and its message handlers) have completed initialization. See # #1679. @iframePort = new AsyncDataFetcher (setIframePort) => - # We set the iframe source here (and not above) to avoid a potential race condition vis-a-vis the "load" - # event (because this callback runs on "nextTick"). + # We set the iframe source and append the new element here (as opposed to above) to avoid a potential + # race condition vis-a-vis the "load" event (because this callback runs on "nextTick"). @iframeElement.src = chrome.runtime.getURL iframeUrl + document.documentElement.appendChild shadowWrapper + @iframeElement.addEventListener "load", => # Get vimiumSecret so the iframe can determine that our message isn't the page impersonating us. chrome.storage.local.get "vimiumSecret", ({ vimiumSecret }) => -- cgit v1.2.3