Support match_about_blank in content scripts

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: Jérémie Bertrand, Assigned: zombie)

Tracking

(Blocks: 1 bug)

48 Branch
mozilla52
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Created attachment 8752503 [details]
Web-Ext - bug isolation.zip

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

a year ago
Component: Untriaged → WebExtensions
OS: Unspecified → Windows 10
Product: Firefox → Toolkit
Hardware: Unspecified → x86_64

Updated

a year ago
Priority: -- → P3
Whiteboard: triaged
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
Duplicate of this bug: 1285185
Duplicate of this bug: 1285185
Duplicate of this bug: 1286002
Comment hidden (mozreview-request)
(Assignee)

Comment 5

11 months 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

11 months 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

11 months ago
Attachment #8800081 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)

Comment 8

10 months 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: 1226547
(Assignee)

Comment 9

10 months 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

10 months 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

10 months 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)

Updated

10 months ago
Blocks: 1310331
(Assignee)

Comment 12

10 months 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

10 months 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

10 months ago
Keywords: checkin-needed

Comment 16

10 months 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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cc266f8bc70a
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Updated

10 months ago
Depends on: 1312161
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

10 months 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)
status-firefox52: fixed → ---
Target Milestone: mozilla52 → ---
(Assignee)

Updated

10 months ago
Depends on: 1312357
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8800081 - Flags: review+ → review?(kmaglione+bmo)

Comment 21

10 months 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

10 months ago
Keywords: checkin-needed

Comment 22

10 months 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

10 months 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
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

10 months ago
Flags: needinfo?(tomica)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8800081 - Flags: review+ → review?(kmaglione+bmo)

Comment 27

10 months 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

10 months 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

10 months 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

10 months ago
Keywords: checkin-needed

Comment 31

10 months 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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/33f28f5ab038
Status: REOPENED → RESOLVED
Last Resolved: 10 months ago10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Updated

10 months ago
Blocks: 1315553
(Assignee)

Updated

10 months ago
No longer blocks: 1315553

Comment 33

10 months 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

10 months 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

10 months 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

10 months 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

10 months 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

10 months 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

9 months 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

9 months 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)
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

6 months 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

6 months 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

6 months 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

6 months 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.
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

6 months 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
You need to log in before you can comment on or make changes to this bug.