fix: content script is incorrectly invalidated when injected multiple times #1938
Conversation
`window.postMessage` is performed asynchronously, which results in race conditions where a content script can be incorrectly invalidated `document.dispatchEvent` invokes event handlers synchronously which means only old scripts will be invalidated
No longer necessary to handle wxt-dev#884 since `document.dispatchEvent` is handled synchronously
✅ Deploy Preview for creative-fairy-df92c4 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| } | ||
|
|
||
| stopOldScripts() { | ||
| // Use postMessage so it get's sent to all the frames of the page. |
There was a problem hiding this comment.
In case there's concern that changing from window.postMessage to document.dispatchEvent will change behavior when it comes to other frames: This comment is incorrect because window.postMessage only sends a message to the targeted window. It doesn't automatically send messages to other frames — you'd need to specifically target those windows in order to send a message.
There was a problem hiding this comment.
Doesn't the * target mean we're sending messages to all frames?
There was a problem hiding this comment.
This is my main concern. Basically, this class is meant to properly stop and shut down a content script when an extension updates. In production or in development, you will encounter bugs if we don't properly stop ALL the content scripts running on a page. So if in one update, a content script changes from allFrames: true to allFrames: false, some scripts running in iframes won't be stopped by using custom events.
Because I'm 99% sure targeting "*" sends a message to all iframes on the page, and any level.
Now, that said, the current approach has tradeoffs as well, this race condition you found. If we don't send a message to all frames, then technically those scripts will still stop after they try and use an extension API, only to find that they can't because the context was invalidated after the update.
On a separate note, another thing to consider is that this is a breaking change that will effect all WXT extensions on their next update. We're going to send a custom event, but the old version of the extension will be listening for messages. This could be resolved by continuing to send the message...
Can I get some more context around why you would inject the same script multiple times in quick succession, like from the reproduction? I need more info to decide how likely this race condition is to weigh it against merging this PR and changing the behavior.
There was a problem hiding this comment.
window.postMessage and iframes
Targeting "*" doesn't send a message to all iframes on the page. The targetOrigin parameter of window.postMessage specifies which origin is allowed to receive the message.
In order to send a message to an iframe, the iframe's window needs to be targeted directly: e.g. iframe.contentWindow.postMessage(message, targetOrigin);
From MDN:
Furthermore, an accessing script must have obtained the window object of the accessed document beforehand. This can occur through methods such as window.open() for popups or iframe.contentWindow for iframes.
The current code of window.postMessage(message, "*") simply sends a message to the current window and allows any origin to receive it, but it doesn't send a message to iframes.
That means the current approach actually has a bug where content scripts inside iframes are not invalidated at all when the content script is injected again (e.g. when the extension updates).
I've created a video demonstration of the behavior with allFrames: true with the current implementation and the fix proposed in this PR: https://www.loom.com/share/a172168ee3744979b949ef8411d126bc
Backwards compatibility
I've added to the PR to temporarily continue sending the message via window.postMessage as well in order to invalidate content scripts from before this change. This makes this change backwards compatible and not a breaking change.
For the particular scenario you gave: The new implementation doesn't invalidate content scripts inside iframes when injecting a content script is changed from allFrames: true to allFrames: false. But this wouldn't be breaking change since the current implementation doesn't handle that case either. I personally think this is an edge case that's not worth worrying about, in the same way that WXT doesn't try to invalidate content scripts that were injected in previous versions of an extension but were removed or renamed in the current version. I think it'd be fine to let them stop when trying to use an extension API.
Overall, I think this change is worth making because it (1) fixes the bug with content scripts inside iframes not being invalidated and (2) entirely eliminates race conditions with invalidations given the synchronous nature of document.dispatchEvent.
Context for multiple injections
Here's why the content script is injected multiple times in quick succession in my extension (Easy Scraper):
When the user clicks the extension icon to open the popup window, a content script is injected into the current tab and is injected again whenever a new page is loaded in the current tab. The popup window does this by having a listener for browser.tabs.onUpdated and checking for changeInfo.status === "complete" to detect when a new page is loaded.
However, there are cases when a page causes browser.tabs.onUpdated to fire multiple times with changeInfo.status === "complete". For example, it's fired with status complete again when the page sets the hash (e.g. window.location.hash = "section") or changes the URL without reloading the page (e.g. window.history.pushState({}, '', '/new-url');.
So there are cases where the initial page load causes browser.tabs.onUpdated to fire in quick succession, which causes the popup window to inject the content script multiple times. (There's a check that runs to see if the current tab already has the content script before it tries to inject it, but in this case, that check fails because no content script has been injected yet when onUpdated is fired multiple times.)
For extensions that dynamically inject the content script using browser.tabs.onUpdated (as in my case where the activeTab permission is used along with a popup), this is a fairly unavoidable scenario.
namukang
changed the title
2 participants
Overview
Video of issue and fix: https://www.loom.com/share/97983e5d38904af594d5f486bbf9f1f4?sid=e588c652-c868-4cb0-8db2-521650726919
This PR replaces
window.postMessagewithdocument.dispatchEventin order to avoid race conditions when invalidating content scripts.Since
document.dispatchEventinvokes event handlers synchronously, it prevents issues with invalidation due to the asynchronous nature ofwindow.postMessage.I cleaned up related code in
ContentScriptContextthat followed as a result of this change:document.dispatchEventoperates within its frame,stopOldScriptsis called in all cases, not just in the top frame so that duplicate content scripts are invalidated when they're injected into a frame.Related Issue
This PR closes #1937