1503047 - Snippets are not loaded due to missing element

Status: VERIFIED FIXED

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

Description

[Kate Hudson :k88hudson]

Due to the inner snippets element getting accidentally cleared by Activity Stream's unmounting code, snippets are no longer loaded by Firefox.

This bug was fixed by https://bugzilla.mozilla.org/show_bug.cgi?id=1492942 but remains on Firefox 63 release.

The effect of this is that snippets will never be shown on new tab for users in  Firefox 63.

Comment 1

[Kate Hudson :k88hudson]

In this case "snippets" includes all of marketing's new tab page campaigns for the duration of Firefox 63, so this is a pretty severe bug.

Comment 2

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked as a blocking issue for 63.

Comment 3

[Erin Lancaster [:elan]]

:pascalc :ritu, can you outline what the next steps are to get this fix to 63.0 users? Please ping me in Slack if you need help pulling people together. Thank you.

Comment 4

[Erin Lancaster [:elan]]

Michele: Can you comment on the level of severity that Kate refers to above? Thank you.

Comment 5

[Michele Warther]

It means that new users don't get any onboarding messages in the 9 languages we support and existing users aren't getting any announcements (Monitor, Lockbox, US Elections, Referral Programs etc.) and that's just this week. We are losing 10's of millions of impressions each day and have an average of 5-7 new campaigns launching each week, it's a very big issue for us.

Comment 6

[Kate Hudson :k88hudson]

Attachment: Bug 1503047 - Fix snippets not loading by ensuring snippets element exists

Comment 7

[Kate Hudson :k88hudson]

More detailed summary of what happened here: 

Some point during the 63 cycle, we landed some code that accidentally removed the inner element in which snippets are injected. This would have affected 63 nightly and beta and currently impacts 63 release, and prevents any snippets from being show on the new tab page.

During the 64 cycle, the bug was circumstantially fixed by an unrelated bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1492942 ), however, the fix for this bug should only require a much smaller and less risky patch.

Comment 8

[Pascal Chevrel:pascalc]

Does it also impact Firefox for Android or is the impact Desktop only?

Comment 9

[Andrei Oprea [:andreio]]

It's Desktop only.

Comment 10

[Andrei Oprea [:andreio]]

STR for the patch:
1. In a new firefox profile 
2. After dismissing the onboarding tour snippets (have no snippet showing up)
3. Open the browser console
4. Ensure `ABOUTHOME_SNIPPETS` is defined and has some elements
   * typing in `ABOUTHOME_SNIPPETS` in the console should return Array(x) where X are the number of snippets available to show
5. Paste the entire content of this gist https://gist.githubusercontent.com/piatra/e1b6c48a5bd935508eb3778638b56800/raw/36591aef7de08129641033e08ccd3ac8d682c285/bootstrapsnippets.js into the console
6. See the snippet that showed up

Comment 11

[Tim Spurway [:tspurway]]

Comment on attachment 9021030 [details]
Bug 1503047 - Fix snippets not loading by ensuring snippets element exists

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1503047

User impact if declined: Users will not see any Snippets campaigns, including US Election Snippets scheduled for November 2018

Is this code covered by automated tests?: Unknown

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 1. In a new firefox profile 
2. After dismissing the onboarding tour snippets (have no snippet showing up)
3. Open the browser console
4. Ensure `ABOUTHOME_SNIPPETS` is defined and has some elements
   * typing in `ABOUTHOME_SNIPPETS` in the console should return Array(x) where X are the number of snippets available to show
5. Paste the entire content of this gist https://gist.githubusercontent.com/piatra/e1b6c48a5bd935508eb3778638b56800/raw/36591aef7de08129641033e08ccd3ac8d682c285/bootstrapsnippets.js into the console
6. See the snippet that showed up

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The patch is fairly small and straightforward

String changes made/needed: no

Comment 12

[Andrei Oprea [:andreio]]

Alternative STR (and probably better/easier)

1. On a fresh profile after completing the onboarding tour (Skip tour button is enough)
2. Set `browser.aboutHomeSnippets.updateUrl` to `https://snippets.allizom.org/%STARTPAGE_VERSION%/%NAME%/%VERSION%/%APPBUILDID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/`
3. In the browser devtools type in `gSnippetsMap.clear()` to remove everything and fetch new snippets from this endpoint
4. A snippet should show up.

Comment 13

[Kate Hudson :k88hudson]

Note that if you do not see onboarding messages on the new tab in order to remove them, you may need to set the `browser.onboarding.notification.mute-duration-on-first-session-ms` pref to 0.

Comment 14

[Pascal Chevrel:pascalc]

Comment on attachment 9021030 [details]
Bug 1503047 - Fix snippets not loading by ensuring snippets element exists

Uplift approved for 63.0.1

Comment 15

[Chris More [:cmore]]

Thanks for the quick patch and uplift approval to 63.0.1.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/57ecd86334a5

Comment 17

[Dave Townsend [:mossop]]

(In reply to Tim Spurway [:tspurway] from comment #11)
> Is this code covered by automated tests?: Unknown

This seems like a strange answer, how can we not know?

Regardless, with a bug of this severity we really should be adding an automated test to verify that this doesn't break again. Who is on the hook for that?

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Tim Spurway [:tspurway] from comment #11)
> Feature/Bug causing the regression: Bug 1503047

I believe the real bug that caused this regression is Bug 1492942.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Ritu Kothari (:ritu) from comment #18)
> (In reply to Tim Spurway [:tspurway] from comment #11)
> > Feature/Bug causing the regression: Bug 1503047
> 
> I believe the real bug that caused this regression is Bug 1492942.

That is incorrect. Hi Kate, Tim, can you please point out which bug caused this regression? You added this bug # as the bug causing regression in comment 11, which seems inaccurate.

Comment 20

[Tim Spurway [:tspurway]]

Snippets is a difficult system to build automated tests for.  The Snippets payload is server-side, each Snippet has arbitrary JS code injected into the newtab that can make it appear or not, though there are some tests, I don't know if it's possible to write a test that can give adequate coverage to the existing (soon to be legacy) Snippets system.

This along with the other bugs (like bug #1408349) are the reasons we landed a complete replacement for the way Snippets are rendered in Firefox.  The new implementation is more testable, more secure, has better data collection and will ultimately lead to better messaging to our users.  

This patch is for 63 only, and will be superseded by the new Snippets implementation in 64.

Comment 21

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I can confirm this issue is fixed, I verified using Firefox 63.0.1 on Win 10 x64, Ubuntu 18.04 x64 and Mac OS X 10.13, following the STR from comment 10 a snippet show up: "Happy Day of the Dead from your friends at Firefox!"

Comment 22

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I verified on Firefox 64.0b5 on Win 10 x64, Ubuntu 18.04 x64 and Mac OS X 10.13, following the STR from comment 10 a snippet show up: "Happy Day of the Dead from your friends at Firefox!"

Comment 23

[Kate Hudson :k88hudson]

We do not have automated testing that covers this particular issue; for the reasons Tim mentioned, it's unfortunately difficult to test the current implementation that has been in tree for some time and we don't have great coverage.

We are planning to roll-out the new version of the system in 64, which has must better automated testing coverage, no more JS injection, and better QA tools.

I think the bug that broke this was a CFR-related uplift (Bug 1488758) but I'm still working on verifying this.

Comment 24

[Kate Hudson :k88hudson]

Another issue we ran into here is that there are a number of intermittent issues with snippets that come up related to indexedDB/downgrading, as well as expected reasons for snippets not show up that are hard to determine from bug reports (e.g. server just not returning content, onboarding tour conflicts). These were also addressed in the 64 implementation.