1195353 - Firefox should not delete extension proxy files when the add-on is found to be broken

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Mindaugas <LA-MJ>]

Steps:
1) Use extension proxy file[1] for add-on development in aurora channel
2) Set xpinstall.signatures.required to false
3) Upgrade from FF41 to FF42

Result:
The proxy file was deleted on Firefox launch after upgrade

Expected:
The proxy file should have not been deleted. Add-on development should continue without a hitch.

[1] https://developer.mozilla.org/en-US/Add-ons/Setting_up_extension_development_environment#Firefox_extension_proxy_file

Comment 1

[Mindaugas <LA-MJ>]

The original description applies to a profile that was previously used for Firefox beta.
Additionally UNIX symlinks are removed as well.

The 'dev-edition-default' one must have some prefs set or something and has a different behavior. Namely, the file is not removed but the extension is not in the list anyway.

Comment 2

[Dave Townsend [:mossop]]

(In reply to 4mr.minj from comment #1)
> The original description applies to a profile that was previously used for
> Firefox beta.
> Additionally UNIX symlinks are removed as well.

FWIW symlinks have never been a supported way to add extensions into Firefox, still we shouldn't be deleting their contents.

Comment 3

[Robert Helmer [:rhelmer]]

On Firefox release (40.0) and Nightly (43.0a1) on Mac OS X with xpinstall.signatures.required=false, I see this in the console for a proxy add-on:

1439921975864	addons.xpi	WARN	Disabling foreign installed add-on @fox-candy in app-profile

In both cases I get the about:newaddon?id=@fox-candy dialog on startup, and the proxy add-on works fine if I allow the installation.

The only way I've been able to repro so far is if there is something wrong with the proxy file (e.g. ends in .xpi and the zip reader fails, path in proxy file does not contain manifest, etc.)

If you start Firefox from a terminal, do you see a message about why this add-on has been disabled?

Comment 4

[Dave Townsend [:mossop]]

Note that if you enable extensions.logging.enabled you'll see a bunch of messages from the add-ons manager which might reveal what is going on.

Comment 5

[Mindaugas <LA-MJ>]

This issue is too unpredictable. Proxy files are deleted only sometimes. However the addon is never listed among installed

Here's the output when proxy file is removed:
> addons.xpi	WARN	addMetadata: Add-on {myAddonID} is invalid: SyntaxError: JSON.parse: unexpected character at line 22 column 4 of the JSON data (resource://gre/modules/addons/XPIProvider.jsm:755:18) JS Stack trace: [e-mail]:755:18 < [e-mail]:1181:17 < [e-mail]:314:40 < [e-mail]:275:3 < createAsyncFunction/[e-mail]:249:14 < [e-mail]:1290:12 < [e-mail]:1301:3 < [e-mail]:3477:22 < [e-mail]:3758:23 < [e-mail]:3919:34 < [e-mail]:2413:25 < [e-mail]:221:12 < [e-mail]:703:5 < [e-mail]:874:9 < [e-mail]:2514:5 < [e-mail]:55:7

I get nothing in cases when it is not removed.

FYI there is nothing wrong with that proxy file: old nightly (42.0a1) accepts it but aurora 42.0a2 does not.

Comment 6

[Dave Townsend [:mossop]]

Ah interesting. We're finding a bad install manifest for the extension and so removing because of that (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#3488). What has changed in 42 is our new support for Web Extensions. Is your extension a Web Extension or is it just a standard extension with an install.rdf file?

Firefox is finding a manifest.json file in your extension, treating it as a Web Extension but seeing an error in the manifest and so rejecting the extension because of that.

It would make sense to not remove pointer files in that case, we should expect that extensions under development occasionally go invalid.

Comment 7

[Mindaugas <LA-MJ>]

No, it's not a web extension.

> <em:unpack>true</em:unpack>
> <em:type>2</em:type>
> <em:bootstrap>true</em:bootstrap>

Comment 8

[Dave Townsend [:mossop]]

Is there a manifest.json file in the extension's directory? If not that suggests something is broken with how we're detecting these

Comment 9

[Mindaugas <LA-MJ>]

BINGO. The same directory is used for chrome. You should test for install.rdf before that ;)

Comment 10

[Mindaugas <LA-MJ>]

By chrome I mean google chrome, of course. Google chrome extensions use manifest.json since forever. Forcing to use different checkouts between browsers would be very inconvenient.

Comment 11

[Dave Townsend [:mossop]]

So we're starting to support chrome-like extensions. The reason we're checking for manifest.json first right now is that it allows you to write an add-on that uses the chrome extension support in Firefox when available but still supports older Firefox that only support install.rdf. Maybe that's a mistake though, it seems likely that there will be many people in the same situation as you.

What are your thoughts Bill?

Comment 12

[Mindaugas <LA-MJ>]

I have an idea why manifest.json parsing fails then.

Google chrome allows to use comments in that file even though they are not allowed in the JSON spec.

Comment 13

[Dave Townsend [:mossop]]

(In reply to 4mr.minj from comment #12)
> I have an idea why manifest.json parsing fails then.
> 
> Google chrome allows to use comments in that file even though they are not
> allowed in the JSON spec.

That's going to be fun for us to support, filed bug 1196283 for that.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Let's check for install.rdf first. If people want to support old versions of Firefox, they'll have to use the maxVersion stuff and ship separate add-ons. I'm worried that lots of Firefox add-ons also happen to include manifest.json files. We don't want to start breaking them if our WebExtension support isn't up to par yet.

However, I think we should WONTFIX the comment issue. It doesn't make sense for us to try to be bug-compatible with Chrome.

Comment 15

[Dave Townsend [:mossop]]

Ok, I've spun off checking for install.rdf first into bug 1196301, that should be a straightforward fix we can uplift to 41. I'd like to keep this bug open for not deleting pointer files when the add-on is broken, that's still something we should fix, though it has been like it for a while so I don't think it is as important.

You can figure out the comments for manifest.json in bug 1196283

Comment 16

[Dave Townsend [:mossop]]

(In reply to Dave Townsend [:mossop] from comment #15)
> Ok, I've spun off checking for install.rdf first into bug 1196301, that
> should be a straightforward fix we can uplift to 41.

That should be 42, not 41

Comment 17

[Dave Townsend [:mossop]]

Feel like taking this Rob? It should just be a case of checking installLocation.isLinkedAddon before removing the add-on and writing tests. Might need to check the updateMetadata case too though.

Comment 18

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1195353 - leave extension proxy files that point to invalid manifests r?mossop

bug 1195353 - leave extension proxy files that point to invalid manifests r?mossop

Comment 19

[Robert Helmer [:rhelmer]]

I did not modify the updateMetadata function, because I cannot figure out any way to get an update check to run against an unpacked add-on (proxy files only work for unpacked add-ons, right?)

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 8692391 [details]
MozReview Request: bug 1195353 - leave extension proxy files that point to invalid manifests r?mossop

https://reviewboard.mozilla.org/r/26255/#review23979

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1635
(Diff revision 1)
> -      if (!aInstallLocation.locked)
> +      if (aInstallLocation.isLinkedAddon)

isLinkedAddon is a function

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1636
(Diff revision 1)
> -        aInstallLocation.uninstallAddon(aId);
> +        logger.warn("Could not uninstall invalid item because it is a proxy file");

I think a better warning is: "Not uninstalling invalid item because it is a proxy file"

Comment 21

[Robert Helmer [:rhelmer]]

Comment on attachment 8692391 [details]
MozReview Request: bug 1195353 - leave extension proxy files that point to invalid manifests r?mossop

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

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 8692391 [details]
MozReview Request: bug 1195353 - leave extension proxy files that point to invalid manifests r?mossop

https://reviewboard.mozilla.org/r/26255/#review24095

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8b459a753480

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/8b459a753480

Comment 25

[Marat Tanalin]

For what it's worth, the latest Developer Edition 51.0a2 (20161016004034) is still affected while Nightly 52.0a1 (20161016030205) is not.

For example, if `matches` entry inside `content_scripts` entry in addon's manifest contains `*.` instead of `*` as host (e.g. as a result of misreading documentation), the proxy file is deleted when starting Firefox Developer Edition. Entire relevant part of manifest:

	"content_scripts": [
		{
			"matches": ["https://*./*"],
			"js": ["example.js"],
			"run_at": "document_end"
		}
	],