1128458 - Discovery pane: Only send information about addons of add-on types supported by addons.mozilla.org (e.g. not Userscripts, Styles)

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

Tested with Firefox 35.0.1 on Windows 8.1, but according to the code that also applies to Nightly

The discovery pane should only send information about addons of add-on types supported by addons.mozilla.org  to that server, e.g. not Userscripts (for Greasemonkey and Scriptih), Styles (for Stylish) etc.

The code simply fetches all addons of any type: http://mxr.mozilla.org/mozilla-aurora/source/toolkit/mozapps/extensions/content/extensions.js#2032

Comment 1

[Dave Townsend [:mossop]]

Implementing bug 1245571 would likely make this a wontfix as we'd remove the discovery pane specific functionality.

Comment 2

[Caitlin Neiman [:caitmuenster]]

If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started. 

If you need help getting started after you have set up your development environment, please needinfo :aswan.

Comment 3

[Mark Striemer [:mstriemer]]

Most of the data that is passed to the discovery pane isn't actually needed, but we can still limit it to what the discovery pane will actually look at. The discovery pane tries to detect if the user has any add-ons or themes installed when it loads, but we currently send all add-ons including language packs, dictionaries and plugins.

The add-ons that get included in the data should be retrieved from `AddonManager.getAddonsByType(["extension", "theme"])` instead of `AddonManager.getAllAddons()` as it is currently implemented [1].

[1] https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/toolkit/mozapps/extensions/content/extensions.js#1992

Comment 4

[Divyansh Sharma [:spiro]]

If this bug is not assigned to anyone, can I attempt to resolve it? Also can you guide me since I am a newbie. Thanks :)

Comment 5

[Caitlin Neiman [:caitmuenster]]

Hey Divyansh, go for it! This bug is mentored by aswan, and if you need any guidance, please check the box that says "need more information from _______" and select "mentor" from the dropdown field. 

Your first steps are to set up the development environment and find the code mentioned in comment 3. :)

Comment 6

[Divyansh Sharma [:spiro]]

Hey Andrew, I have made changes to the code as per the comment. 

But I have 2 questions:
1) After building and running firefox, how do I observe the changes?
2) What are steps to create a pull request?

Thanks

Comment 7

[Andrew Swan [:aswan]]

Hi, thanks for your interest!

(In reply to Divyansh Sharma [:spiro] from comment #6)
> 1) After building and running firefox, how do I observe the changes?

A simple test would be:
1. Launch the browser with your changes, navigate to about:addons and the discovery panel
2. Open devtools
3. Use the devtools frame selector to choose the embedded browser that has a url starting with discovery.addons.mozilla.org
4. In the devtools console, check document.location

> 2) What are steps to create a pull request?

We use phabricator for code reviews, instructions for submitting a patch are here:
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Comment 8

[Divyansh Sharma [:spiro]]

I have uploaded a patch. Kindly review it.

Thanks!

Comment 9

[Divyansh Sharma [:spiro]]

Attachment: Bug in addons panel.

Earlier, discovery pane used to send all info of addons of any type. It has been modified to provide info of only those addons which are supported by addons.mozilla.com.

Change: let aAddons = await AddonManager.getAllAddons();
 To:          let aAddons = await AddonManager.getAddonsByTypes(["extension", "theme"]);  

Bug :1128458

# NEW DIFFERENTIAL REVISION
# Describe the changes in this new revision.
#
# Included commits in branch default:
#
#         45b492adbc5a Add test file.

Comment 10

[Divyansh Sharma [:spiro]]

Attachment: Bug :1128458 [fixed]

Bug :1128458
Discovery pane now sends only information of addons supported by addons.mozilla.com prior to which it sent info of all addons.

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 9006751 [details]
Bug :1128458 [fixed]

Andrew Swan [:aswan] has approved the revision.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1507ae5193d7
Discovery pane now sends only information of addons supported by addons.mozilla.com prior to which it sent info of all addons. a=aswan

Comment 13

[Andreea Pavel [:apavel]]

Lando returned:

Failed to Land
With Diff 13204 on Sat, September 8, 2018, 4:53 AM GMT+3, by [e-mail].
Error: hg error in cmd: hg push -r tip upstream: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 1 changesets with 1 changes to 1 files remote: remote: remote: ************************** ERROR **************************** remote: Rev 10bd99d8770d needs "Bug N" or "No bug" in the commit message. remote: divyansh <[e-mail]> remote: Bug :1128458 [fixed] r=aswan remote: remote: Bug 1128458 - Discovery pane now sends only information of addons supported by addons.mozilla.com prior to which it sent info of all addons. remote: remote: Differential Revision: https://phabricator.services.mozilla.com/D5102 remote: ************************************************************* remote: remote: remote: transaction abort! remote: rollback completed remote: pretxnchangegroup.c_commitmessage hook failed abort: push failed on remote


I used arc land instead and edited the commit message: Bug 1128458 - Discovery pane now sends only information of addons supported by addons.mozilla.com prior to which it sent info of all addons. a=aswan

Comment 14

[Andreea Pavel [:apavel]]

***a= instead of r= due to mistype.

Comment 15

[Andreea Pavel [:apavel]]

Backed ot for failing bc at toolkit/mozapps/extensions/test/browser/browser_discovery.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1507ae5193d7b718b991fb8861406db2c3ceace2

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198194069&repo=mozilla-inbound&lineNumber=5394

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/1d85662d6e3e56b285b68d41f596c7dab23f37e6


[task 2018-09-08T02:35:46.186Z] 02:35:46     INFO - TEST-START | toolkit/mozapps/extensions/test/browser/browser_discovery.js
[task 2018-09-08T02:35:46.203Z] 02:35:46     INFO - GECKO(8042) | 1536374146194	addons.manager	DEBUG	Starting provider: <unnamed-provider>
[task 2018-09-08T02:35:46.207Z] 02:35:46     INFO - GECKO(8042) | 1536374146194	addons.manager	DEBUG	Registering shutdown blocker for <unnamed-provider>
[task 2018-09-08T02:35:46.212Z] 02:35:46     INFO - GECKO(8042) | 1536374146195	addons.manager	DEBUG	Provider finished startup: <unnamed-provider>
[task 2018-09-08T02:35:46.455Z] 02:35:46     INFO - GECKO(8042) | 1536374146444	addons.repository	DEBUG	No addons.json found.
[task 2018-09-08T02:35:46.574Z] 02:35:46     INFO - TEST-INFO | started process screentopng
[task 2018-09-08T02:35:47.060Z] 02:35:47     INFO - TEST-INFO | screentopng: exit 0
[task 2018-09-08T02:35:47.062Z] 02:35:47     INFO - Buffered messages logged at 02:35:46
[task 2018-09-08T02:35:47.062Z] 02:35:47     INFO - Registering mock add-on provider
[task 2018-09-08T02:35:47.068Z] 02:35:47     INFO - Running test 1
[task 2018-09-08T02:35:47.068Z] 02:35:47     INFO - Console message: 1536374146194	addons.manager	DEBUG	Starting provider: <unnamed-provider>
[task 2018-09-08T02:35:47.068Z] 02:35:47     INFO - Console message: 1536374146194	addons.manager	DEBUG	Registering shutdown blocker for <unnamed-provider>
[task 2018-09-08T02:35:47.068Z] 02:35:47     INFO - Console message: 1536374146195	addons.manager	DEBUG	Provider finished startup: <unnamed-provider>
[task 2018-09-08T02:35:47.068Z] 02:35:47     INFO - Loading manager window in tab
[task 2018-09-08T02:35:47.068Z] 02:35:47     INFO - Console message: [JavaScript Error: "this.mm.content is null, can't access property "devicePixelRatio" of it" {file: "resource:///modules/FaviconLoader.jsm" line: 443}]
[task 2018-09-08T02:35:47.068Z] 02:35:47     INFO - loadIcons@resource:///modules/FaviconLoader.jsm:443:9
[task 2018-09-08T02:35:47.068Z] 02:35:47     INFO - FaviconLoader/this.iconTask<@resource:///modules/FaviconLoader.jsm:439:44
[task 2018-09-08T02:35:47.068Z] 02:35:47     INFO - idleDispatch/</<@resource://gre/modules/PromiseUtils.jsm:40:21
[task 2018-09-08T02:35:47.068Z] 02:35:47     INFO - 
[task 2018-09-08T02:35:47.068Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Should have an add-ons manager window - 
[task 2018-09-08T02:35:47.072Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Should be displaying the correct UI - 
[task 2018-09-08T02:35:47.073Z] 02:35:47     INFO - must wait for load
[task 2018-09-08T02:35:47.075Z] 02:35:47     INFO - Console message: 1536374146444	addons.repository	DEBUG	No addons.json found.
[task 2018-09-08T02:35:47.076Z] 02:35:47     INFO - window has focus, waiting for manager load
[task 2018-09-08T02:35:47.077Z] 02:35:47     INFO - Manager waiting for view load
[task 2018-09-08T02:35:47.078Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Should not get category when manager window is not loaded - 
[task 2018-09-08T02:35:47.079Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Should not open category when manager window is not loaded - 
[task 2018-09-08T02:35:47.081Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Should not check visible state when manager window is not loaded - 
[task 2018-09-08T02:35:47.082Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Category should be visible if attempting to open it - 
[task 2018-09-08T02:35:47.084Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Should have loaded the right url - 
[task 2018-09-08T02:35:47.085Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | There should be a hash - 
[task 2018-09-08T02:35:47.086Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Hash should be a JS object - 
[task 2018-09-08T02:35:47.088Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Test add-on 1 should be listed - 
[task 2018-09-08T02:35:47.090Z] 02:35:47     INFO - Buffered messages finished
[task 2018-09-08T02:35:47.091Z] 02:35:47     INFO - TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Test add-on 2 should be listed - 
[task 2018-09-08T02:35:47.092Z] 02:35:47     INFO - Stack trace:
[task 2018-09-08T02:35:47.093Z] 02:35:47     INFO - chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_discovery.js:testHash:120
[task 2018-09-08T02:35:47.094Z] 02:35:47     INFO - chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_discovery.js:null:198
[task 2018-09-08T02:35:47.095Z] 02:35:47     INFO - chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/head.js:log_exceptions:175
[task 2018-09-08T02:35:47.096Z] 02:35:47     INFO - chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/head.js:run_next_test/<:216
[task 2018-09-08T02:35:47.098Z] 02:35:47     INFO - chrome://mochikit/content/browser-test.js:run:1333
[task 2018-09-08T02:35:47.100Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Test add-on 3 should be listed - 
[task 2018-09-08T02:35:47.101Z] 02:35:47     INFO - Testing data for add-on [e-mail]
[task 2018-09-08T02:35:47.102Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Name should be correct - 
[task 2018-09-08T02:35:47.103Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Version should be correct - 
[task 2018-09-08T02:35:47.104Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | Type should be correct - 
[task 2018-09-08T02:35:47.105Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | userDisabled should be correct - 
[task 2018-09-08T02:35:47.107Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | blocklisted should be correct - 
[task 2018-09-08T02:35:47.109Z] 02:35:47     INFO - TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_discovery.js | isCompatible should be correct - 
[task 2018-09-08T02:35:47.111Z] 02:35:47     INFO - Testing data for add-on [e-mail]

Comment 16

[Divyansh Sharma [:spiro]]

Andrew

What is the error and how to resolve it?

Comment 17

[Divyansh Sharma [:spiro]]

Andrew

Bug test case is failing for plugins. As in parameters we passsed only "extension" and "theme", do we also need "plugin" information because it is also included in add-ons. Kindly give your opinion.

Comment 18

[Andrew Swan [:aswan]]

The test should be updated to no longer expect to be passed details about plugins.
Do you need help modifying the test or running it locally before resubmitting the patch?

Comment 19

[Divyansh Sharma [:spiro]]

Yes sir. That would be really helpful

Comment 20

[Andrew Swan [:aswan]]

There's an MDN page here:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest

But briefly, in your local build, you should be able to just run
mach mochitest toolkit/mozapps/extension/test/browser/browser_discovery.js
to run the specific test that failed.  It would be a good idea to run all the tests in that directory to make sure nothing else is failing:
mach mochitest toolkit/mozapps/extension/test/browser

As for the actual change, since the discovery panel no longer cares about plugins, please just remove the code in the test that references plugins (ie, addon2 throughout the file)

Comment 21

[Divyansh Sharma [:spiro]]

Attachment: Bug 1128458 in addons panel.

Bug 1128458 - Discovery pane send info of only those addons which are supported by addons.mozilla.com prior to which it used to send all info of addons of any type.

Comment 22

[Divyansh Sharma [:spiro]]

Andrew

I have submitted the patch. But it fails some tests while being pushed into tree. I have already removed add occurrences of addon2 throughout the file, still it is not being resolved. 

Kindly guide me.

Comment 23

[Andrew Swan [:aswan]]

(In reply to Divyansh Sharma [:spiro] from comment #22)
> I have submitted the patch. But it fails some tests while being pushed into
> tree. I have already removed add occurrences of addon2 throughout the file,
> still it is not being resolved. 

Please tell me what you changed, what's failing, what you've tried to do to fix it, and where you're stuck.

Comment 24

[Divyansh Sharma [:spiro]]

(In reply to Andrew Swan [:aswan] from comment #23)
> (In reply to Divyansh Sharma [:spiro] from comment #22)
> > I have submitted the patch. But it fails some tests while being pushed into
> > tree. I have already removed add occurrences of addon2 throughout the file,
> > still it is not being resolved. 
> 
> Please tell me what you changed, what's failing, what you've tried to do to
> fix it, and where you're stuck.

Changes:
   ---> I changed AddonManager.getAllAddons() to AddonManager.getAddonsByTypes(["extension", "theme"]) in /toolkit/mozapps/extensions/content/extensions.js
   ---> Then I removed all relevant lines of addon2 in /toolkit/mozapps/extensions/test/browser/browser_discovery.js

When I tried to push it into tree following errors occured. I cannot understand or resolve them. https://treeherder.mozilla.org/#/jobs?repo=try&revision=766b49df9f3f120c8f13d2040f4e2607f9be38e0

Comment 25

[Andrew Swan [:aswan]]

So, this code is still comparing the hash in the discovery page url to the list of all installed extensions:
https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/toolkit/mozapps/extensions/test/browser/browser_discovery.js#130

It needs to be adjusted to expect what is actually intended to be sent (ie, just extensions and themes)

Comment 26

[Divyansh Sharma [:spiro]]

Aswan

My local tests summary is:

Overall Summary
===============

mochitest-browser
~~~~~~~~~~~~~~~~~
Ran 4528 checks (107 tests, 4421 subtests)
Expected results: 4520
Skipped: 8 tests
OK

mochitest-plain
~~~~~~~~~~~~~~~
Ran 4 checks (2 tests, 2 subtests)
Expected results: 3
Skipped: 1 tests
OK

I am not seeing any problem here. Help me out!
Thanks

Comment 27

[Divyansh Sharma [:spiro]]

Oh! Sorry. My source code got replaced working code, so test cases were getting passed.

Comment 28

[Divyansh Sharma [:spiro]]

Attachment: Bug 1128458 - Make discovery pane only send details about extensions and themes

Comment 29

[Divyansh Sharma [:spiro]]

Aswan

Yes, I have tested all the cases locally and they are working fine.

Thanks!

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60713d0fd643
Make discovery pane only send details about extensions and themes r=aswan

Comment 31

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/60713d0fd643