Closed Bug 1372448 Opened 7 years ago Closed 7 years ago

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

Categories

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

55 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox55 + verified
firefox56 + verified
firefox57 --- verified

People

(Reporter: yfdyh000, Assigned: aswan)

References

Details

(Whiteboard: investigating)

Attachments

(2 files)

STR:
1. Download the https://addons.mozilla.org/firefox/addon/bookmarks-organizer/ to "bookmarksorganizer@agenedia.com.xpi" file.
2. Place the WebExtension xpi into %appdata%\\Mozilla\Extensions\{ec8030f7-c20a-464f-9b0e-13a3a9e97384}\bookmarksorganizer@agenedia.com.xpi 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/.
Assignee: nobody → aswan
Whiteboard: investigating
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?
Flags: needinfo?(rhelmer)
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
Kev, any strong feelings on the latter bit of comment 1?
Flags: needinfo?(kev)
Flags: needinfo?(rhelmer)
Unassigning myself until we get clarity about the desired behavior.
Assignee: aswan → nobody
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?
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.
Flags: needinfo?(kev)
need info'ing aswan for his availability to take this bug/timing based on comment 7 - which is the PM decision on desired behavior.
Flags: needinfo?(aswan)
i'm on it
Assignee: nobody → aswan
Flags: needinfo?(aswan)
Priority: -- → P1
Tracking as I have been told that it is important
Attachment #8896634 - Flags: review?(kmaglione+bmo)
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("xpcshell@tests.mozilla.org", "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()`
Attachment #8896634 - Flags: review?(kmaglione+bmo) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/039b0e260e3c
Remove broken "new profile" check when determining if an addon is sideloaded r=kmag
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
Attachment #8896634 - Flags: approval-mozilla-release?
Attachment #8896634 - Flags: approval-mozilla-beta?
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
Flags: needinfo?(ryanvm)
Attachment #8896634 - Flags: approval-mozilla-release?
Attachment #8896634 - Flags: approval-mozilla-release+
Attachment #8896634 - Flags: approval-mozilla-beta?
Attachment #8896634 - Flags: approval-mozilla-beta+
RyanVM, if you could land it, that would be awesome :)
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
Flags: needinfo?(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.
Flags: needinfo?(aswan)
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.
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.
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/946e48295eb0
Remove broken "new profile" check when determining if an addon is sideloaded r=kmag
https://hg.mozilla.org/mozilla-central/rev/946e48295eb0
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Thanks to everyone for jumping on this and getting a fix in over the weekend. Awesome.
Flags: needinfo?(ryanvm)
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.
Added to the release notes with:
Fix an issue with new installation notification for sideload add-ons
Attached image 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.
You need to log in before you can comment on or make changes to this bug.