Closed Bug 1494275 Opened 2 years ago Closed 2 years ago

CFR can wrongly recommend outdated versions of the addons

Categories

(Firefox :: Messaging System, defect, P1)

Unspecified
All
defect

Tracking

()

VERIFIED FIXED
Firefox 64
Iteration:
64.3 - Oct 12
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 + verified
firefox64 --- verified

People

(Reporter: ppop, Assigned: ursula)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

Attached image 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:
Assignee: nobody → usarracini
Priority: -- → P1
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.
Blocks: 1491379
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!
Flags: needinfo?(khudson)
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)
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Comment on attachment 9012311 [details]
Bug 1494275 - Support dynamic install urls for CFR

Ursula Sarracini (:ursula) has approved the revision.
Attachment #9012311 - Flags: review+
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
Iteration: --- → 64.2 (Sep 28)
Pushed by khudson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d0595bcbe068
Support dynamic install urls for CFR r=ursula
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 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
Attachment #9012311 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(khudson)
Attachment #9012311 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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!
Flags: needinfo?(usarracini)
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.
Flags: needinfo?(usarracini)
Approval Request Comment
See previous request - this patch fixes the merge conflict
Attachment #9012593 - Flags: review?(khudson)
Attachment #9012593 - Flags: approval-mozilla-beta?
Attachment #9012593 - Flags: review?(khudson) → review+
Comment on attachment 9012593 [details] [diff] [review]
Bug_1494275_beta_uplift_patch.diff

Updated patch approved for uplift to beta.
Attachment #9012593 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Flags: needinfo?(usarracini)
Approval Request Comment
Sorry this is the actual merge conflict fix. Last one!
Attachment #9012593 - Attachment is obsolete: true
Flags: needinfo?(usarracini)
Attachment #9012595 - Flags: review?(khudson)
Attachment #9012595 - Flags: approval-mozilla-beta?
Attachment #9012595 - Flags: review?(khudson) → review+
Attachment #9012614 - Attachment is obsolete: true
Attachment #9012595 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
Blocks: 1494427
https://hg.mozilla.org/mozilla-central/rev/410ea4b30b37
Iteration: 64.2 (Sep 28) → 64.3 (Oct 12)
Target Milestone: --- → Firefox 64
Depends on: 1526387
Component: Activity Streams: Newtab → Messaging System
You need to log in before you can comment on or make changes to this bug.