1385864 - Web extension with "chrome://favicon/" permissions in manifest breaks webrequest

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Rustam]

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

Steps to reproduce:

I am using the following code for test:
browser.webRequest.onBeforeSendHeaders.addListener(
	event => {
		console.log( 'onBeforeSendHeaders' );
		console.log( event );

		return;
	},
	{ 'urls': [ '<all_urls>' ] },
	[ 'blocking', 'requestHeaders' ]
);

This code in Embedded WebExtension, not pure webextension


Actual results:

console.log is not outputted in FF dev edition.
In current normal Firefox all is working


Expected results:

It must work!

Comment 1

[Rustam]

55.0b13 64 bit

Comment 2

[Rustam]

My manifest permissions: 
"permissions": [
    "background",
    "proxy",
    "storage",
    "webRequest",
    "webRequestBlocking",
    "<all_urls>"
  ],

Comment 3

[Rustam]

Minimal code for testing: 

manifest.json

{
  "manifest_version": 2,
  "name": "dev",
  "short_name": "dev",
  "version": "3.16.8",
  "description": "description",
  "background": {
    "scripts": [
      "background.js"
    ]
  },
	"icons": {
    "64": "app_icon64.png"
  },
	
  "permissions": [
    "background",
    "proxy",
    "storage",
    "webRequest",
    "webRequestBlocking",
    "<all_urls>"
  ],
  "optional_permissions": [
    "management",
    "chrome://favicon/"
  ],
  "content_security_policy": "script-src 'self' https://ssl.google-analytics.com; object-src 'self'"
}

Comment 4

[Rustam]

background.js

console.log( 'background start' );

browser.webRequest.onBeforeSendHeaders.addListener(
  event => {
    console.log( 'onBeforeSendHeaders' );
    console.log( event );

    return;
  },
  { 'urls': [ '<all_urls>' ] },
  [ 'blocking', 'requestHeaders' ]
);

console.log( 'background test 1' );

Comment 5

[Rustam]

By chat conservations: in just FF beta there is no bug, in FF dev edition there is bug

Comment 6

[Rustam]

*conversations

Comment 7

[Rustam]

Personally tested in pure Firefox Beta - this test (above) does not work for me

Comment 8

[Tomislav Jovanovic :zombie]

The issue is with your manifest, firefox doesn't support the "chrome://favicon/" permission, so your extension doesn't even load.

Unfortunately, the error is invisible unless you are using the current Nightly (which is generally suggested while developing).

Comment 10

[Rustam]

What it's working in FF54?

Comment 11

[Rustam]

*Why

Comment 12

[Tomislav Jovanovic :zombie]

Not sure, we probably started being strict about permissions in 55.

Comment 13

[Andy McKay]

Sound like bug 1373293 which was fixed in 56.

Comment 14

[Tomislav Jovanovic :zombie]

But that says it got uplifted to 55, which doesn't match my testing.

Reopening since it seems the fix for bug 1373293 missed optional_permissions, and this is breaking webrequests as is.

Comment 15

[Tomislav Jovanovic :zombie]

Attachment: Bug 1385864 - Drop invalid optional_permissions from manifests

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

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8893597 [details]
Bug 1385864  - Drop invalid optional_permissions from manifests

https://reviewboard.mozilla.org/r/164684/#review170098

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca0419d7845f
Drop invalid optional_permissions from manifests  r=kmag

Comment 18

[Tomislav Jovanovic :zombie]

Comment on attachment 8893597 [details]
Bug 1385864  - Drop invalid optional_permissions from manifests

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1322235
[User impact if declined]: This prevents certain extensions from being installed.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: No, manually tested from local build.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low-risk.
[Why is the change risky/not risky?]: it's very simple, just marks unknown strings to be dropped from manifest.
[String changes made/needed]: None.

Comment 19

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8893597 [details]
Bug 1385864  - Drop invalid optional_permissions from manifests

Looks like we need this for webextensions to work - let's uplift for 56 beta 1.

Comment 20

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/739ff1fa9eaf

Comment 22

[Andrei Vaida [:avaida]]

(In reply to Tomislav Jovanovic :zombie from comment #18)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: No.
> [Needs manual test from QE? If yes, steps to reproduce]: No, manually tested
> from local build.

Setting qe-verify- based on Tomislav's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 23

[Kuba Niewiarowski]

Related to bug 1315616?