WNP pages in Firefox 56 are not displayed

RESOLVED FIXED in Firefox 55
(Needinfo from 2 people)

Status

()

Toolkit
Application Update
P1
major
RESOLVED FIXED
22 days ago
a day ago

People

(Reporter: mtabara, Assigned: rstrong, NeedInfo)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox55 fixed, firefox56 fixed, firefox57 fixed)

Details

(Whiteboard: [releaseduty])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

22 days ago
Discuss & setup WNP for 55.0

Comment 1

22 days ago
Hi Mihai, yes we do want to continue showing WNP in 55.0 please.  It's an useful marketing channel for getting mobile download conversions.

Comment 2

18 days ago
Julien and Mihai, 

the webpage you can point to for the WNP is: https://www.mozilla.org/en-US/firefox/55.0/whatsnew/

(locale should autodetect)
Flags: needinfo?(mtabara)
Flags: needinfo?(jcristau)
(Reporter)

Comment 3

18 days ago
For 55.0 I followed this docs[1] and manually added the WNP blob as follows:

mihaitabara@mozspace:[]~/Downloads/misc$ diff Firefox-55.0-build3.json Firefox-55.0-build3-whatsnew.json 
1a2,3
>     "actions": "showURL",
>     "openURL": "https://www.mozilla.org/%LOCALE%/firefox/55.0/whatsnew/?oldversion=%OLD_VERSION%",
54c56
<     "name": "Firefox-55.0-build3",
---
>     "name": "Firefox-55.0-build3-whatsnew",

@catlee: r?

[1]: https://wiki.mozilla.org/Release:Release_Automation_on_Mercurial:Updates_through_Shipping#Set-up_whatsnew_page

Comment 4

18 days ago
+jpetto to weigh in.
Flags: needinfo?(mtabara)
Flags: needinfo?(jon)
Flags: needinfo?(jcristau)

Comment 5

18 days ago
I am not the authority here, but this URL from comment 3 looks correct to me:

https://www.mozilla.org/%LOCALE%/firefox/55.0/whatsnew/?oldversion=%OLD_VERSION%

It is at least correct in having a placeholder for locale instead of "en-US".
Flags: needinfo?(jon)
LGTM
(Reporter)

Comment 7

18 days ago
(In reply to Chris AtLee [:catlee] [PTO until Aug 14] from comment #6)
> LGTM

Thanks. Firefox-55.0-build3-whatsnew is up in Balrog, ready to be mapped on Monday.
(Reporter)

Comment 8

15 days ago
So this ate more than I anticipated today. tl;dr - from releng side things look good. Balrog is returning the right urls and the right actions as this indicates for example[1]. It's similar to what we currently have for the 54.0.1 WNP[2].

A bunch of us from Releng along with QE we've tried debugging this and ended up with the following conclusions:
* if we install 53.0.3 and update to 54.0.1 we get the 54 WNP, then if we switch the channel to release-cdntest and update to 55.0 it gets a WNP with version "54.0/.../OLDVERSION=54.0.1
* however if we first install 54.0.1 and launch, close, switch channel, update, restart we get no WNP at all
* when updating from 54 to 55.0 the WNP page doesn't show up (release-cdntest)
* when updating from 53 directly to 55.0 on release-cdntest, WNP page doesn't show up.

Steps we did, both us from RelEng and confirmed with QE earlier in the day were:
1. download 53.0.3 --> open in fresh profile (dismiss any errant tabs) --> update --> close firefox --> set update channel to "release-cdntest" --> open firefox --> update again --> expect to see a WNP

2. and seperately: download 54.0.1 --> open in fresh profile (dismiss any errant tabs) --> close --> set update channel to "release-cdntest" --> open firefox --> update --> expect to see a WNP

From 1 you get https://www.mozilla.org/en-US/firefox/54.0.1/whatsnew/?oldversion=%OLD_VERSION% -- and from 54.0.1 you get no WNP at all. Neither cleaning profiles, sessions or any other caching-data form didn't help. As aforementioned above Balrog works as expected so this is something to do with the updater.

Setting app.update.log to True and running the startup logic showed some errors at start-up that can be seen here[3].
Out of what we debugged, sounds like there's some variables in that only in certain circumstances get overwritten[4].

Robert Strong has been able to reproduce the bug, if my understanding is right and he might file a separate bug to deal with this. Most likely there's pretty much nothing we can do for this release.




[1]: https://aus5.mozilla.org/update/3/Firefox/54.0.1/20170424145525/Darwin_x86_64-gcc3-u-i386-x86_64/my/release-cdntest/default/default/default/update.xml?force=1
[2]: https://aus5.mozilla.org/update/3/Firefox/53.0.1/20170424145525/Darwin_x86_64-gcc3-u-i386-x86_64/my/release/default/default/default/update.xml?force=1
[3]: http://i.imgur.com/FfVrNDK.png
[4]: https://dxr.mozilla.org/mozilla-release/source/browser/components/nsBrowserContentHandler.js#484
Is it an in product change which caused this regression?
If so, could mozregression help with the regression detection?
Thanks

Comment 10

15 days ago
My current issue:

- mozregression blows away the app install dir on retry.
- i need to quit the app to edit the update channel, then retry
- when i retry, my edited update channel is blown away, so it's back to updating against the `nightly` channel

Comment 11

15 days ago
Hi,

We've pushed the same WNP for the FX53 AND FX54 releases, whats the difference between how we implemented WNP then vs now?
Nightly/inbound builds would not accept release mar files, so I think that'll be hard to get going without also customizing app.update.url to point to some static file. You'd need something that provides a nightly mar file but sets the actions and openURL metadata. 

Alternatively you could manually start with 54.0b1 on release-cdntest, since that'll be OK with release mar files, and see if it also has this problem. If that does show the WNP then a manual binary search amongst 54 betas might help narrow the range.

Comment 13

15 days ago
Hey Nick and relman/releng. Just wanted to see if I could close the loop here on questions regarding WNP.

Can you confirm:

1) Will the mobile promo WNP pop for fx55 people tomorrow that upgrade to fx55?

1a) If no to above, is it too late to resolve by tomorrow?

Thanks,
Chris
Flags: needinfo?(nthomas)
My read of this bug is users updating to 55.0 will NOT get the WNP we want to show them. 

Comment #8 indicates this is a problem with 55.0 (since it affects updating from 53 and 54, and there's the 'Unable to find update' error in the console after update, see http://i.imgur.com/FfVrNDK.png). I don't know what we would need to fix the problem, and it seems unlikely we could turn around that before regular shipping time tomorrow. Passing the ni to rstrong though.
Flags: needinfo?(nthomas) → needinfo?(robert.strong.bugs)
Might be a regression from bug 1301517 ?
Depends on: 1388204
nthomas' assessment is correct and it is likely a regression from bug 1301517.

Since the pendulum has swung back to showing the what's new page pretty much all of the time I don't think it makes sense to use the system that was built to seldom if ever show the what's new page so I filed Bug 1388204 along with the reasons why I think this is a better path forward.
Flags: needinfo?(robert.strong.bugs)
(Reporter)

Updated

15 days ago
Blocks: 1388306
We would still like to be able to show this page for 55 (or for any dot releases we might have for 55). Is it possible to fix this for 55.0.1?
(Reporter)

Comment 18

15 days ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #17)
> We would still like to be able to show this page for 55 (or for any dot
> releases we might have for 55). Is it possible to fix this for 55.0.1?

This might be a good question for :rstrong in bug 1388204 I suppose.
From what rstrong was saying earlier, it isn't possible to fix this in a 55 dot release without backing out the work in bug 1301517, which supports photon. Maybe that isn't impossible to rewrite but my guess is that it would not be trivial and would need extra rounds of testing (and we aren't sure what backing it out, or rewriting it, would break).  So I see rstrong's point that changing the way we approach the WNP entirely might be more feasible.

Comment 20

14 days ago
@rstrong and @lizzard So whats the plan and timeline to get the WNP up for this release?  We've committed to execs to show the WNP promoting mobile during every release cycle to users who update
Flags: needinfo?(robert.strong.bugs)
Flags: needinfo?(lhenry)
I can come up with a new patch for Nightly 57 and I am fine with that patch being backed out.
Flags: needinfo?(robert.strong.bugs)
Sylvestre, over to you if you might want to try to get a patch from rstrong to include in 55.0.1.
Flags: needinfo?(lhenry) → needinfo?(sledru)
All it should take is
hg backout 7ac40f983b13

It backed out cleanly but I'm compiling release just to verify tests pass as well.
Tests are fine. If you would prefer that I perform the backout on mozilla-release I can do so Wednesday. I'll address the mozilla-beta in bug 1388204.

Comment 25

14 days ago
uplift
OK, I did the backout:
https://hg.mozilla.org/releases/mozilla-release/rev/d8a11d7e960e24252a58c3d705fc525322ff58cc
status-firefox55: --- → fixed
Component: Releases → Application Update
Flags: needinfo?(sledru)
Product: Release Engineering → Toolkit
QA Contact: catlee
We will need QE to verify that
Flags: qe-verify+
Summary: Setup WNP for Firefox 55.0 release → WNP pages in Firefox 55 are not displayed
Created attachment 8895261 [details] [diff] [review]
backout-wnp.diff

Uplift request for posterity

Approval Request Comment
[Feature/Bug causing the regression]: bug 1386224
[User impact if declined]: No "what's new" page
[Is this code covered by automated tests?]: Yes: https://codecov.io/gh/marco-c/gecko-dev/src/master/toolkit/mozapps/update/nsUpdateService.js#L2609
but this wasn't enough to detect this issues...
[Has the fix been verified in Nightly?]: This is a backout, we are back in the previous state
[Needs manual test from QE? If yes, steps to reproduce]:  Yes
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: I don't think it is as we are back in the previous state we were in
[Why is the change risky/not risky?]:
[String changes made/needed]: No
Attachment #8895261 - Flags: approval-mozilla-release?
Assignee: nobody → robert.strong.bugs
What will happen for 56 - do we need to backout there as well?
Flags: needinfo?(robert.strong.bugs)
 I'll address mozilla-beta in bug 1388204.
Flags: needinfo?(robert.strong.bugs)
Comment on attachment 8895261 [details] [diff] [review]
backout-wnp.diff

I landed it in m-r
Attachment #8895261 - Flags: approval-mozilla-release? → approval-mozilla-release+
Created attachment 8895729 [details]
Firefox-55.0.1-build2-whatsnew
Attachment #8895729 - Flags: review?(mtabara)
(Reporter)

Comment 32

13 days ago
Comment on attachment 8895729 [details]
Firefox-55.0.1-build2-whatsnew

Looks good overall but:

1. openURL needs to point to 55.0 WNP as per RelMan -> "https://www.mozilla.org/%LOCALE%/firefox/55.0/whatsnew/?oldversion=%OLD_VERSION%"

2. detailsUrl needs to point to 55.0 release notes as per RelMan -> "https://www.mozilla.org/%LOCALE%/firefox/55.0/releasenotes/"

3. The file to be uploaded to Balrog needs to match inner naming + extension (in this case json): "Firefox-55.0.1-build2-whatsnew.json"
Attachment #8895729 - Flags: review?(mtabara) → review-
Created attachment 8895841 [details]
Firefox-55.0.1-build2-whatsnew.json

fixed all 3 things you reported
Attachment #8895729 - Attachment is obsolete: true
Attachment #8895841 - Flags: review?(mtabara)
(Reporter)

Comment 34

13 days ago
Comment on attachment 8895841 [details]
Firefox-55.0.1-build2-whatsnew.json

Looks good!
Attachment #8895841 - Flags: review?(mtabara) → review+
The backout in 55.0.1 successfully fixed the bug. We're seeing WNP pages after updates to 55.0.1.
Status: NEW → RESOLVED
Last Resolved: 12 days ago
Resolution: --- → FIXED

Comment 36

12 days ago
Thank you all for the hustle here!
Seems very likely this still affects 56 and 57 still. I know the bug title refers specifically to 55, which is on release, but since we still need to land a fix for future releases, we may want to keep this bug open.  Or, maybe it makes more sense to close this and mark the dependent bug as blocking 56.
Status: RESOLVED → REOPENED
status-firefox56: --- → ?
status-firefox57: --- → ?
Resolution: FIXED → ---
Duplicate of this bug: 1388204
I was going to fix this in bug 1386224 but this bug is fine.
Attachment #8895261 - Attachment is obsolete: true
Attachment #8895841 - Attachment is obsolete: true
(In reply to Robert Strong (PTO until 8/14) [:rstrong] (use needinfo to contact me) from comment #39)
> I was going to fix this in bug 1386224 but this bug is fine.
Sorry, I meant bug 1388204
I asked this over in bug 1388204 but it isn't clear whether this is ok... would it be ok if beta updates always had showURL so it is possible to check if there is a problem earlier in the cycle? This is one of those cases where unit tests won't cover it since it requires a real application startup sequence.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Summary: WNP pages in Firefox 55 are not displayed → WNP pages in Firefox 56 are not displayed
For 55.0.2 build3 I've setup Firefox-55.0.2-build1-whatsnew in Balrog. It's basically the same process as for attachment 8895841 [details]/comment #32 but I've used 55.0.2 in the openURL. Diff from Firefox-55.0.2-build1 is:

1a2,3
>     "actions": "showURL",
>     "openURL": "https://www.mozilla.org/%LOCALE%/firefox/55.0.2/whatsnew/?oldversion=%OLD_VERSION%",
3c5
<     "detailsUrl": "https://www.mozilla.org/%LOCALE%/firefox/55.0.2/releasenotes/",
---
>     "detailsUrl": "https://www.mozilla.org/%LOCALE%/firefox/55.0/releasenotes/",
28c30
<     "name": "Firefox-55.0.2-build1",
---
>     "name": "Firefox-55.0.2-build1-whatsnew",
FTR, that's somewhat a guess of RelMan's intentions. openURL uses 55.0.2 because that still returns the same content, and is likely more useful to anyone looking at website analytics than joining data for updates to 55.0, 55.0.1, and 55.0.2 together. detailsUrl has 55.0 because the -whatsnew release will be used for updates from 54.0 and older, and the most the useful content in that case is the release notes for 55.0, rather than the incremental fixes in 55.0.2 (based on how we wrote the 55.0.1 anyway).
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #41)
> I asked this over in bug 1388204 but it isn't clear whether this is ok...
> would it be ok if beta updates always had showURL so it is possible to check
> if there is a problem earlier in the cycle? This is one of those cases where
> unit tests won't cover it since it requires a real application startup
> sequence.

I think that seems reasonable to me. Which URL would we show? https://www.mozilla.org/en-US/firefox/55.0.2/whatsnew/?oldversion=%OLD_VERSION this for 56 and something similar for 57? 

Sylvestre, Liz, what do you think? We could also let Softvision know to test for this in their beta sign offs.
Flags: needinfo?(rkothari) → needinfo?(sledru)
I think the default provided by Firefox is
https://www.mozilla.org/projects/firefox/%VERSION%/whatsnew/?oldversion=%OLD_VERSION%

which will automatically replace %VERSION% and %OLD_VERSION%

I'd like it for at least one update early in the beta cycle going forward.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #45)
> I think the default provided by Firefox is
> https://www.mozilla.org/projects/firefox/%VERSION%/whatsnew/
> ?oldversion=%OLD_VERSION%
> 
> which will automatically replace %VERSION% and %OLD_VERSION%
> 
> I'd like it for at least one update early in the beta cycle going forward.

The URLs for /whatsnew pages should look like:

https://www.mozilla.org/firefox/%VERSION%/whatsnew/?oldversion=%OLD_VERSION%

Locale is auto-detected (by browser preferences) and redirected to by mozilla.org.
Looks like someone blanked the prefs out in Firefox so the value has to come from balrog.
https://dxr.mozilla.org/mozilla-central/search?q=pref(%22startup.homepage_override_url&redirect=false
Robert, please explain me the impact of "would it be ok if beta updates always had showURL" ?
I don't know what it means for the users or the preparation of the WNP
I agree we need to be able to test the WNP before build/release day. 
I think what Robert is proposing is that we show it for every beta.
I'll need to double check what the logic is used by Firefox to show the what's new page. It is possible that it would only be shown one time normally though I can change the pref Firefox uses before updating to simulate the conditions of a version change when updating to test this in beta.

At a minimum I would like there to be at least one update on beta where the update metadata has showURL so I can verify that it is working properly.
> I think what Robert is proposing is that we show it for every beta.
I don't think we want that.

> At a minimum I would like there to be at least one update on beta where the update metadata has showURL so I can verify that it is working properly.
Looks like a good testing plan to avoid the 55 issue.
Flags: needinfo?(sledru)
I just verified that the page will only be shown to clients when there is a major version change due to the version that Firefox checks. This won't prevent me from verifying that the What's New page since the version it checks against is a preference that I can modify.

So, always including showURL, etc. in the update metadata on Beta except when updating from a previous beta version will make it so clients won't see the What's New page.

Ben, would it be reasonable to include showURL, etc. for beta updates that don't have a version change?
Flags: needinfo?(bhearsum)
Flags: needinfo?(lhenry)
Created attachment 8898817 [details] [diff] [review]
patch rev1
Attachment #8898817 - Flags: review?(mhowell)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #52)
> I just verified that the page will only be shown to clients when there is a
> major version change due to the version that Firefox checks. This won't
> prevent me from verifying that the What's New page since the version it
> checks against is a preference that I can modify.
> 
> So, always including showURL, etc. in the update metadata on Beta except
> when updating from a previous beta version will make it so clients won't see
> the What's New page.
> 
> Ben, would it be reasonable to include showURL, etc. for beta updates that
> don't have a version change?

I don't see why not from the Balrog side of things. I'll have to defer to someone else for the Release Promotion side. Mihai or Nick - what do you think?
Flags: needinfo?(nthomas)
Flags: needinfo?(mtabara)
Flags: needinfo?(bhearsum)
Attachment #8898817 - Flags: review?(mhowell) → review+

Comment 55

5 days ago
@rstrong,

we'd like to show the WNP to a user twice within every major release cycle.  To be clear, a release cycle is fx 55.x, 55.x.x etc.  

Im not clear on your definition of a "major release - does it mean a 56.0 release only and not a dot release?
Flags: needinfo?(robert.strong.bugs)
Flags: needinfo?(nthomas)
Flags: needinfo?(mtabara)
Adding back needinfo's for nthomas and mtabara for comment #54.

mhan, I don't know that code and it is well outside of application update. I only investigated it in regards to this bug and how it works on beta.
https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#98
Flags: needinfo?(robert.strong.bugs)
Flags: needinfo?(nthomas)
Flags: needinfo?(mtabara)
(In reply to mhan@mozilla.com from comment #55)
> @rstrong,
> 
> we'd like to show the WNP to a user twice within every major release cycle. 
> To be clear, a release cycle is fx 55.x, 55.x.x etc.  
> 
> Im not clear on your definition of a "major release - does it mean a 56.0
> release only and not a dot release?
A cursory check of the code leads me to believe that this won't be an issue since the call to Services.appinfo.platformVersion returns the actual version so 56.0.1 will be returned from that call. The reason the WNP won't be shown on beta except if showURL is specified and there is a version is that the actual version on beta is the same as the first release of a new version so 56.0 will be returned by the call to Services.appinfo.platformVersion for all beta updates until beta changes its version on merge day.
(In reply to mhan@mozilla.com from comment #55)
> @rstrong,
> 
> we'd like to show the WNP to a user twice within every major release cycle. 
> To be clear, a release cycle is fx 55.x, 55.x.x etc.  
> 
> Im not clear on your definition of a "major release - does it mean a 56.0
> release only and not a dot release?
With how often you are planning to show the WNP this defeats the purpose of using the update metadata to determine when to show the WNP. The point of this method was so the WNP would be rarely shown since it was one of the things clients complained about for a variety of reasons. If this does end up happening please consider using the method that existed prior too this one which is much simpler to show a WNP after a version change.

Comment 59

4 days ago
Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f349e0705a1c
use the active update and fallback to the latest update in the update history when determining whether to show the what's new page. r=mhowell

Comment 60

3 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f349e0705a1c
Status: REOPENED → RESOLVED
Last Resolved: 12 days ago3 days ago
status-firefox57: ? → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8898817 [details] [diff] [review]
patch rev1

Approval Request Comment
[Feature/Bug causing the regression]: bug 1301517
[User impact if declined]: Clients won't display the What's New page
[Is this code covered by automated tests?]: No, see comment #41 for info on how this can be manually tested before releases.
[Has the fix been verified in Nightly?]: I have verified this fixes the bug in Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No steps needed to reproduce this bug
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The change just checks the active update if present and falls back to the update in history if the active update isn't present.
[String changes made/needed]: None
Attachment #8898817 - Flags: approval-mozilla-beta?
Comment on attachment 8898817 [details] [diff] [review]
patch rev1

Please uplift to beta so we can test the WNP.
Attachment #8898817 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 63

a day ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/cb49f51c66fa
status-firefox56: ? → fixed
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #62)
> Comment on attachment 8898817 [details] [diff] [review]
> patch rev1
> 
> Please uplift to beta so we can test the WNP.
To verify this releng will need to add actions="showURL" and openURL="<url>" to the update xml and the browser.startup.homepage_override.mstone preference will need to be changed (I typically go with the previous version to simulate what typically happens on the client) so the Firefox code that opens the What's New page detects a version change.
You need to log in before you can comment on or make changes to this bug.