Closed Bug 1373293 Opened 3 years ago Closed 2 years ago

MatchPattern exception on chrome://favicon/

Categories

(WebExtensions :: Untriaged, defect, P1)

defect

Tracking

(firefox54- unaffected, firefox55+ fixed, firefox56+ fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox54 - unaffected
firefox55 + fixed
firefox56 + fixed

People

(Reporter: andy+bugzilla, Assigned: kmag)

Details

(Whiteboard: [triaged])

Attachments

(2 files)

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

The extension tree tabs: 

https://addons.mozilla.org/en-US/firefox/addon/tree-tabs/

Won't install in Nightly or Beta. Removing the permission:

"chrome://favicon/"

Let's it install. The exception is:

Exception { message: "", result: 2147942487, name: "NS_ERROR_ILLEGAL_VALUE", filename: "resource://gre/modules/Extension.jsm", lineNumber: 566, columnNumber: 0, data: null, stack: "loadManifest@resource://gre/modules…", location: XPCWrappedNative_NoHelper }
Nightly is 56 and Beta is 55. I can install the addon in 54. Mark 54 back to unaffected.
tracking as breaking a popular extension
There's my proposal (reviewboard)...take it or leave it.  I think it's fine to continue addon install if a bad/unsupported url is in permissions.  In this case, it's one less papercut.
Attachment #8878581 - Flags: review?(kmaglione+bmo)
Comment on attachment 8878581 [details]
Bug 1373293 ignore and report invalid origin permissions,

https://reviewboard.mozilla.org/r/149902/#review154588
Attachment #8878581 - Flags: review?(aswan) → review+
Attachment #8878581 - Flags: review?(kmaglione+bmo)
Assignee: kmaglione+bmo → mixedpuppy
Should we add a unit test for this?
I'd rather we just change the schema code to drop permissions that aren't valid. We should already be warning about this unknown permission, so adding a check here means we'll wind up warning twice.
Since Shane is out this week, bouncing over to Kris to either land the patch as is, or improve.
Assignee: mixedpuppy → kmaglione+bmo
Priority: -- → P1
Whiteboard: [triaged]
I received few emails, asking why my extension cannot install, I was scratching my head for few days already without a clue why and what is going on. Finally I found the exception code which led me here.

Frustrating thing is that it is not showing anything. Just error that extension is corrupt.
I couldn't run it in debug to find out what is wrong, it just does not load.
For now I think you should just ignore invalid permissions, and in the future if you want to block, you should make some warning, like for all other errors. Not just straight "this extensions is corrupt"...

In Chromium, "chrome://favicon/" is needed to access internal and extensions favicons via url.
To access internal favicons, pattern is: "chrome://favicon/" + tab.url.
Without it I have no permission to load favicons from extensions tabs, this is really important for "tabs suspending extensions" that capture original favicon and change opacity to make "unloaded" effect.
Moreover, if I don't add this, console throws billions "Not allowed to load local resource:chrome://xxxxxx".
Anyway this method is used in many extensions like vTabs, Sidewise, Tabs Outliner and so on.
It did not give any errors in Firefox, so I left it there.

Firefox, internal pages still will not allow me to get favicons, so I have to find another way.
Anyone knows how to do that, since I'm not familiar with Firefox APIs? Something like "about" does not work either...
Comment on attachment 8880096 [details]
Bug 1373293: Drop invalid permissions when normalizing manifests.

https://reviewboard.mozilla.org/r/151452/#review156392

Looks good. I just have one question about the test. r+ whether you decide to update the test or not.

::: toolkit/components/extensions/test/xpcshell/test_ext_unknown_permissions.js:20
(Diff revision 1)
> +  await extension.startup();
> +
> +  const {WebExtensionPolicy} = Cu.import("resource://gre/modules/Extension.jsm", {});
> +
> +  let policy = WebExtensionPolicy.getByID(extension.id);
> +  Assert.deepEqual(policy.permissions, ["activeTab", "http://*/*"]);

Do we want to assert that the warning was logged as well? I assume the code added to Schemas.jsm will cause a warning to be logged to the console.
Attachment #8880096 - Flags: review?(bob.silverberg) → review+
Comment on attachment 8880096 [details]
Bug 1373293: Drop invalid permissions when normalizing manifests.

https://reviewboard.mozilla.org/r/151452/#review156392

> Do we want to assert that the warning was logged as well? I assume the code added to Schemas.jsm will cause a warning to be logged to the console.

Yeah, I suppose I should.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b04ed6a0a52af869867351bc0a10e86033e718e8
Bug 1373293: Drop invalid permissions when normalizing manifests. r=bsilverberg
Comment on attachment 8880096 [details]
Bug 1373293: Drop invalid permissions when normalizing 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, tests should be sufficient.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low-risk.
[Why is the change risky/not risky?]: The change is relatively simple, and mainly just drops permission strings that aren't understood.
[String changes made/needed]: None.
Attachment #8880096 - Flags: approval-mozilla-beta?
Comment on attachment 8880096 [details]
Bug 1373293: Drop invalid permissions when normalizing manifests.

add-on compat fix for 55.0b5
Attachment #8880096 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I am now on Nightly Version 56.0a1 (2017-06-23) (64-bit) -- doesn't that mean I should have the fix?
The "tree tabs" extension still doesn't install.
(In reply to Ralf Jung from comment #19)
> I am now on Nightly Version 56.0a1 (2017-06-23) (64-bit) -- doesn't that
> mean I should have the fix?
> The "tree tabs" extension still doesn't install.

Can you post the build source from about:buildconfig? It installs for me in the latest nightly.
"Built from https://hg.mozilla.org/mozilla-central/rev/594cc32b632396a867ef1f98428968b224d82151"

The Browser console says
```
1498239786635	addons.xpi	WARN	Download of https://addons.mozilla.org/firefox/downloads/latest/tree-tabs/platform:2/addon-809637-latest.xpi?src=api failed: [Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/Extension.jsm :: loadManifest :: line 572"  data: no] Stack trace: loadManifest()@resource://gre/modules/Extension.jsm:572
```
It sounds like you have cached schema data from an older version. I'll bump the cache schema version.
> It sounds like you have cached schema data from an older version.

I created this profile yesterday, so it has seen exactly one upgrade -- from yesterday's nightly to today's.

Is there any way for me to clear that cache, to test of this will help?
Extension is installing fine on today's nightly.
Ralf, I did an update 2 days ago and it was released yesterday. I didn't know when the patch was going to be released. I replaced permissions from "chrome://favicon/" to "<all_urls>".
Duplicate of this bug: 1385864
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.