1322782 - Content scripts match patterns take fragment identifiers into account

Status: VERIFIED FIXED

(WebExtensions :: Compatibility, defect, P1)

Description

[Maurício Kishi]

Attachment: firefox-webext-match-bug.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.75 Safari/537.36

Steps to reproduce:

Content scripts aren't being executed in urls with fragment identifiers (hashes).


Create a `manifest.json` file with the following contents:

```
{
	"manifest_version": 2,
	"name": "Hash-bug",
	"version": "1.0.0",

	"content_scripts": [{
		"matches": ["https://wiki.mozilla.org/WebExtensions"],
		"js": ["content.js"]
	}]
}
```


And a `content.js` file:

```
alert('`content.js` executed')
```

(there are attached as a zip)

Load the extension and open "https://wiki.mozilla.org/WebExtensions" and "https://wiki.mozilla.org/WebExtensions#some-fragment".


Actual results:

The content script wasn't executed on "https://wiki.mozilla.org/WebExtensions#some-fragment".


Expected results:

Both pages should have triggered the content script and shown the alert. That's also Chrome's behavior.


ps. I've tested this on both Firefox Stable and Developer Edition.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1322782 - ContentScripts MatchPattern should ignore the URL fragment.

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

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

The attached patch pushed to try:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=3674ef3e1e724f5fadf1d3e4d66ed926ea122b16

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8820049 [details]
Bug 1322782 - ContentScripts MatchPattern should ignore the URL fragment.

https://reviewboard.mozilla.org/r/99582/#review100304

r=me with the ref-based matches removed entirely

::: toolkit/modules/addons/MatchPattern.jsm:98
(Diff revision 1)
> +        this.pathMatch(uri.cloneIgnoringRef().path) ||
> +        this.pathMatch(uri.path)

No need to check both. Just ignore the ref entirely.

::: toolkit/modules/tests/xpcshell/test_MatchPattern.js:94
(Diff revision 1)
>    pass({url:"http://mozilla.org", pattern:["http://mozilla.org/", "http://mozilla.com/"]});
>    pass({url:"http://mozilla.com", pattern:["http://mozilla.org/", "http://mozilla.com/"]});
>    fail({url:"http://mozilla.biz", pattern:["http://mozilla.org/", "http://mozilla.com/"]});
> +
> +  // Match url with fragments.
> +  pass({url:"http://mozilla.org/base#some-fragment", pattern:"http://mozilla.org/base"});

Missing spaces after ":"

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8820049 [details]
Bug 1322782 - ContentScripts MatchPattern should ignore the URL fragment.

https://reviewboard.mozilla.org/r/99582/#review100304

> No need to check both. Just ignore the ref entirely.

are you sure of this? 
It looks like that by removing the second check (`this.pathMatch(uri.path)`) then we break it the other way around,
(e.g. this test doesn't pass anymore: `pass({url:"http://mozilla.org/base#some-fragment", pattern:"http://mozilla.org/base#some-fragment"});` and it looks like it should), am I wrong?

> Missing spaces after ":"

I usually add a space after the ":", but I didn't added it on the new lines in this test so that they are consistent with the rest of the file (none of the others ":" are currently spaced)

do you want me to add it only on the new one?
(I'm not completely against, just asking).

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8820049 [details]
Bug 1322782 - ContentScripts MatchPattern should ignore the URL fragment.

https://reviewboard.mozilla.org/r/99582/#review100304

> are you sure of this? 
> It looks like that by removing the second check (`this.pathMatch(uri.path)`) then we break it the other way around,
> (e.g. this test doesn't pass anymore: `pass({url:"http://mozilla.org/base#some-fragment", pattern:"http://mozilla.org/base#some-fragment"});` and it looks like it should), am I wrong?

Yes, you'll need to remove those tests.

> I usually add a space after the ":", but I didn't added it on the new lines in this test so that they are consistent with the rest of the file (none of the others ":" are currently spaced)
> 
> do you want me to add it only on the new one?
> (I'm not completely against, just asking).

Around all of them. I'm not sure why ESLint is currently allowing this, but it shouldn't be.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1322782 - Fixed coding style in MatchPattern tests.

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

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8820049 [details]
Bug 1322782 - ContentScripts MatchPattern should ignore the URL fragment.

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

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8820778 [details]
Bug 1322782 - Fixed coding style in MatchPattern tests.

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

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8820049 [details]
Bug 1322782 - ContentScripts MatchPattern should ignore the URL fragment.

https://reviewboard.mozilla.org/r/99582/#review100304

> Yes, you'll need to remove those tests.

Thanks for the confirmation, I've removed the second check and the related assertions in the test.

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8820049 [details]
Bug 1322782 - ContentScripts MatchPattern should ignore the URL fragment.

https://reviewboard.mozilla.org/r/99582/#review100304

> Around all of them. I'm not sure why ESLint is currently allowing this, but it shouldn't be.

It is weird, this directory doesn't seem to have any explicit configuration that ignore the "key-spacing" eslint rule.

In the meantime I've added the space to all the other assertions in a separate commit added to this mozreview queue (only to keep the actual fix readable and separate from the conding style fix).

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

"flu"-related typo: conding style -> coding style

Comment 12

[Dave Royal]

Does this bug explain why a content script I wrote today works in Chromium and not in Fx 50?

Manifest: "matches": ["http://www.ybw.com/forums/*"]
Page: http://www.ybw.com/forums/mobile.php?do=gridmenu

Works in Fx 52.0a2 (2016-12-21) (64-bit)

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8820778 [details]
Bug 1322782 - Fixed coding style in MatchPattern tests.

https://reviewboard.mozilla.org/r/100224/#review100800

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f55678bd8ac
ContentScripts MatchPattern should ignore the URL fragment. r=kmag
https://hg.mozilla.org/integration/autoland/rev/012ca1fe0548
Fixed coding style in MatchPattern tests. r=kmag

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f55678bd8ac
https://hg.mozilla.org/mozilla-central/rev/012ca1fe0548

Comment 16

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 53.0a1 (2016-12-13) under Windows 10 64-bit.

Verified fixed on Firefox 53.0a1 (2016-12-27) under Windows 10 64-bit and Ubuntu 16.04 32-bit. The content script is successfully triggered on both pages.

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8820049 [details]
Bug 1322782 - ContentScripts MatchPattern should ignore the URL fragment.

Approval Request Comment

[Feature/Bug causing the regression]:
WebExtensions Content Scripts are supposed to ignore
the URL fragments when they are applied by "a matched url pattern", and this bug (Bug 1322782) has introduced on Firefox 53 the changes needed to MatchPattern, so that it can behave as expected.

[User impact if declined]:
WebExtensions with content scripts are going to do not inject their content scripts into the expected URLs, when they are matched by URL and the current url contains a fragment (and Chrome Extensions ported to a WebExtension are likely to experience compatibility issues)  

[Is this code covered by automated tests?]:
Yes, the affected component was already covered by automated tests, the change also adds additional test cases to test that the fixed version behaves as expected with URLs that include a fragment. 

[Has the fix been verified in Nightly?]:
Yes, the fix has been landed on Nightly (as Firefox 53) and it has been on Aurora for a while.

[Needs manual test from QE? If yes, steps to reproduce]: 
No, the fix has been manually verified on Nightly, the automated tests verify the behavior of the component, and that the fix has been applied correctly.
(Nevertheless the fix is easily reproducible and it can be verified manually)

[List of other uplifts needed for the feature/fix]:
- https://bugzilla.mozilla.org/attachment.cgi?id=8820049 (this patch, which contains the actual fix and additional test case)
- https://bugzilla.mozilla.org/attachment.cgi?id=8820778 (test only, it fixes the coding style of the test file)

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
The fix on the MatchPattern component is small and limited to a single line, the existent test coverage cover the known behaviors, and the fix also add the needed additional test cases to the test suite.

[String changes made/needed]:
No

Comment 18

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8820049 [details]
Bug 1322782 - ContentScripts MatchPattern should ignore the URL fragment.

webextensions MatchPattern fix, beta52+

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e39019e1dffa
https://hg.mozilla.org/releases/mozilla-beta/rev/ef2f58069641

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/e39019e1dffa
https://hg.mozilla.org/releases/mozilla-esr52/rev/ef2f58069641

Comment 21

[Andrei Vaida [:avaida]]

Taking Comment 17 into consideration and the fact that this patch has automated coverage, I don't think manual testing would bring enough value to justify an increase in our current bug verification efforts -- setting qe-verify-.

Luca, if you think it's the other way around, don't hesitate to flip the flag and/or ni? me.