1372448 - No new installation notification for sideload add-ons on <appid> directory

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[YF (Yang)]

STR:
1. Download the https://addons.mozilla.org/firefox/addon/bookmarks-organizer/ to "[e-mail]" file.
2. Place the WebExtension xpi into %appdata%\\Mozilla\Extensions\{ec8030f7-c20a-464f-9b0e-13a3a9e97384}\[e-mail] according to https://developer.mozilla.org/en-US/Add-ons/Installing_extensions#Windows (I know this document is outdated).


Actual results:
1. No new installation (sideload) notification in Menu button.
2. The addon appear in Add-ons Manger, disabled by default.


Expected results:
1. A installation notification to remind for activation, just like place the xpi into [profile]/extensions.
2. Notice if the add-ons is not compatible, unsigned, or damaged?


Additional question:
1. If the WebExtension no specified the applications - gecko - id, it is impossible to be sideload? e.g. https://addons.mozilla.org/firefox/addon/smartup/.

Comment 1

[Andrew Swan [:aswan]]

So the problem is here:
http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/toolkit/mozapps/extensions/internal/XPIProviderUtils.js#1281-1282

The simple test for "is this a new profile" was broken by bug 1356826.
That logic was originally introduced in bug 1249074, but since that time we switched from an about:newaddon tab for each sideloaded extension to the less intrusive notification-on-the-hamburger-menu flow.  I'm also unclear on the original rationale for bug 1249074, relying on the user knowing to go to about:addons seems bad to me.

The expedient fix here would be to set seen to false unconditionally which would fix this bug and would also mean that during the first run of a new profile, the new sideload flow would be invoked for any system-installed extensions.  I suppose the alternative is to add another parameter to processFileChanges() to indicate whether this is a new profile and plumb that through to addMetadata().  I have a slight preference for the former, but Dave/Rob, what say you?

Comment 2

[Dave Townsend [:mossop]]

(In reply to Andrew Swan [:aswan] from comment #1)
> So the problem is here:
> http://searchfox.org/mozilla-central/rev/
> 61054508641ee76f9c49bcf7303ef3cfb6b410d2/toolkit/mozapps/extensions/internal/
> XPIProviderUtils.js#1281-1282
> 
> The simple test for "is this a new profile" was broken by bug 1356826.

I imagine so.

> That logic was originally introduced in bug 1249074, but since that time we
> switched from an about:newaddon tab for each sideloaded extension to the
> less intrusive notification-on-the-hamburger-menu flow.  I'm also unclear on
> the original rationale for bug 1249074, relying on the user knowing to go to
> about:addons seems bad to me.

The original rationale was that new users of Firefox shouldn't be bothered with existing add-ons on their system. If their apps wanted them to know about it they could tell them where to go.

> The expedient fix here would be to set seen to false unconditionally which
> would fix this bug and would also mean that during the first run of a new
> profile, the new sideload flow would be invoked for any system-installed
> extensions.

This is a product question.

Comment 3

[Andrew Swan [:aswan]]

Kev, any strong feelings on the latter bit of comment 1?

Comment 4

[Andrew Swan [:aswan]]

Unassigning myself until we get clarity about the desired behavior.

Comment 6

[Sergey Shchennikov]

Any update? Its also reproduce with foreign install via windows registry (HKLM\Software\Mozilla\Firefox\Extensions). 
Will this bug appear in 55 release? No third-party installation possible at this moment. Any workaround?

Comment 7

[Kev Needham [:kev]]

Ugh, didn't realize this affected all side-loaded add-ons regardless of how they were installed.

Definitely need to fix this ASAP. Expedient fix in comment #1 makes the most sense.

Can we get this landed and uplifted? It breaks a lot of install flows.

Comment 8

[:shell escalante]

need info'ing aswan for his availability to take this bug/timing based on comment 7 - which is the PM decision on desired behavior.

Comment 9

[Andrew Swan [:aswan]]

i'm on it

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Tracking as I have been told that it is important

Comment 11

[Andrew Swan [:aswan]]

Attachment: Bug 1372448 Remove broken "new profile" check when determining if an addon is sideloaded

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

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8896634 [details]
Bug 1372448 Remove broken "new profile" check when determining if an addon is sideloaded

https://reviewboard.mozilla.org/r/167916/#review173094

::: toolkit/mozapps/extensions/test/xpcshell/test_seen.js:19
(Diff revision 1)
> -// By default disable add-ons from the profile
> +createAppInfo("[e-mail]", "XPCShell", "1", "1.9.2");
> -Services.prefs.setIntPref("extensions.autoDisableScopes", AddonManager.SCOPE_PROFILE);
>  
>  // Installing an add-on through the API should mark it as seen
>  add_task(async function() {
> +  startupManager();

Nit: `await promiseStartupManager()`

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8896634 [details]
Bug 1372448 Remove broken "new profile" check when determining if an addon is sideloaded

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/039b0e260e3c
Remove broken "new profile" check when determining if an addon is sideloaded r=kmag

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8896634 [details]
Bug 1372448 Remove broken "new profile" check when determining if an addon is sideloaded

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1356826

[User impact if declined]:
users will not be offered sideloaded extensions installed anywhere outside of individual profiles (eg, those installed in system-wide locations).  system-wide sideloading is not uncommon and it is broken without this fix.

[Is this code covered by automated tests?]:
the patch includes a new xpcshell test

[Has the fix been verified in Nightly?]:
not yet

[Needs manual test from QE? If yes, steps to reproduce]: 
manual verification would be good, STR are in the original bug report

[List of other uplifts needed for the feature/fix]:
none

[Is the change risky?]:
not especially

[Why is the change risky/not risky?]:
the actual code change is small and in a code path that only applies to searching for sideloaded extensions.  the change loosens limits on which extensions we offer to users which could be annoying if we show too many but that's the worst consequence.

[String changes made/needed]:
none

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8896634 [details]
Bug 1372448 Remove broken "new profile" check when determining if an addon is sideloaded

Ok, fix some partner issues.
Taking it in 56.0b3 and 55.0.2
Bravo for this work during the weekend

Comment 17

[Sylvestre Ledru [:Sylvestre]]

RyanVM, if you could land it, that would be awesome :)

Comment 18

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

Backed out for failing modified xpcshell toolkit/mozapps/extensions/test/xpcshell/test_seen.js on Windows:

https://hg.mozilla.org/integration/autoland/rev/bbd9bea14824338b1918c6973e5bc91b16c5609a

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=039b0e260e3c5ab254780b539fa9fe642e5e05db&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122823821&repo=autoland

07:05:09     INFO -  TEST-PASS | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_seen.js |  - true == true
07:05:09     INFO -  TEST-PASS | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_seen.js |  - true == true
07:05:09     INFO -  PID 7264 | 1502607908741	addons.manager	DEBUG	shutdown
07:05:09     INFO -  PID 7264 | 1502607908741	addons.manager	DEBUG	Calling shutdown blocker for LightweightThemeManager
07:05:09     INFO -  PID 7264 | 1502607908741	addons.manager	DEBUG	Calling shutdown blocker for GMPProvider
07:05:09     INFO -  PID 7264 | 1502607908742	addons.manager	DEBUG	Calling shutdown blocker for PluginProvider
07:05:09     INFO -  PID 7264 | 1502607908742	addons.manager	DEBUG	Calling shutdown blocker for XPIProvider
07:05:09     INFO -  PID 7264 | 1502607908742	addons.xpi	DEBUG	shutdown
07:05:09     INFO -  PID 7264 | 1502607908742	addons.xpi-utils	DEBUG	shutdown
07:05:09     INFO -  "CONSOLE_MESSAGE: (info) 1502607908739	addons.xpi-utils	DEBUG	Async JSON file read took 0 MS"
07:05:09     INFO -  "CONSOLE_MESSAGE: (info) 1502607908739	addons.xpi-utils	DEBUG	Finished async read of XPI database, parsing..."
07:05:09     INFO -  "CONSOLE_MESSAGE: (info) 1502607908740	addons.xpi-utils	DEBUG	Successfully read XPI database"
07:05:09     INFO -  "CONSOLE_MESSAGE: (info) 1502607908741	addons.manager	DEBUG	shutdown"
07:05:09     INFO -  "CONSOLE_MESSAGE: (info) 1502607908741	addons.manager	DEBUG	Calling shutdown blocker for LightweightThemeManager"
07:05:09     INFO -  "CONSOLE_MESSAGE: (info) 1502607908741	addons.manager	DEBUG	Calling shutdown blocker for GMPProvider"
07:05:09     INFO -  "CONSOLE_MESSAGE: (info) 1502607908742	addons.manager	DEBUG	Calling shutdown blocker for PluginProvider"
07:05:09     INFO -  "CONSOLE_MESSAGE: (info) 1502607908742	addons.manager	DEBUG	Calling shutdown blocker for XPIProvider"
07:05:09     INFO -  "CONSOLE_MESSAGE: (info) 1502607908742	addons.xpi	DEBUG	shutdown"
07:05:09     INFO -  "CONSOLE_MESSAGE: (info) 1502607908742	addons.xpi-utils	DEBUG	shutdown"
07:05:09     INFO -  PID 7264 | 1502607908747	addons.manager	DEBUG	Async provider shutdown done
07:05:09     INFO -  Unexpected exception NS_ERROR_FILE_ACCESS_DENIED: Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]
07:05:09     INFO -  @Z:/task_1502606389/build/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell/test_seen.js:91:3
07:05:09     INFO -  async*asyncFunction@resource://gre/modules/Task.jsm:241:18
07:05:09     INFO -  Task_spawn@resource://gre/modules/Task.jsm:166:12
07:05:09     INFO -  _run_next_test@Z:\task_1502606389\build\tests\xpcshell\head.js:1488:9
07:05:09     INFO -  run@Z:\task_1502606389\build\tests\xpcshell\head.js:701:9
07:05:09     INFO -  _do_main@Z:\task_1502606389\build\tests\xpcshell\head.js:221:3
07:05:09     INFO -  _execute_test@Z:\task_1502606389\build\tests\xpcshell\head.js:544:5
07:05:09     INFO -  @-e:1:1
07:05:09     INFO -  exiting test
07:05:09     INFO -  "CONSOLE_MESSAGE: (info) 1502607908747	addons.manager	DEBUG	Async provider shutdown done"
07:05:09     INFO -  NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIFile.remove]
07:05:09     INFO -      @Z:/task_1502606389/build/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell/test_seen.js:66:5
07:05:09     INFO -      _execute_test/<@Z:\task_1502606389\build\tests\xpcshell\head.js:604:15
07:05:09     INFO -      _execute_test@Z:\task_1502606389\build\tests\xpcshell\head.js:601:3
07:05:09     INFO -      @-e:1:1
07:05:09     INFO -  Error: Found unexpected files in temporary directory: systemwide-extensions at resource://testing-common/AddonTestUtils.jsm:311
07:05:09     INFO -      init/<@resource://testing-common/AddonTestUtils.jsm:311:15
07:05:09     INFO -      _execute_test/<@Z:\task_1502606389\build\tests\xpcshell\head.js:604:15
07:05:09     INFO -      _execute_test@Z:\task_1502606389\build\tests\xpcshell\head.js:601:3
07:05:09     INFO -      @-e:1:1

Comment 19

[Andrew Swan [:aswan]]

Comment on attachment 8896634 [details]
Bug 1372448 Remove broken "new profile" check when determining if an addon is sideloaded

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

Comment 20

[Andrew Swan [:aswan]]

I pushed a new version that might address the Windows failures but buildbot windows builds are busted right now and I can't seem to manage to get the test to run on taskcluster.  I have to drop offline right now, if anybody wants to give this a try on windows, please be my guest.
When I'm back online, if I still can't get a try run, I'll just land this with the test disabled on windows for now since there's some pressure to get this landed and I think the failures represent issues with the test rather than a product flaw.

Comment 21

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

Windows buildbot jobs are expected to be busted on Try with mozilla-central because these builds are in Taskcluster. They are just still there if someone pushes from a branch which still uses buildbot. The job which is important for you is the build on Windows 2012 opt (which is green on your pushes). I requested a bunch of Windows XPCshell runs for your latest push.

Comment 22

[Kris Maglione [:kmag]]

At this point, I recommend that you just land it. I'll vouch for your fix, that it should solve the problem. If it doesn't, we can disable the test and fix it in a follow-up.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/946e48295eb0
Remove broken "new profile" check when determining if an addon is sideloaded r=kmag

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/946e48295eb0

Comment 25

[Kev Needham [:kev]]

Thanks to everyone for jumping on this and getting a fix in over the weekend. Awesome.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1ad3a57bcf6a

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/ef72d049bdac

Comment 28

[Emil Ghitta, QA [:emilghitta]]

We tried to reproduce this issue on 55.0.2-build1 as well, but we didn't managed to do so by following the steps mentioned in comment 0 (even with an affected build). Therefore we can't confirm if this issue was fixed or not.

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with:
Fix an issue with new installation notification for sideload add-ons

Comment 30

[Vasilica Tamas, QA [:vtamas]]

Attachment: Animation.gif

I was able to reproduce the initial issue on Firefox 55.0.1 (20170809080026) under Windows 10 64-bit.

Verified as fixed on Firefox 57.0a1 (20417-08-16), Firefox 56.0b3 (20170815141045) and Firefox 55.0.2 (20170814072924) under Windows 10 64-bit. The yellow notification and the installation pop-up are successfully displayed for legacy add-ons and webextensions installed via Windows registry key. See attached screencast.

Notice that legacy add-ons and unsigned extensions are automatically displayed in Legacy Extensions tab and there is no installation pop-up prompted for them while testing on Firefox 57.0a1.