Issue metadata
Sign in to add a comment
|
Page scripts are run before script appended to page through contentScript.
Reported by
m...@mostlystatic.com,
Aug 4 2016
|
||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.82 Safari/537.36 Steps to reproduce the problem: The problem occurs when appending a script tag from a content script. The file loaded by the script is the injected file. 1. Check out https://github.com/mattzeunert/content-script-injected-code-executed-late 2. Load extension directory into Chrome 3. Open test.html What is the expected behavior? It logs: 1. contentScript 2. injected 3. in test.js What went wrong? It logs: 1. contentScript 2. in test.js 3. injected WebStore page: Did this work before? N/A Chrome version: 52.0.2743.82 Channel: n/a OS Version: OS X 10.11.6 Flash Version: Shockwave Flash 22.0 r0 This seems to be related to how long it takes to load either the injected file or the JavaScript file loaded by the web page. For example, if I add more code to the test.js file my injected code will be run first. If I then add even more code to injected.js then the page JavaScript (test.js) is again executed first I'm not sure if the problem only occurs when viewing pages on localhost or the file system, or if a similar problem can occur with cached content.
,
Aug 4 2016
I think part of the issue is that the parse tree has been created already at this point, and your injected script is just being added to the end of the HTML. You can see this in the inspector. However, it doesn't explain why you are getting different order of execution based on the size, since it should execute in they appear on the DOM (if not explicitly deferred). I'll have another look soon to compare the behaviour with the varying of the sizes. I think AdBlock are able to achieve the same desired behaviour you need, so there should be a way. Need to play around more.
,
Aug 5 2016
It seems like whichever script loads first is run first. I tried setting `scriptEl.text` to the script content instead of setting `scriptEl.src`, and it executes in the correct order. For me, that's an acceptable workaround.
,
Aug 5 2016
I got different results this morning, possibly I made a change without realizing. If you create a plain HTML page and put a script tag before <head>, and one within <body>, it executes in order - the first script tag is moved and actually rendered in the <head> tag. However, when you mess with `documentElement` in the content script, it doesn't move it, and executes out of order. I suspected that `document_start` was too late to get Chrome to modify parsing order of tree, but since you get the right order by setting the text property, I'm at a loss.
,
Oct 18 2016
This is a bad bug which is breaking extensions and was broken recently. It's definitely a race condition as refreshing a page can make it work when before it didn't and vice versa, so I think the text fix mentioned isn't and is just a fluke.
,
Oct 18 2016
I've found a way to reliably reproduce this with one of my extensions when navigation is involved. The culprit is between 398927 (GOOD) and 398939 (NOT). It can only be 398937. https://chromium.googlesource.com/chromium/src.git/+/d63bd15666f24c048b0caa641cace3eec9460769
,
Oct 18 2016
Hm I think this is not breaking the spec, just exposing existing raciness inherent in the spec. Inserting scripts via document.createElement follows different rules than declaring scripts in markup: Obtuse spec link: https://html.spec.whatwg.org/multipage/scripting.html#non-blocking This article also has a nice explanation: https://www.html5rocks.com/en/tutorials/speed/script-loading/#toc-dom-rescue Note: this analysis is based on your comment in #1.
,
Oct 19 2016
I'm not sure there's not a bug here somewhere. I used aysnc = false on the injected <script> element as recommended on the page, which made no difference, when it should be blocking according to spec. I think the bug is in the fact that at document_start you can only append to document.documentElement. > In the case of "document_start", the files are injected after any files from css, but before any other DOM is constructed or any other script is run. https://developer.chrome.com/extensions/content_scripts And when a DOM is then constructed, the very first <script>, injected by the content script, can get somehow pushed down the queue. I think this is what comment #4 also mentions. I'm not familiar with the internals, so I don't know how or why. What I can definitely say that the script was executed in order, as expected, every time before 398937. As it is now, document_start is somewhat useless, because you cannot reliably beat <script> elements on the page.
,
Oct 19 2016
Adding HTML>Script for further help on createElement and async = false. I don't have a great reference, but I *think* async = false might only guarantee ordering within inserted scripts. Can you post the most reliable repro you have? The patch should not have modified insertion order, just when we send out preloads for scripts we've discovered in the markup. I can double check this though.
,
Oct 20 2016
I was experiencing this very same bug, and was able to come up with the following workaround:
var url = '//third-party-js.com/third-party.js',
urlEncoded = window.btoa(unescape(encodeURIComponent(url))),
urlContent = localStorage.getItem(urlEncoded);
if (!urlContent) {
var xhr = new XMLHttpRequest();
xhr.open('GET', url, true);
xhr.onload = function(e) {
if ((xhr.readyState === 4) && (xhr.status === 200)) {
localStorage.setItem(urlEncoded, xhr.responseText);
window.location.reload(true);
}
};
xhr.send(null);
document.write('<script src="' + url + '"><\/scr' + 'ipt>');
} else {
var script = document.createElement('script');
script.textContent = urlContent;
document.documentElement.append(script);
}
,
Oct 21 2016
I notice now that I misunderstood the text fix mentioned in comment #3, which does indeed work. So the bug is in loading a src address, even it if loads a local file via chrome.extension.getURL.
,
Dec 12 2016
Hi csharrison, could you explain in more detail how that change exposes non-determinism which was always there? An the surface this *looks* like a regression. Content script "document start" seems to be advertised as being before anything else happens, and the change mentioned in Comment 6 does seem to affect a hook used by extensions. re: async, you might be thinking of "non-blocking" [1]. In this case I suppose it is set (the initial state.) That flag is flipped back and forth a lot in HTML to make script elements behave a certain way as they're moved in and out of the DOM in various pretty narrow edge cases. It affects prepare a script [2] around step 22, but read thoroughly because a clause which *doesn't* mention non-blocking may be the operative one here. [1] https://html.spec.whatwg.org/#script-processing-model [2] https://html.spec.whatwg.org/#prepare-a-script I'm taking the script component off this bug. This looks like a regression in extensions. If you can reduce it to a web page which demonstrates a bug, I'd be happy to take a look, but it doesn't make a ton of sense to me to be talking about HTML semantics for extensions content_script. For example, how does the extension system create the script element and get it into the page? Given user reports that it shows up at various places in the tree, it sounds like extensions are doing something asynchronously and you may be hosed before you even reach the DOM.
,
Dec 12 2016
I am not sure how the extension system hooks in, I was mainly rationalizing as if the content scripts is the first element in <head>. +devlin cc if there's anything more to be aware of (wrt isolate extensions, etc.) I think my thought was raciness between (a) and (b) (a) The "list of scripts that will execute in order as soon as possible". Generated by the content script injecting a script element. (b) The "pending parsing-blocking script". First script tag in <head> Since my change was flagged by the bisect, and it only affects preloads, I assumed there might be a race where (b) could sometimes load and execute before (a) if (b)'s preload starts before (a) is executed. I can certainly take another look at this today though.
,
Dec 12 2016
dominicc, my understanding of this issue can be reproduced without extensions with the following HTML:
<script>
var s = document.createElement('script');
s.src = "/slow.php"; // Loads in 5s. Logs to console.
document.head.prepend(s);
</script>
<script src="/fast-script.js"></script>
The fast second script is loaded and executed first, before the first slow script. If the scripts are reversed and the slow script is second after the inline script, the slow script still executes last.
If it helps, I can upload this repro to a public facing server. This is where I imagined the raciness comes in, where my patch gave the document's scripts a "head start" over extension inserted scripts, making them win this race more often. Please let me know if I'm off base here.
,
Dec 12 2016
I've confirmed that before my patch (#398936) we still have this race, and it is possible for a fast script to beat the script injected by the content script in the OP.
,
Apr 27 2017
Hi There! I'm another extension developer, just trying to inject a script tag before pages load! I'm coming from issue 715712 , which I believe is probably just a duplicate of this one. I've created a test case web page for this issue here: https://flyswatter.github.io/chrome-contentscript-bug-reproduction/ Requires my extension, MetaMask to be installed: https://chrome.google.com/webstore/detail/metamask/nkbihfbeogaeaoehlefnkodbefgpgknn?authuser=2 I understand a portion of this discussion is over "whether this behavior is to spec or not", and I'd love to move the discussion towards "what should happen?", because I think from the use cases of the extension developers here, it's clear that extensions should be able to inject a script tag before other scripts load. Without that ability, there's a hard limit on how much of an "Extension" they really can be. I'll be going through all the hacks and workarounds proposed here tomorrow, so thanks for all the tips, everyone! I'd also like to take this opportunity (since it seems like the correct group, and a small one), to point out that Mozilla has been considering an API that could potentially solve these problems, allowing an extension to be "externally connectable from a website", and it is briefly discussed here: https://bugzilla.mozilla.org/show_bug.cgi?id=1319168 This would allow extensions to simply make an object and its API available to websites, without fussing with using a content script to hack a script tag into every page, muddying the DOM and all that. I'd love to see this become a common standard way of allowing Extension devs to experiment with new web APIs. That's all, thanks for reading!
,
Apr 27 2017
csharrison, I see what you're saying. I was confused; I thought it was the document_start *content script* that was running after the script in the page. You're saying that the content script runs at the right time, but the script tag may run after scripts in the page. I guess from the HTML point of view, document.write at that point may run the script synchronously. But I just tried that and it hangs the loading of pages, probably because the parser pretty carefully raises and lowers shields when document.write can happen. I'm going to flip this back to untriaged. I feel we need help from the people who work on extensions. I'm happy to help with this but I don't have enough context about what guarantees extensions intend to provide.
,
Apr 27 2017
Thanks dan.finlay for bringing this up. This is definitely something Chrome team should try to make easy for authors. dominicc, agreed we should still do something here, sorry for the miscommunication around the problem! I think I must apologize, because I remember chatting with rdevlin offline about this back in October in an unrecorded medium and I forget some of the guarantees that were mentioned. My hazy memory (please, correct me if I'm wrong), is that the content script wants to execute fully and completely in isolation, before any page-level parsing happens. Obviously this hits problems when we start talking about asynchronous operations like loading happening within the content script. Getting document.write to work in a content script could be a workable solution. I'm not sure how complex a change that would entail though.
,
Apr 28 2017
The guarantees we make about content scripts are here: https://developer.chrome.com/extensions/content_scripts Most relevantly, "In the case of "document_start", the files are injected after any files from css, but before any other DOM is constructed or any other script is run." From this, I'd personally expect that a <script> tag added to the dom in the content script would execute before the rest of the DOM is constructed, because that was my understanding of the typical behavior on the web (though I'm definitely not a spec expert). csharrison, does this make sense from a blink/scripting standpoint?
,
Apr 28 2017
At least my understanding is that inserting a <script> tag is effectively asynchronous if done through document.createElement, and synchronous if done via document.write. So even if the content script is wholly executed before any other script we could still hit races due to the asynchronous operation. I think a reasonable solution would be to fix synchronous insertion in content scripts via document.write.
,
May 1 2017
Yeah it is weird to say it but document.write would probably do what extension authors want here. And hanging is bad, it should probably either work or fail and not hang. Re: Comment 19/"before any DOM is constructed" I think the code waits for the document element to exist and you'd probably want to continue to be the case. I guess document.write and script element injection isn't going to work well with Content Security Policy. I wonder if extensions just needs an API here that is "run script in the context of the main page"?
,
May 1 2017
We'll experiment with document.write, but for the sake of context, I'd like to explain our use case once more, as simply as I can: We're trying to provide a non-native browser API. This is an object that we will make available globally to all other scripts that run in the page. Our API does not mutate the DOM (we only mutate DOM to insert this API). We do not mutate the page context, we only receive method calls, do work in a background context, and call back. This allows us to allow our community of developers to experiment with some pretty forward-looking and rough-edged browser APIs, with a view towards making a more peer-to-peer browser. Anyways, we'll try document.write, and I'll report back if that works. We *have* tried insertElement as an alternative, and that worked even worse: https://github.com/MetaMask/metamask-plugin/pull/482/files
,
May 1 2017
Dan, thanks for posting your use case. This is 100% something we should support without causing undue burden to you. Unfortunately it looks like in comment 17 dominicc didn't have much success with using document.write in a content script.
,
Jul 18 2017
Any progress on this bug? It still bites my users regularly.
,
Jul 18 2017
Sorry to say but I don't think there's been any progress on this issue. Let me see if I can get it on the loading team's radar by adding some appropriate labels.
,
Jul 18 2017
Thanks for trying to nudge it along!
,
Jul 25 2017
hiroshige, I think you know as much about script loading/starting as anyone at this point. Is that something you'd be interested in taking on? Feel free to reassign/unassign as appropriate.
,
Oct 4 2017
@Dan >We *have* tried insertElement as an alternative, and that worked even worse: https://github.com/MetaMask/metamask-plugin/pull/482/files It looks like the problem is that you're still setting the .src property on the script element, so the script element's text content is completely ignored and the script is still being loaded asynchronously from the src url. If you remove the `scriptTag.src = extension.extension.getURL('scripts/inpage.js')` line, then the injected script will run synchronously as it's inserted into the DOM. You can then fix the issue that Chrome no longer knows what file the code belongs to by changing the definition of inpageText: const inpageText = fs.readFileSync(path.join(__dirname + '/inpage.js')).toString() + '//# sourceURL=' + extension.extension.getURL('scripts/inpage.js') + '\n' (Then once you do this, you'll have the code synchronously executing and all as you want, but you'll get an error about global or require not being defined. The issue is that the inpageText variable contains the string contents of the un-browserified source code instead of the browserify-compiled code. The MetaMask build process needs to build inpage.js first before building contentscript.js, and contentscript.js's inpageText=fs.readFileSync line needs to point to the result of that.)
,
Oct 4 2017
Wow, agentm, I think you might have figured this out! I'll have to make the build process changes you've described, but this is a very exciting development, thank you, will return with the results soon!
,
Oct 14 2017
Sorry for the long wait, but I believe that agentm found the problem here. This was perhaps not a Chrome bug at all, but a problem with how we were adding a script tag to the loaded page: With a src attribute, instead of only with innerText. It seems that we've fixed it on master, and it'll be shipped to production soon!
,
Oct 14 2017
It's definitely a Chrome problem, in that it's a regression.
,
Jan 22 2018
,
Feb 13 2018
Summary so far (am I summarizing correctly?): Currently, contentScript is executed before any <script> elements are inserted in the DOM tree, but the script elements injected by the contentScript are/can be executed after <script> elements in the main web page. A reliable way to execute an external script injected by a contentScript before any other (parser-blocking) scripts in web pages, is wanted. However, according to HTML spec, there are no ways for this: - If the element has a src attribute, then it is put into either "list of scripts that will execute in order as soon as possible" or "set of scripts that will execute as soon as possible" (as the element is not marked as parser-inserted), which doesn't guarantee any execution order relative to parser-blocking scripts. - We can insert inline script element (setting el.innerText instead of el.src) that is executed synchronously, but not an external script. - Inserting scripts by document.write() was also considered, but didn't seem to work. So probably we'll need extension-specific way to guarantee the execution order.
,
Feb 13 2018
A candidate implementation I came up with is to block the HTML parser just after a content script is executed if there are pending scripts, but this will complicate parser-blocking mechanism of HTML parser (which is already complicated). Content scripts are executed, for example, in extensions::ExtensionFrameHelper::RunScriptsAtDocumentStart() in blink::HTMLConstructionSite::InsertHTMLHtmlStartTagBeforeHTML() in blink::HTMLTreeBuilder::ProcessStartTag(), i.e. when <html> element is inserted into the DOM, and thus the HTML parser is already started. So we need to pause the already started HTML parser. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 Deleted