Closed
Bug 1272890
Opened 9 years ago
Closed 8 years ago
Support match_about_blank in content scripts
Categories
(WebExtensions :: Untriaged, defect, P3)
Tracking
(firefox52 fixed)
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: man-pack, Assigned: zombie)
References
Details
(Whiteboard: triaged)
Attachments
(2 files)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160513004028
Steps to reproduce:
I migrate an addon to WebExtension and I use the executeScript to a new page (blank).
Code in the background script:
browser.tabs.create({ url: "about:blank" }, function (tab) {
browser.tabs.executeScript(tab.id, { matchAboutBlank: true, code: "console.log(44);" }, function () {
console.log(chrome.runtime.lastError);
});
});
Permissions in the manifest are activeTab and <all_urls>.
Actual results:
The script isn't executed and I have the "No matching window error".
The workaround indicated in the bug 1254003 (add a timer to wait that the page is loaded) didn't work.
Expected results:
The script is executed and '44' is logged in the console.
Reporter | ||
Updated•9 years ago
|
Component: Untriaged → WebExtensions
OS: Unspecified → Windows 10
Product: Firefox → Toolkit
Hardware: Unspecified → x86_64
Updated•9 years ago
|
Priority: -- → P3
Whiteboard: triaged
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 10 → All
Hardware: x86_64 → All
Summary: browser.tabs.executeScript with param matchAboutBlank outputs "No matching window" → Support match_about_blank in content scripts
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
I scoped this down to only content scripts (not executeScript), and iframes, because the fix is somewhat invasive and hacky.
When this is done, I'll fix the rest in a followup bug.
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•8 years ago
|
||
Also, the test is somewhat convoluted (inspired by test_ext_exclude_include_globs.html), because I couldn't think of a simpler way to to do it. If there is an obvious better way, I'll be glad to do it.
Assignee | ||
Updated•8 years ago
|
Attachment #8800081 -
Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Comment 8•8 years ago
|
||
Note that according to Chrome documentation this option affects about:blank and about:srcdoc (the current patch only considers the first). However, the Chrome implementation is inconsistent as it disallows content scripts for various other frames types that inherit principal - e.g. blob: or data: URIs. These should really be treated in the same way.
Blocks: webext-port-abp
Assignee | ||
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts
https://reviewboard.mozilla.org/r/85098/#review84288
::: toolkit/components/extensions/ExtensionContent.jsm:141
(Diff revision 2)
> }
>
> + if (this.match_about_blank && uri.spec === "about:blank") {
> + // When matching about:blank documents, the checks below need
> + // to be performed against the "owner" document's URI.
> + uri = window.document.nodePrincipal.URI;
Looking at this after bug 1272890 comment 8, maybe it makes sense to *always* check against the principal URI? I don't see an obviuos case where that would not work as intended..
Assignee | ||
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts
https://reviewboard.mozilla.org/r/85098/#review84290
::: toolkit/components/extensions/ExtensionContent.jsm:141
(Diff revision 2)
> }
>
> + if (this.match_about_blank && uri.spec === "about:blank") {
> + // When matching about:blank documents, the checks below need
> + // to be performed against the "owner" document's URI.
> + uri = window.document.nodePrincipal.URI;
(and by "always", I mean "when match_about_blank is on")
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts
https://reviewboard.mozilla.org/r/85098/#review84642
Looks great! Thanks!
I'm just slightly worried how much we do in the `global-created & href != about:blank` case, and it needs a couple more tests.
::: toolkit/components/extensions/ExtensionContent.jsm:482
(Diff revision 2)
> + // Normally, we trigger document_start on document-element-inserted,
> + // except for about:blank which only sees content-document-global-created.
> + if (topic == "document-element-inserted" || window.location.href == "about:blank") {
> - this.trigger("document_start", window);
> + this.trigger("document_start", window);
> + }
It seems like we should return early, immediately inside the outer `if`, if the event is `content-document-global-created` and the URL is not `about:blank`... I guess everything else should be safe to run twice, but it does seem like a bit of a footgun waiting to happen.
Also, I think there may be times when we get `content-document-global-created` when the URL is `about:blank`, and then the window gets reused with a different URL. I'm not 100% sure either way, though, so it might be worth some extra testing.
::: toolkit/components/extensions/test/mochitest/mochitest.ini:54
(Diff revision 2)
> [test_ext_contentscript_context.html]
> [test_ext_contentscript_create_iframe.html]
> [test_ext_contentscript_devtools_metadata.html]
> [test_ext_contentscript_exporthelpers.html]
> [test_ext_contentscript_css.html]
> +[test_ext_contentscript_about_blank.html]
We also need to test this with `tabs.executeScript` and `tabs.insertCSS` in a browser mochitest.
::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_about_blank.html:22
(Diff revision 2)
> + content_scripts: [
> + {
> + match_about_blank: true,
> + matches: ["http://mochi.test/*/file_with_about_blank.html", "http://example.com/*"],
> + all_frames: true,
> + js: ["all.js"],
We should also test this with CSS.
::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_about_blank.html:50
(Diff revision 2)
> + },
> + };
> +
> + function background() {
> + browser.runtime.onMessage.addListener(([script, top]) => {
> + const full = script + (top ? ":top" : ":iframe");
How about `\`${script}:${top ? "top": "iframe"}\``?
::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_about_blank.html:62
(Diff revision 2)
> + const extension = ExtensionTestUtils.loadExtension({manifest, files, background});
> + yield extension.startup();
> +
> + let count = 0;
> + extension.onMessage("script", script => {
> + info("script ran: " + script);
Template string please.
::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_about_blank.html:67
(Diff revision 2)
> + info("script ran: " + script);
> + count++;
> + });
> +
> + let win = window.open("http://example.com/" + PATH);
> + yield Promise.all([extension.awaitMessage("all:top"), extension.awaitMessage("all:iframe")]);
Please split into multiple lines, as below.
::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_about_blank.html:73
(Diff revision 2)
> + extension.awaitMessage("all:top"), extension.awaitMessage("all:iframe"),
> + extension.awaitMessage("mochi_without:top"),
> + extension.awaitMessage("mochi_with:top"), extension.awaitMessage("mochi_with:iframe"),
One `awaitMessage` per line, please.
Attachment #8800081 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 12•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts
https://reviewboard.mozilla.org/r/85098/#review84642
> We also need to test this with `tabs.executeScript` and `tabs.insertCSS` in a browser mochitest.
Filed followup bug 1310331.
Comment hidden (mozreview-request) |
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts
https://reviewboard.mozilla.org/r/85098/#review86382
::: toolkit/components/extensions/ExtensionContent.jsm:464
(Diff revision 3)
> return;
> }
>
> + // Make sure we always load exactly once (notice the logical XOR ^),
> + // usually on document-element-inserted, except for about:blank documents.
> + if ((topic == "content-document-global-created") ^ (window.location.href == "about:blank")) {
s/\^/!=/
Attachment #8800081 -
Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 16•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc266f8bc70a
implement match_about_blank for content scripts r=kmag
Keywords: checkin-needed
Comment 17•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 18•8 years ago
|
||
Back this out for frequent failures in test_ext_contentscript_about_blank.html and a cascade of non-webextension tests:
https://hg.mozilla.org/integration/autoland/rev/60dd82380d43a2b681f50842238f829204486290
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=38087766&repo=mozilla-inbound
This also has the following and more errors:
488 INFO TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_basic_form_pwonly.html | Test to default value of field pname1 in form 4 - got "1234", expected ""
543 INFO TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_input_events_for_identical_values.html | Test timed out.
574 INFO TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_mousecapture_area.html | Test timed out.
Status: RESOLVED → REOPENED
Flags: needinfo?(tomica)
Resolution: FIXED → ---
Assignee | ||
Comment 19•8 years ago
|
||
Hey Sebastian, we are tracking this in bug 1312161. It seems to be happening only on Android, so if we can't figure it out quickly, I think we can disable the test, without the need for a full backout.
Flags: needinfo?(tomica)
Updated•8 years ago
|
status-firefox52:
fixed → ---
Target Milestone: mozilla52 → ---
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8800081 -
Flags: review+ → review?(kmaglione+bmo)
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts
https://reviewboard.mozilla.org/r/85098/#review87502
Attachment #8800081 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 22•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a904e8a06a7
implement match_about_blank for content scripts r=kmag
Keywords: checkin-needed
Assignee | ||
Comment 23•8 years ago
|
||
This seems to be causing test_ext_tab_teardown.html to fail, so another backout incoming..
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=372b1297c3cc6a8ebda5ebe03af97ef8fdce755b&selectedJob=5682958
Comment 24•8 years ago
|
||
Backed out for timing out in test_ext_tab_teardown.html:
https://hg.mozilla.org/integration/autoland/rev/77d16c6dc1f8013b9fc5a97312eaef0d14cf62d7
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6a904e8a06a7517c16cef2a330f33c2cf0ffc864
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=5684472&repo=autoland
[task 2016-10-26T16:28:23.569922Z] 16:28:23 INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_tab_teardown.html | ExtensionContext URL after reload should be tab URL
[task 2016-10-26T16:28:23.571927Z] 16:28:23 INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_tab_teardown.html | ExtensionContext after closing tab
[task 2016-10-26T16:28:23.574182Z] 16:28:23 INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_tab_teardown.html | unload tab's ExtensionContext
[task 2016-10-26T16:28:23.575987Z] 16:28:23 INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_tab_teardown.html | ExtensionContext URL at closing tab should be tab URL
[task 2016-10-26T16:28:23.577738Z] 16:28:23 INFO - SpawnTask.js | Leaving test test_extension_page_tabs_create_reload_and_close
[task 2016-10-26T16:28:23.579358Z] 16:28:23 INFO - SpawnTask.js | Entering test test_extension_page_window_open_reload_and_close
[task 2016-10-26T16:28:23.580858Z] 16:28:23 INFO - Extension loaded
[task 2016-10-26T16:28:23.582650Z] 16:28:23 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_tab_teardown.html | Test timed out.
[task 2016-10-26T16:28:23.584039Z] 16:28:23 INFO - reportError@SimpleTest/TestRunner.js:114:7
[task 2016-10-26T16:28:23.585460Z] 16:28:23 INFO - TestRunner._checkForHangs@SimpleTest/TestRunner.js:135:7
Flags: needinfo?(tomica)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(tomica)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8800081 -
Flags: review+ → review?(kmaglione+bmo)
Comment 27•8 years ago
|
||
mozreview-review |
Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts
https://reviewboard.mozilla.org/r/85098/#review88608
::: toolkit/components/extensions/ExtensionContent.jsm:510
(Diff revision 7)
> + // Load on document-element-inserted, except for about:blank which doesn't
> + // see it, and needs special late handling on DOMContentLoaded event.
> + if (topic === "document-element-inserted") {
> + this.loadInto(window);
> + this.trigger(window);
> + this.loadedIntoWindows.add(getInnerWindowID(window));
I'm worried about the possibilty of leaks here. Is this really necessary? The existing WeakMaps should really be enough...
Assignee | ||
Comment 28•8 years ago
|
||
mozreview-review |
Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts
https://reviewboard.mozilla.org/r/85098/#review88642
::: toolkit/components/extensions/ExtensionContent.jsm:560
(Diff revision 7)
>
> if (event.type == "DOMContentLoaded") {
> - this.trigger("document_end", window);
> + const windowId = getInnerWindowID(window);
> + // If we haven't loaded into the window yet, it means this is an explicit
> + // about:blank document, and needs special late loading and trigger.
> + if (!this.loadedIntoWindows.has(windowId)) {
I don't understand how an existing Map could reliably be used for this test here?
Comment hidden (mozreview-request) |
Comment 30•8 years ago
|
||
mozreview-review |
Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts
https://reviewboard.mozilla.org/r/85098/#review89050
Attachment #8800081 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 31•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/33f28f5ab038
implement match_about_blank for content scripts r=kmag
Keywords: checkin-needed
Comment 32•8 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 33•8 years ago
|
||
I can see that this fix is slated to be released in version 52, is there a possibility of getting it in any of the earlier versions? I don't know if this is the right place to ask this question, but I couldn't find any other.
Assignee | ||
Comment 34•8 years ago
|
||
It would probably help if you could give more details on why you are asking for uplifts. What's the name of your addon, what does it do, how many users does it have, and how it is impacting those users if this is (not) uplifted into earlier Firefox versions.
The core issue of this bug is that about:blank is the weirdest page for a browser [1]. The fix is a fairly low-level change, and as evidenced by the numerous tries (and intermittents, and backouts), could subtly break other parts of the codebase. Also, the code _around_ this fix has changed significantly recently, which means grafting this onto an earlier version would be even trickier.
Because of those reasons, I would not vote for the uplift, but then again, I'm not the person who makes these decisions.
1) https://hsivonen.fi/about-blank/
Comment 35•8 years ago
|
||
I verified this issue on Firefox 51.0a2 (20161013004015) and Firefox 52.0a1 (20161108030212).
I can see the same results in the browser console, there is no ‘44’ displayed after the script is executed, any thoughts on this?
Here is a video: http://screencast.com/t/8O1On5qcPfgk
Flags: needinfo?(tomica)
Assignee | ||
Comment 36•8 years ago
|
||
> I can see the same results in the browser console, there is no ‘44’
> displayed after the script is executed, any thoughts on this?
The title and scope of this bug changed somewhat. This only implements it for non-top level about:blank documents, in other words: iframes.
The use case from the original comment is being solved in bug 1310331.
Flags: needinfo?(tomica)
Comment 37•8 years ago
|
||
(In reply to Tomislav Jovanovic :zombie from comment #36)
> > I can see the same results in the browser console, there is no ‘44’
> > displayed after the script is executed, any thoughts on this?
>
> The title and scope of this bug changed somewhat. This only implements it
> for non-top level about:blank documents, in other words: iframes.
>
> The use case from the original comment is being solved in bug 1310331.
Thanks! I will track the other bug.
Could you provide some steps for manual testing for this one so I can verify it?
Comment 38•8 years ago
|
||
(In reply to Tomislav Jovanovic :zombie from comment #34)
> It would probably help if you could give more details on why you are asking
> for uplifts. What's the name of your addon, what does it do, how many users
> does it have, and how it is impacting those users if this is (not) uplifted
> into earlier Firefox versions.
>
> The core issue of this bug is that about:blank is the weirdest page for a
> browser [1]. The fix is a fairly low-level change, and as evidenced by the
> numerous tries (and intermittents, and backouts), could subtly break other
> parts of the codebase. Also, the code _around_ this fix has changed
> significantly recently, which means grafting this onto an earlier version
> would be even trickier.
>
> Because of those reasons, I would not vote for the uplift, but then again,
> I'm not the person who makes these decisions.
>
> 1) https://hsivonen.fi/about-blank/
Thanks for your reply, we have a chrome extension which detects all ad iframes on a page and gives users an option to like, dislike or save an ad banner. We are planning to support firefox as well. The problem is that most ad iframes have their origin defined as "aabout:blank", this effectively means that we cannot run content scripts against these ads, which renders out add-on useless.
I respect your concerns against the uplift, but is there a place where I can put in a formal request for that? If it helps we are willing to sponsor the uplift.
Assignee | ||
Comment 39•8 years ago
|
||
(In reply to CosminB from comment #37)
> Could you provide some steps for manual testing for this one so I can verify it?
It's a bit tricky to make a stand-alone extensions to test this manually, so I'll do it after we settle this issue with uplifts.
(In reply to AK from comment #38)
> is there a place where I can put in a formal request for that?
You can consider this your formal request for uplift.
I looked at the current code in Aurora, and changes required are not as bad as I though previously. My first patch was basically against that version of the code, before I had to rebase it on top of Rob's OOP changes.
Andy (or Kris), can you give the official response on weather we want ask for uplift here?
Flags: needinfo?(amckay)
Comment 40•8 years ago
|
||
Sorry, I can't see a good reason to ask for an uplift here, we'd normally only do that for critical bugs or regressions.
Flags: needinfo?(amckay)
Comment 41•8 years ago
|
||
I've documented this here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/content_scripts#match_about_blank
Does it look correct to you?
Flags: needinfo?(tomica)
Comment 42•8 years ago
|
||
On FF 53.0a2 (2017-02-22), if both "run_at" is set to "document_start" and "match_about_blank" is set to true, still injection does not happen at "document_start".
Assignee | ||
Comment 43•8 years ago
|
||
(In reply to Jeremy from comment #42)
> if both "run_at" is set to "document_start" and "match_about_blank" is set
> to true, still injection does not happen at "document_start".
Yes, this is a limitation of our implementation. The earliest that your script will be able to run is `document_end`, which is usually just one event loop tick later than `document_start` for "about:blank" documents.
The `run_at` is a bit of a misnomer, it was never a guarantee that the code will run *at* that time, but only that it will not run "before".
(In reply to Will Bamberg [:wbamberg] from comment #41)
> Does it look correct to you?
The part mentioning "about:srcdoc" URIs is incorrect. They do work with this patch.
The part about "inherited URL" is a bit misleading, since we don't actually do any recursive checks. Can you maybe try rewarding that using terms like "owning document" and "origin", that I think we have used in other places?
Also, it might be worth noting the limitation about `document_start` from above.
Flags: needinfo?(tomica) → needinfo?(wbamberg)
Comment 44•8 years ago
|
||
(In reply to Tomislav Jovanovic :zombie from comment #43)
> Yes, this is a limitation of our implementation.
:zombie, this injection before the actual page is being loaded is very problematic at least for security & privacy extensions. For instance in my popup blocker, an script needs to be installed before others to be able to actually block annoying popups. Most ad-popup scripts do these simple tricks:
```js
var iframe = document.createElement('iframe');
document.body.appendChild(iframe);
var link = iframe.contentDocument.createElement('a');
link.target = '_blank';
link.src = 'http://example.com/';
link.click();
document.body.removeChild(iframe);
```
or
```js
var iframe = document.createElement('iframe');
document.body.appendChild(iframe);
iframe.contentWindow.open('http://example.com');
document.body.removeChild(iframe);
```
See them in action (Test 17/1, 17/2, 17/3)
http://tools.add0n.com/popup-blocker.html
So in all these tests, my extension (https://addons.mozilla.org/firefox/addon/popup-blocker/) cannot block the popups as it is not even being injected in these dynamically created blank iframes after they get attached to the body
These tests pass just fine in the Chrome version (https://chrome.google.com/webstore/detail/aefkmifgmaafnojlojpnekbpbmjiiogg) though
Assignee | ||
Comment 45•8 years ago
|
||
In that case, can you please file a separate bug for this issue so it could be discussed and prioritized properly, as there is possibly a non-trivial amount of work needed.
Comment 46•8 years ago
|
||
Thanks for the feedback, :zombie. I've made all the changes you suggested except to talk about "origin", as we discussed.
Flags: needinfo?(wbamberg)
Comment 47•8 years ago
|
||
(In reply to Tomislav Jovanovic :zombie from comment #45)
> In that case, can you please file a separate bug for this issue so it could
> be discussed and prioritized properly, as there is possibly a non-trivial
> amount of work needed.
Done, https://bugzilla.mozilla.org/show_bug.cgi?id=1342418
Depends on: CVE-2018-18495
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•