1272890 - Support match_about_blank in content scripts

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Jérémie Bertrand]

Attachment: 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.

Comment 4

[Tomislav Jovanovic :zombie]

Attachment: bug 1272890 - implement match_about_blank for content scripts

Review commit: https://reviewboard.mozilla.org/r/85098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85098/

Comment 5

[Tomislav Jovanovic :zombie]

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.

Comment 6

[Tomislav Jovanovic :zombie]

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.

Comment 7

[Tomislav Jovanovic :zombie]

Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85098/diff/1-2/

Comment 8

[Wladimir Palant]

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.

Comment 9

[Tomislav Jovanovic :zombie]

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

Comment 10

[Tomislav Jovanovic :zombie]

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

[Kris Maglione [:kmag]]

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.

Comment 12

[Tomislav Jovanovic :zombie]

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 13

[Tomislav Jovanovic :zombie]

Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85098/diff/2-3/

Comment 14

[Kris Maglione [:kmag]]

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/\^/!=/

Comment 15

[Tomislav Jovanovic :zombie]

Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85098/diff/3-4/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc266f8bc70a
implement match_about_blank for content scripts r=kmag

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cc266f8bc70a

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 19

[Tomislav Jovanovic :zombie]

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.

Comment 20

[Tomislav Jovanovic :zombie]

Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85098/diff/4-5/

Comment 21

[Kris Maglione [:kmag]]

Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts

https://reviewboard.mozilla.org/r/85098/#review87502

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a904e8a06a7
implement match_about_blank for content scripts r=kmag

Comment 23

[Tomislav Jovanovic :zombie]

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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 25

[Tomislav Jovanovic :zombie]

Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85098/diff/5-6/

Comment 26

[Tomislav Jovanovic :zombie]

Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85098/diff/6-7/

Comment 27

[Kris Maglione [:kmag]]

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

Comment 28

[Tomislav Jovanovic :zombie]

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 29

[Tomislav Jovanovic :zombie]

Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85098/diff/7-8/

Comment 30

[Kris Maglione [:kmag]]

Comment on attachment 8800081 [details]
bug 1272890 - implement match_about_blank for content scripts

https://reviewboard.mozilla.org/r/85098/#review89050

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33f28f5ab038
implement match_about_blank for content scripts r=kmag

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/33f28f5ab038

Comment 33

[AK]

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.

Comment 34

[Tomislav Jovanovic :zombie]

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

[CosminB]

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

Comment 36

[Tomislav Jovanovic :zombie]

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

Comment 37

[CosminB]

(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

[AK]

(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.

Comment 39

[Tomislav Jovanovic :zombie]

(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?

Comment 40

[Andy McKay]

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.

Comment 41

[Will Bamberg [:wbamberg]]

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?

Comment 42

[Jeremy]

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".

Comment 43

[Tomislav Jovanovic :zombie]

(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.

Comment 44

[Jeremy]

(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

Comment 45

[Tomislav Jovanovic :zombie]

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

[Will Bamberg [:wbamberg]]

Thanks for the feedback, :zombie. I've made all the changes you suggested except to talk about "origin", as we discussed.

Comment 47

[Jeremy]

(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