What's New page not displayed correctly after update
Categories
(Thunderbird :: General, defect, P1)
Tracking
(thunderbird_esr128? fixed, thunderbird129 affected, thunderbird130 fixed)
People
(Reporter: sancus, Assigned: freaktechnik)
References
(Blocks 1 open bug, Regression)
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
48 bytes,
text/x-phabricator-request
|
corey
:
approval-comm-esr128+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
Details | Review |
This function is not triggering correctly after updating to Thunderbird 128: https://searchfox.org/comm-esr128/rev/38abeb35c357a3032d84535d7c26da4ea37c4605/mail/base/content/specialTabs.js#1022
Possibly because there is no import for openLinkExternally.
This will need to be fixed before we launch 128. The What's New page should pop up on the first startup after updating, and then the donation appeal should appear on the 2nd restart.
Reporter | ||
Updated•1 month ago
|
Updated•1 month ago
|
Comment 1•1 month ago
|
||
The function isn't missing. Is it a Balrog problem? https://searchfox.org/comm-esr128/source/mail/branding/include/release-prefs.js#4
Reporter | ||
Comment 2•1 month ago
|
||
(In reply to Geoff Lankow (:darktrojan) from comment #1)
The function isn't missing. Is it a Balrog problem? https://searchfox.org/comm-esr128/source/mail/branding/include/release-prefs.js#4
Not sure, Rob was the one who suggested it might be an import issue.
Comment 3•1 month ago
|
||
Wasn't that updated here? https://hg.mozilla.org/comm-central/rev/2405535297f5f50389d9f1678cd286f571caf046
Comment 4•1 month ago
|
||
Comment 5•1 month ago
|
||
I uploaded a patch with the missing import, just in case that's the issue.
I can't test it right now because my local build is busted, trying to troubleshoot it.
Updated•1 month ago
|
Comment 6•1 month ago
•
|
||
I have high confidence this is the issue.
- https://searchfox.org/comm-esr128/rev/38abeb35c357a3032d84535d7c26da4ea37c4605/mail/base/content/messenger.js#174
calls specialTabs.openSpecialTabsOnStartup() - https://searchfox.org/comm-esr128/rev/38abeb35c357a3032d84535d7c26da4ea37c4605/mail/base/content/specialTabs.js#740
calls this.showWhatsNewPage(); - https://searchfox.org/comm-esr128/rev/38abeb35c357a3032d84535d7c26da4ea37c4605/mail/base/content/specialTabs.js#1040-1046 is the fun...
the override_url pref is the fallback default, usually empty. ingetPostUpdateOverridePage
it can be overridden by a "showURL" action in the XML returned by Balrog. That looks like this: (as of last night, abbreviated for clarity)
<?xml version="1.0"?><updates xmlns="http://www.mozilla.org/2005/app-update"><update xmlns="http://www.mozilla.org/2005/app-update" appVersion="115.12.2" buildID="20240621154414" channel="esr" detailsURL="https://live.thunderbird.net/thunderbird/releasenotes?locale=en-US&version=115.12.2&channel=esr" displayVersion="115.12.2" installDate="1720622824411" isCompleteUpdate="true" name="Thunderbird 115.12.2" previousAppVersion="115.12.0" promptWaitTime="86400" serviceURL="https://aus5.mozilla.org/update/6/Thunderbird/115.12.0/20240610193835/Linux_x86_64-gcc3/en-US/esr/Linux%206.9.8-zen1-1-zen%20(GTK%203.24.42%2Clibpulse%2017.0.0)/ISET:SSE4_2,MEM:128585/default/default/update.xml" type="minor" statusText="The Update was successfully installed"><patch size="62676836" type="complete" URL="https://download.mozilla.org/?product=thunderbird-115.12.2-complete&os=linux64&lang=en-US" finalURL="https://download-installer.cdn.mozilla.net/pub/thunderbird/releases/115.12.2/update/linux-x86_64/en-US/thunderbird-115.12.2.complete.mar" selected="true" state="succeeded" hashFunction="sha512" hashValue="03eabd9b8b106f289dcd4439f2c4d7b289211d61f67af2fb80edbf1f282000e21c8fc47de8b13b168a9729fd21d6f009abb346db77e6618638d5157af70dd531" internalResult="0"/></update></updates>
Assuming there's no "silent" in the update XML and the enterprise policy is not blocking it, the openURL property has the what's new page URL which is passed to openLinkExternally. (Side note, the "detailsURL" field is where the release notes URL comes from that is populated into the about: dialog and a few other places.)
Testing this scenario is a bit of a pain outside of a release context. There is toolkit/mozapps/update/tests/browser/browser_showWhatsNewPageTest.js
which could probably be adapted for Thunderbird.
Assignee | ||
Comment 7•1 month ago
|
||
I don't think the patch from aleca is the correct approach to resolve this.
When I bypass the MOZ_UPDATER
appconstant check in a local build the page opens just fine from what I can tell - however I haven't managed to get a working ESR build. I think my adaptation of that test you referred to does actually manage to pull the value from the XML, because I don't see any other way of it getting the test value - even though it seems to be opened before the test actually manages to run.
If some JS is failing you should see that in the error console of the build at least, and I see no error logs here so far?
Do you have any more concrete reproduction instructions?
Assignee | ||
Comment 8•1 month ago
•
|
||
If I try to apply the XML you pasted, it points out that it's not valid because of isCompleteUpdate "true"
instead of isCompleteUpdate="true"
.
EDIT: if I correct that the page gets correctly opened.
Comment 9•1 month ago
|
||
The call to um.getReadyUpdate()
fails here: https://searchfox.org/comm-esr128/rev/38abeb35c357a3032d84535d7c26da4ea37c4605/mail/base/content/specialTabs.js#1037(In reply to Martin Giger [:freaktechnik] from comment #8)
If I try to apply the XML you pasted, it points out that it's not valid because of
isCompleteUpdate "true"
instead ofisCompleteUpdate="true"
.
Gah! Copy/paste problem. The "=" is there. Fixed above.
Comment 10•1 month ago
|
||
(In reply to Martin Giger [:freaktechnik] from comment #8)
If I try to apply the XML you pasted, it points out that it's not valid because of
isCompleteUpdate "true"
instead ofisCompleteUpdate="true"
.EDIT: if I correct that the page gets correctly opened.
Good catch, thanks for confirming.
I also had big suspicion that my patch was kinda useless and not the cause of the problem due to no console errors.
Comment 11•1 month ago
|
||
what i've been doing is
- install 115 & setting up a mailbox
- set the update channel to esr-localtest-next.
- run the update and stop tb
- grab specialtabs.js out of
updated/omni.ja
, and stick a "debugger" line at the beginning ofshowWhatsNewPage()
- start tb with
--jsdebugger --wait-for-jsdebugger
- then step through. i fail at the um.getReadyUpdate() line as it returns null.
Reporter | ||
Comment 12•1 month ago
•
|
||
(In reply to Martin Giger [:freaktechnik] from comment #8)
If I try to apply the XML you pasted, it points out that it's not valid because of
isCompleteUpdate "true"
instead ofisCompleteUpdate="true"
.EDIT: if I correct that the page gets correctly opened.
As far as I can tell, the XML mistake was only here on Bugzilla. It's not working even with the correct XML on 115.
Assignee | ||
Comment 13•1 month ago
|
||
I think the fix is
diff --git a/mail/base/content/specialTabs.js b/mail/base/content/specialTabs.js
--- a/mail/base/content/specialTabs.js
+++ b/mail/base/content/specialTabs.js
@@ -1019,7 +1019,7 @@ var specialTabs = {
*
* @see {BrowserContentHandler.needHomepageOverride}
*/
- async showWhatsNewPage() {
+ showWhatsNewPage() {
const old_mstone = Services.prefs.getCharPref(
"mailnews.start_page_override.mstone",
""
@@ -1034,7 +1034,7 @@ var specialTabs = {
const um = Cc["@mozilla.org/updates/update-manager;1"].getService(
Ci.nsIUpdateManager
);
- const update = await um.getReadyUpdate();
+ const update = um.updateInstalledAtStartup;
if (update && Services.vc.compare(update.appVersion, old_mstone) > 0) {
let overridePage = Services.urlFormatter.formatURLPref(
correcting a porting mistake from bug 1897367.
Comment 14•1 month ago
•
|
||
I can confirm this fix does work -- ran with patched omni.ja without using debugger. (both alex's and martin's patches applied)
Assignee | ||
Comment 15•1 month ago
|
||
Assignee | ||
Comment 16•1 month ago
|
||
(In reply to Rob Lemley [:rjl] from comment #14)
(both alex's and martin's patches applied)
I've been testing without alex's patch and that's worked fine, so I don't think we need that one.
Assignee | ||
Updated•1 month ago
|
Updated•1 month ago
|
Updated•1 month ago
|
Reporter | ||
Updated•1 month ago
|
Reporter | ||
Updated•1 month ago
|
Reporter | ||
Comment 17•1 month ago
|
||
Comment on attachment 9412094 [details]
Bug 1906983 - Fix what's new page not showing up after update. r=#thunderbird-reviewers
[Approval Request Comment]
Regression caused by (bug #): Bug 1897367
User impact if declined: No what's new page
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): Little other than the What's New page still being broken.
Comment 18•1 month ago
•
|
||
Comment on attachment 9412094 [details]
Bug 1906983 - Fix what's new page not showing up after update. r=#thunderbird-reviewers
[Triage Comment]
Approved for esr128
Assignee | ||
Comment 19•1 month ago
•
|
||
I think beta might need a different patch, there were more changes on m-c for 129 in this area I'm currently looking into.
Reporter | ||
Comment 20•1 month ago
|
||
(In reply to Martin Giger [:freaktechnik] from comment #19)
I think beta might need a different patch, there were more changes on m-c for 129 in this area I'm currently looking into.
If the patch doesn't work on trunk, we should just uplift this one, and check a normal fix into comm-central and let it ride the merges. Beta doesn't need a what's new page right now.
Comment 21•1 month ago
|
||
Comment on attachment 9412094 [details]
Bug 1906983 - Fix what's new page not showing up after update. r=#thunderbird-reviewers
Removed approval-comm-beta in favor of picking up the comm-central fix in the next merge.
Assignee | ||
Comment 22•1 month ago
|
||
Pushed to ESR: https://hg.mozilla.org/releases/comm-esr128/rev/5b8f11ce4188f2d46aa8021a390e543e92a23e0d
I'll provide a patch for comm-central that also includes a test to cover this code and it'll follow the latest changes in m-c, too.
Assignee | ||
Updated•1 month ago
|
Assignee | ||
Comment 23•1 month ago
|
||
This is the continuation of the changes needed for ESR including porting the browser_showWhatsNewPageTest.js
from mozilla-central. I've stripped all the support files to only include things used by this test except the
setup/teardown in the head file. Further I've ignored the platformVersion vs. appVersion things Firefox does,
since it seems we only care about appVersion.
Updated•1 month ago
|
Assignee | ||
Comment 24•1 month ago
|
||
Please check in just the second, newer patch that hasn't been uplifted to ESR.
Comment 25•1 month ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/4c6d065ee010
Fix what's new page after update and add tests. r=mkmelin
Description
•