1494275 - CFR can wrongly recommend outdated versions of the addons

Status: VERIFIED FIXED

(Firefox :: Messaging System, defect, P1)

Description

[Patriciu Pop [:ppop], Ecosystem QA]

Attachment: addon.gif

[Affected versions]:
- Firefox Beta, Version 63.0b9, Build ID 20180924202103

[Affected Platforms]:
- All Windows
- All Linux
- All Mac

[Prerequisites]:
- Have the "browser.newtabpage.activity-stream.asrouter.messageProviders" pref set to "[{"id":"cfr", "cohort": "one_per_day", "frequency": {"custom": [{"period": "daily", "cap": 1}]}, "type":"local","localProvider":"CFRMessageProvider","enabled":true}]"
- Frecency for https://www.reddit.com is at least 10000.

[Steps to reproduce]:
1. Open the browser with the profile from prerequisites.
2. Open a new tab and navigate to https://www.reddit.com
3. Click the "Recommendation" button and click the "Add Now" button from the doorhanger.
4. Observe the doorhanger.

[Expected result]:
- The Add-on is downloaded and the option to enable it is displayed.

[Actual result]:
- The Add-on is not downloaded and the following error message is displayed: "The add-on could not be downloaded because of a connection failure."

[Additional Notes]:
- In the Browser Console the following error is displayed: 1537952045667 addons.xpi WARN Download of https://addons.mozilla.org/firefox/downloads/file/991623/reddit_enhancement_suite-5.12.5-an+fx.xpi failed: 404 Not Found
- The latest AMO version of the "Reddit Enhancement Suite" is 5.12.6 while the addon attempts to install version 5.12.5
- I have attached a screen recording of the issue:

Comment 1

[Kate Hudson :k88hudson]

It sounds like in order to resolve this issue we will need change the strategy here to hit an external AMO api rather than hard-coding URLs, since the hard-coded URLs fail to install anything and we can't control the update cycle of a particular add-on.

Because this results in a failure to install the ad-don, this is a blocking bug for the 63 study.

Comment 2

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4451

Comment 3

[Pascal Chevrel:pascalc]

Hi Kate, please land the patch today on mozilla-central and attach a patch for uplift if you want it in 63 beta 10 that will ship this Friday, thanks!

Comment 4

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/05fb100d324931609cc951fce101fe8535cd5e10
Fix Bug 1494275 - CFR can wrongly recommend outdated versions of the addons (#4451)

Comment 5

[Kate Hudson :k88hudson]

Attachment: Bug 1494275 - Support dynamic install urls for CFR

Comment 6

[Ursula Sarracini (:ursula)]

Comment on attachment 9012311 [details]
Bug 1494275 - Support dynamic install urls for CFR

Ursula Sarracini (:ursula) has approved the revision.

Comment 7

[Ursula Sarracini (:ursula)]

Yeah this was sort of anticipated since we're hard coding a url with the version in it. We didn't know it was going to break the entire experience, we thought it would just install the old version of the add-on, which wouldn't have been so bad. I will look into finding a better solution for this, and we can get this uplifted.

Worst case, we have to query the add-ons API to get the url to the latest xpi when we show the recommendation. This would involve making a network request when we show the recommendation. Not ideal

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0595bcbe068
Support dynamic install urls for CFR r=ursula

Comment 9

[:wezhou]

Does anyone know how often the browser queries the AMO API endpoint?

From AMO's perspective, we'd like to know the estimated amount of requests (and the incoming traffic pattern, if possible) that will be introduced to our service because of this change, so that we can adjust our capacity accordingly.

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/d0595bcbe068

Comment 11

[Kate Hudson :k88hudson]

Comment on attachment 9012311 [details]
Bug 1494275 - Support dynamic install urls for CFR

Approval Request Comment
[Feature/Bug causing the regression]: URLS for recommended add-ons no longer work if the add-on is updated  
[User impact if declined]: users will receive recommendations that fail if accepted
[Is this code covered by automated tests?]: yes 
[Has the fix been verified in Nightly?]: not yet, patch has landed on Mozilla central
[Needs manual test from QE? If yes, steps to reproduce]: see steps to reproduce in the first comment of this bug
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]:no 
[Why is the change risky/not risky?]: affects shield study only
[String changes made/needed]: none

Comment 12

[Pascal Chevrel:pascalc]

Comment on attachment 9012311 [details]
Bug 1494275 - Support dynamic install urls for CFR

Blocker to CFR shield study launching on Oct.1st, approved for uplift to  63 beta 10. Thanks.

Comment 13

[Eliza Balazs [:ebalazs_]]

Hi, I get this conflict when trying to graft this revision:

merging browser/components/newtab/content-src/asrouter/templates/CFR/templates/ExtensionDoorhanger.schema.json       
merging browser/components/newtab/lib/CFRMessageProvider.jsm                                                         
merging browser/components/newtab/lib/CFRPageActions.jsm                                                             
merging browser/components/newtab/test/unit/asrouter/templates/ExtensionDoorhanger.test.jsx                          
warning: conflicts while merging browser/components/newtab/lib/CFRMessageProvider.jsm! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/newtab/lib/CFRPageActions.jsm! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/newtab/test/unit/asrouter/templates/ExtensionDoorhanger.test.jsx!

ursula: Can you please have a look at this? Thank you!

Comment 14

[Kate Hudson :k88hudson]

We have a patch with merge conflicts fixed, we are just waiting on phabricator to publish. If it doesn't in the next couple of minutes we will attach as a text diff.

Comment 15

[Ursula Sarracini (:ursula)]

Attachment: Bug_1494275_beta_uplift_patch.diff

Approval Request Comment
See previous request - this patch fixes the merge conflict

Comment 16

[Pascal Chevrel:pascalc]

Comment on attachment 9012593 [details] [diff] [review]
Bug_1494275_beta_uplift_patch.diff

Updated patch approved for uplift to beta.

Comment 17

[Eliza Balazs [:ebalazs_]]

Hi, I get the following conflict when trying to import this:

applying Bug_1494275.diff
file browser/components/newtab/test/unit/asrouter/CFRPageActions.test.js already exists
1 out of 1 hunks FAILED -- saving rejects to file browser/components/newtab/test/unit/asrouter/CFRPageActions.test.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory

Comment 18

[Ursula Sarracini (:ursula)]

Attachment: Bug_1494275_beta_uplift_patch.diff

Approval Request Comment
Sorry this is the actual merge conflict fix. Last one!

Comment 19

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f68f9da6cb71

Comment 20

[Ursula Sarracini (:ursula)]

Attachment: Bug 1494275 - Support dynamic install urls for CFR

Comment 21

[Patriciu Pop [:ppop], Ecosystem QA]

I have verified that this issue is no longer reproducible using Firefox Beta (Version 63.0b10, Build ID 20180927143327) and Firefox Nightly (Version 64.0a1, Build ID 20180927220034) on Windows 10 x64, Mac 10.13 and Arch Linux.

Comment 22

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/410ea4b30b37