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

RESOLVED FIXED in Firefox 64

Status

()

defect
RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: aryx, Assigned: spiro, Mentored)

Tracking

Trunk
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 3 obsolete attachments)

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
Whiteboard: [good first bug]
Implementing bug 1245571 would likely make this a wontfix as we'd remove the discovery pane specific functionality.
Mentor: dtownsend
Mentor: dtownsend → aswan
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.
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
Assignee

Comment 4

10 months ago
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 :)
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. :)
Assignee: nobody → sharma.divyansh.501
Assignee

Comment 6

10 months ago
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
Flags: needinfo?(aswan)

Comment 7

10 months ago
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
Flags: needinfo?(aswan)
Assignee

Comment 8

10 months ago
I have uploaded a patch. Kindly review it.

Thanks!
Assignee

Comment 9

10 months ago
Posted file Bug in addons panel. (obsolete) —
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.
Flags: needinfo?(aswan)

Updated

10 months ago
Attachment #9005430 - Attachment description: Add test file. → Bug in addons panel.

Updated

10 months ago
Flags: needinfo?(aswan)
Assignee

Comment 10

10 months ago
Posted file Bug :1128458 [fixed] (obsolete) —
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 on attachment 9006751 [details]
Bug :1128458 [fixed]

Andrew Swan [:aswan] has approved the revision.
Attachment #9006751 - Flags: review+
Assignee

Updated

9 months ago
Keywords: compat
Assignee

Updated

9 months ago
Keywords: compatcheckin-needed

Updated

9 months ago
Attachment #9005430 - Attachment is obsolete: true

Comment 12

9 months ago
Pushed by apavel@mozilla.com:
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
Keywords: checkin-needed
Lando returned:

Failed to Land
With Diff 13204 on Sat, September 8, 2018, 4:53 AM GMT+3, by apavel@mozilla.com.
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 <sharma.divyansh.501@iitg.ernet.in> 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
***a= instead of r= due to mistype.
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 aushelper@mozilla.org
[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 firefox@getpocket.com
Flags: needinfo?(sharma.divyansh.501)
Assignee

Comment 16

9 months ago
Andrew

What is the error and how to resolve it?
Flags: needinfo?(sharma.divyansh.501) → needinfo?(aswan)
Assignee

Comment 17

9 months ago
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.
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?
Flags: needinfo?(aswan)
Assignee

Comment 19

9 months ago
Yes sir. That would be really helpful
Flags: needinfo?(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)
Flags: needinfo?(aswan)
Assignee

Comment 21

9 months ago
Posted file Bug 1128458 in addons panel. (obsolete) —
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.
Assignee

Comment 22

9 months ago
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.
Flags: needinfo?(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.
Flags: needinfo?(aswan)
Assignee

Comment 24

9 months ago
(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
Flags: needinfo?(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)
Flags: needinfo?(aswan)
Assignee

Comment 26

9 months ago
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
Flags: needinfo?(aswan)
Assignee

Comment 27

9 months ago
Oh! Sorry. My source code got replaced working code, so test cases were getting passed.
Flags: needinfo?(aswan)
Attachment #9013164 - Attachment description: Summary: Bug 1128458 - changed test files for not testing for plugins in addons. → Bug 1128458 in addons panel.
Attachment #9013164 - Attachment description: Bug 1128458 in addons panel. → Bug 1128458 - Make discovery pane only send details about extensions and themes
Assignee

Comment 29

9 months ago
Aswan

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

Thanks!
Assignee

Updated

9 months ago
Keywords: checkin-needed
QA Contact: ddurst
Attachment #9008017 - Attachment is obsolete: true
Attachment #9006751 - Attachment is obsolete: true

Comment 30

9 months ago
Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/60713d0fd643
Make discovery pane only send details about extensions and themes r=aswan
Keywords: checkin-needed
QA Contact: ddurst

Comment 31

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/60713d0fd643
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.