Closed Bug 1503047 Opened 6 years ago Closed 6 years ago

Snippets are not loaded due to missing element

Categories

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

63 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox63 blocking verified
firefox64 --- verified

People

(Reporter: k88hudson, Assigned: k88hudson)

Details

Attachments

(1 file)

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.
Component: Activity Stream → Activity Streams: Newtab
Product: Firefox for Android → Firefox
Target Milestone: --- → Firefox 63
Version: Firefox 63 → 63 Branch
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.
: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.
Flags: needinfo?(rkothari)
Flags: needinfo?(pascalc)
Michele: Can you comment on the level of severity that Kate refers to above? Thank you.
Flags: needinfo?(mwarther)
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.
Flags: needinfo?(mwarther)
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.
Does it also impact Firefox for Android or is the impact Desktop only?
Flags: needinfo?(pascalc) → needinfo?(khudson)
It's Desktop only.
Flags: needinfo?(khudson)
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 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
Attachment #9021030 - Flags: approval-mozilla-release?
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.
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 on attachment 9021030 [details]
Bug 1503047 - Fix snippets not loading by ensuring snippets element exists

Uplift approved for 63.0.1
Flags: needinfo?(rkothari)
Attachment #9021030 - Flags: approval-mozilla-release? → approval-mozilla-release+
Thanks for the quick patch and uplift approval to 63.0.1.
https://hg.mozilla.org/releases/mozilla-release/rev/57ecd86334a5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(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?
Flags: needinfo?(tspurway)
Flags: needinfo?(khudson)
(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.
(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.
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.
Flags: needinfo?(tspurway)
Flags: qe-verify+
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!"
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!"
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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.
Flags: needinfo?(khudson)
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.
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.