1479458 - topstories pref changes from shield studies not properly displaying

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Scott [:thecount] Downe]

STRs

1. Set the pref "browser.newtabpage.activity-stream.feeds.section.topstories.options" to default.

2. Open about:home

3. Take note of the third rec.

4. Update "browser.newtabpage.activity-stream.feeds.section.topstories.options" pref to:

{"api_key_pref":"extensions.pocket.oAuthConsumerKey","hidden":false,"provider_icon":"pocket","provider_name":"Pocket","read_more_endpoint":"https://getpocket.com/explore/trending?src=fx_new_tab","stories_endpoint":"https://getpocket.cdn.mozilla.net/v3/firefox/global-recs?version=3&consumer_key=$apiKey&locale_lang=en-US&feed_variant=shield_cache_bug_nightly_spocs_test","stories_referrer":"https://getpocket.com/recommendations","topics_endpoint":"https://getpocket.cdn.mozilla.net/v3/firefox/trending-topics?version=2&consumer_key=$apiKey&locale_lang=en-US","show_spocs":true,"personalized":true}

5. Open a new tab.

Expected: Should update the third rec.
Actual: still showing the old one.

I think there are two issues here.

1. This line here is async https://github.com/mozilla/activity-stream/blob/master/lib/SectionsManager.jsm#L438 which means that event on the next line may fire before the spocs are fetched, and it'll stay this way until the pref is changed again, making it always out of sync by 1 update.

2. If the above is fixed, I'm noticing opening a new tab isn't enough, but you have to refresh it too.

This is mostly because of how shield updates prefs "at some point" it's fast, but happens after the browser runs, and how activity stream reads that pref.

Thoughts?

Comment 1

[Scott [:thecount] Downe]

The directions in step 4 are now out of date. Better to use something like this:

{"api_key_pref":"extensions.pocket.oAuthConsumerKey","hidden":false,"provider_icon":"pocket","provider_name":"Pocket","read_more_endpoint":"https://getpocket.com/explore/trending?src=fx_new_tab","stories_endpoint":"file:///home/scott/spoc.json","stories_referrer":"https://getpocket.com/recommendations","topics_endpoint":"https://getpocket.cdn.mozilla.net/v3/firefox/trending-topics?version=2&consumer_key=$apiKey&locale_lang=en-US","show_spocs":true,"personalized":true}

Just update the line here: file:///home/scott/spoc.json to a path to a json file, I've been using the file attached. It should always show 1 spoc in the third place from theringer.com titled "The Kawhi-DeRozan Trade Is a Win for Both the Spurs and Raptors"

Comment 2

[Scott [:thecount] Downe]

Attachment: spoc.json

Comment 3

[[github robot]]

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

https://github.com/mozilla/activity-stream/commit/be0ccddd3e6f7d7adffa55b0440f8a1104286ca5
Bug 1479458 - Spocs updating when pref changes while browser is running example, a shield study.

https://github.com/mozilla/activity-stream/commit/0a15443fca241607206dd4ba144242928f25275d
Merge pull request #4276 from ScottDowne/t-1479458

Fix Bug 1479458 - Spocs updating when pref changes from a shield study

Comment 4

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/972b3c48c6e9

Comment 5

[Ed Lee :Mardak]

Mossop mentioned on irc:

Just updated to the current nightly and activity stream seems to be busted. The recommendations from pocket are all empty
Oh, they came back after a refresh


Not sure if that was related to the changes here. At first I thought it was just completely broken from negative array index bug 1482484, but a simple refresh means the current tab didn't get the data -- not broadcasted..?

Comment 6

[Scott [:thecount] Downe]

I take it making this somehow reproducible is going to be hard, to simulate whatever it is doing when it does an update?

What's interesting is topics are also blank, so it feels like the bug is in the onInit function, or something preventing it from firing at all.

Comment 7

[Scott [:thecount] Downe]

I managed to reproduce it!

1. If you checkout aab3771a50215d97ed4f34c7056f46a0f943a822 (which is just before this patch landed) and open the browser enough to generate some recs.

2. If you then checkout this patch be0ccddd3e6f7d7adffa55b0440f8a1104286ca5 and run it, you get no recs.

It has something to do with data generated (or cached) from the old patch, then when the new one tries to run it's updated code, it's choking on the old data.

Comment 8

[Scott [:thecount] Downe]

So I think I know what's happening.

These lines https://github.com/mozilla/activity-stream/blob/master/lib/TopStoriesFeed.jsm#L61-L65
and this line https://github.com/mozilla/activity-stream/blob/master/lib/SectionsManager.jsm#L371

Is essentially incorrectly saying if we have cached data, only send the data to preloaded tabs.

However, in this case the tab we just opened isn't a preloaded tab.

In the old code, it would actually always send the signal to broadcast from init, my code miss read that. Patch incoming.

Comment 9

[GitHub Bugzilla PR Linker]

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

Comment 10

[Scott [:thecount] Downe]

I'll get a test added in there too before I ask for review, but for now, that's the fix I think we need.

Comment 11

[Scott [:thecount] Downe]

OK, updated with a test :)

Comment 12

[Marius Coman [:mcoman], Ecosystem QA]

I have verified that this issue is no longer reproducible on Windows 10 x64, Arch Linux and Mac 10.13.3 with the latest version of "Firefox Nightly" (63.0a1 - Build ID 20180812220618) installed. Now, after the value of the "browser.newtabpage.activity-stream.feeds.section.topstories.options" pref is changed (as suggested in comment #1), the third rec is changed.

Comment 13

[[github robot]]

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

https://github.com/mozilla/activity-stream/commit/a6796250ec7481411ecc3301544423cb9d44db24
Bug 1479458 - Fixing broadcast of updated data from cache.

https://github.com/mozilla/activity-stream/commit/2bb73a94ad7b6e670558f287f2b8255165a0bf47
Merge pull request #4324 from ScottDowne/t-1479458-b

Bug 1479458 - Fixing broadcast of updated data from cache.

Comment 14

[Ed Lee :Mardak]

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

Comment 15

[Ed Lee :Mardak]

QA note: Scott fixed a followup issue with this same bug. See comment 6 - 8

Comment 16

[Scott [:thecount] Downe]

Attachment: Bug 1479458 - Topstories pref changes from shield studies not properly displaying