1643309 - About dialog falsely claims App "is up to date" when offline

Status: RESOLVED FIXED

(Toolkit :: Application Update, enhancement, P3)

Description

[Thomas D. (:thomas8)]

Attachment: Screenshot 1: TB offline --> "Daily is up to date" (that's a lie)

STR

1. Have a version of Daily which hasn't been started for some days (so it hasn't seen/downloaded updates yet), and start that.
2. Go offline in TB (click on icon in TB's lower-left corner); computer offline works, too, I think
3. Check About Thunderbird Daily dialogue
4. For comparison: Be online, check again.

Actual result

• Daily falsely claims that "Daily is up to date" (see screenshot)

Expected result

• Daily should be honest and say "Daily cannot check for updates because you are offline."

Comment 1

[Thomas D. (:thomas8)]

Attachment: Screenshot 2: TB online --> "Checking for updates..." --> "Update to 79.0a1" (that's right)

So in the same session, after switching TB online again (see status icon in lower-left corner), here's the truth on update status.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:nalexander, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Nick Alexander :nalexander [he/him]]

First, Thomas, thanks for the report. I think some kind of signal that the update checked failed (due to being offline, or almost any other reason) could be useful. But it's some effort to add additional states, and even if we can check for updates there's no particular expectation that we can also fetch them.

I think this likely applies to Firefox as well as Thunderbird so I generalized the title.

This is not a good first bug, but it might be a good second or third bug for a contributor; I'll mark it as such. If someone with a little experience inside Firefox (simply because that's what I know) is interested, I can try to do some reading through this code to provide some direction on how this feature might be implemented.

Comment 4

[Priyank Singh]

Hi Nick,
Would like to work on this issue.
It think it would be better to display a signal like you said that app couldn't check for updates as the user is offline, regardless the update is available or not.

But it's some effort to add additional states

Could you elaborate this more?

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Priyank Singh from comment #4)

Hi Nick,
Would like to work on this issue.

Hi Priyank,

Thanks for your interest. As I said in #c3, this is not a good first bug. Do you have experience working on Firefox already? This is going to be a challenge even for somebody with some experience.

Comment 6

[Priyank Singh]

I've made some contributions to firefox frontend before, however I only worked with xhtml, css and js files not on the core (c++) code.

With some guidance and overview as what to do, I think I will be able to implement the solution. :)

Comment 7

[Priyank Singh]

gentle ping @Nick

Comment 8

[Priyank Singh]

Hi nick, I would like to know which files contains the related code to implement the solution.
We need to check whether firefox is online or offline and display the required message in about dialog box right?

Also what do you mean by checking additional states in your previous comment.

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Priyank Singh from comment #8)

Hi nick, I would like to know which files contains the related code to implement the solution.
We need to check whether firefox is online or offline and display the required message in about dialog box right?

Also what do you mean by checking additional states in your previous comment.

Priaynk! Sorry for the slow reply.

I think the relevant code is in the "about dialog" -- see the files here -- and in the Firefox AppUpdater module.

AppUpdater defines a state machine and the states are declared in AppUpdater.STATUS. Notice that there is a DOWNLOAD_FAILED but there's no notion of CHECKING_FAILED. That is, when we check, the state machine transitions from CHECKING to either DOWNLOADING (if an update is found) or NO_UPDATES_FOUND (if no update is found).

What this ticket is trying to do is to add a CHECKING_FAILED state.

There's some complication because there seem to be two implementations of the app updater -- a new one and a legacy one. I'll right instructions for the new one and ask what the status is of the legacy one.

1. You'll need to add the declaration and to wire in that new state to the state machine around here.

2. You'll need to update the user interface code to show your new message around here. This will require a new section in aboutDialog.xhtml and new strings in aboutDialog.ftl.

3. Finally, you'll need to figure out other places that use the AppUpdater state machine and make sure that they do the correct thing. I really only see UrlbarProviderInterventions. You could either add new UI (longer, harder) or make your new state act like NO_UPDATES_FOUND for that module.

4. You'll need to add tests for your changes -- I'm not quite sure how to test this right now.

These should all be separate patches/commits, at least at first. See how you get on, Priyank!

Comment 10

[Nick Alexander :nalexander [he/him]]

adw: it looks like there are two implementations of app checking in the tree, and that the pref for toggling defaults to "false", meaning use the old one. Is that correct? Do we have a plan to remove the legacy app updater?

Comment 11

[Drew Willcoxon :adw]

Bug 1600864 covers removing the old updater. I've been meaning to come back to it but lost track. Do you need me to finish it soonish?

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Drew Willcoxon :adw from comment #11)

Bug 1600864 covers removing the old updater. I've been meaning to come back to it but lost track. Do you need me to finish it soonish?

Priyank: this gets better! My guess is that Bug 1600864 is a good ticket for you to work on before this one -- a little simpler, I think. I've asked Drew to add some details about what needs to be done; let's see what he says. (You're welcome to work on this in the interim, but I'd like to see that one done before this lands.)

Comment 13

[Drew Willcoxon :adw]

Bug 1600864 isn't a good candidate for a mentored bug, so I've updated my old patch and requested review. I think we should finish that bug before starting on this one.

Comment 14

[Priyank Singh]

@Nick Thanks for the info you provided as what needs to be done.

Now as I can see @Drew submitted the patch to remove the legacy updater I think I would be good if I can start working on this issue right away.

Comment 15

[Priyank Singh]

Attachment: Bug 1643309 - Added and wired CHECKING_FAILED state

Comment 16

[Priyank Singh]

Attachment: Bug 1643309 - Updated the user interface code to show checking failed message

Depends on D97971

Comment 17

[Priyank Singh]

:nalexander
I have pushed the code and added some comments in phabricator please check.

Comment 18

[Nick Alexander :nalexander [he/him]]

(In reply to Priyank Singh from comment #17)

:nalexander
I have pushed the code and added some comments in phabricator please check.

Priyank -- I see the code and am sorry I didn't review last week. I am sick just this moment but will get to it by the end of this week. Leaving NI so I don't miss it. Sorry for the delay!

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 20

[Nick Alexander :nalexander [he/him]]

I'm going to clear my NI since :bytesized is doing all the work here -- thanks, :bytesized!

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 22

[Leslie]

Hi my name is Leslie and I am an Outreachy applicant. Can I work on this bug?

Comment 23

[Nick Alexander :nalexander [he/him]]

(In reply to Leslie from comment #22)

Hi my name is Leslie and I am an Outreachy applicant. Can I work on this bug?

Sorry for the long delay, Leslie! Yes, you certainly can. Kirk (:bytesized) was working on this earlier and can help. It can be difficult to pick up partial work done by somebody else, but you could start by seeing whether you can apply the existing patches and figuring out what review comments are left to work through.

Good luck! (NI back to you just to be sure you saw this reply.)

Comment 24

[gloriaeskor]

Hi, I am an outreachy intern. Can you assign this to me?

Comment 25

[gloriaeskor]

Hi, I am an outreachy intern. Can you assign this to me?

Comment 26

[Nick Alexander :nalexander [he/him]]

(In reply to gloriaeskor from comment #25)

Hi, I am an outreachy intern. Can you assign this to me?

Generally I prefer to assign after patches are posted and reviewed. If you're interested in picking up this work, feel free! There's a good deal of context and prior art to absorb, though. To start, take the existing patches, import them using moz-phab patch ..., and rebase them onto a current mozilla-central using hg rebase. They will likely need to be updated as they are old.

Comment 28

[David Balažic]

This still happens, just had it with v95.0.2, about dialog says "Firefox is up to date" when in fact 96.0.1 is already out for days. The host has some network problems, which I assume triggered the problem.

This seems related to bug 679742.

Comment 29

[HansJuergen]

Dear people from Mozilla, it is an impossibility that such a security related bug is not fixed even after years and drags on and on. Where is the problem to check if the update server is reachable and if not, indicate that it cannot be determined if the version is up to date? That's 2 or 3 lines of code, what's your problem? I think that the problem needs to be reported to various security institutes before any programmer gets around to fix this extreme security bug?

Comment 30

[Thomas D. (:thomas8)]

Thunderbird is affected by this, and we consider providing false information to the user about the update status of their application a significant security risk. Also highly irritating for support scenarios. Firefox bug 679742 has 6 duplicates, and we have a number of reincarnations and abandoned attempts all over the place - this looks like the one with the highest chance of success.

Kirk (:bytesized), thank you for your reviews here. I am seeing one patch which is already accepted.

• Looking at your last review comment on the other patch, is this just lacking a test for completion?
• Given that volunteer contributors have gone (maybe not a good-first-bug in its entirety after all), would you be able to finish this off?

Comment 31

[Thomas D. (:thomas8)]

(In reply to David Balažic from comment #28)

This seems related to bug 679742.

Yes, thanks David for pointing that out!

Comment 32

[Thomas D. (:thomas8)]

• Can someone explain the meaning of [fidedi-ope] in whiteboard, which relates to see-also link https://mozilla-hub.atlassian.net/browse/FIDEDI-72?
• Does [fidedi-ope] constitute a spelling error and should be [fidedi-open] instead?
• If yes, does this perhaps contribute to the lack of attention?
• If yes, should [fidedi-open] perhaps become a keyword to avoid misspelling?

Comment 33

[Nick Alexander :nalexander [he/him]]

Folks, this is an issue tracker. It's not a place to advocate for your particular issue to be addressed. If non-technical discussion persists here, we'll have to lock the ticket. I'd prefer not to do so.

:thomas8: please do not change the priority and severity flags in this Bugzilla component. They mean specific things to the module owner (:bytesized) and the team that owns these components (including me).

(In reply to Thomas D. (:thomas8) from comment #32)

• Can someone explain the meaning of [fidedi-ope] in whiteboard, which relates to see-also link https://mozilla-hub.atlassian.net/browse/FIDEDI-72?

Yes. fidedi stands for FIrefox Desktop Engineering Desktop Integrations (a mouthful, I know). ope stands for OPerational Excellence.

• Does [fidedi-ope] constitute a spelling error and should be [fidedi-open] instead?
• If yes, does this perhaps contribute to the lack of attention?
• If yes, should [fidedi-open] perhaps become a keyword to avoid misspelling?

No, it shouldn't. I'm reminded of Chesterton's Fence: things may be correct but not as you expect.

Thunderbird is affected by this, and we consider lying to the user about the update status of their application a significant security risk. Also highly irritating for support scenarios. Firefox bug 679742 has 6 duplicates, and we have a number of reincarnations and abandoned attempts all over the place - this looks like the one with the highest chance of success.

If there's anything hilarious about this ticket, it's that Thunderbird doesn't use Firefox's implementation for the About dialog. The work in this ticket -- that I broke down and tried to make happen -- tracks fixing a weakness in Firefox's About dialog state machine. Thunderbird doesn't use it: see https://searchfox.org/comm-central/source/mail/base/content/aboutDialog-appUpdater.js, and note how it's not using browser/modules/AppUpdater.jsm. Y'all can go fix this on your own time.

Comment 34

[Nick Alexander :nalexander [he/him]]

S4 -- there are a million workarounds. P3 -- it should be in our backlog, but we'd happily take a patch.

Comment 35

[Robin Steuber (she/her) [:bytesized]]

(In reply to HansJuergen from comment #29)

Dear people from Mozilla, it is an impossibility that such a security related bug is not fixed even after years and drags on and on.

I wouldn't consider this to be a security bug. We have other mechanisms to alert users if updates fail to be installed for too long. If those mechanisms stop working correctly, that would likely have security implications.

That's 2 or 3 lines of code, what's your problem?

This comment arguably violates the "Commenting Guidelines" in the Bugzilla Etiquette rules. Since you are new here I'm not going to take any action on this yet, but you need to be more careful to follow the rules in the future.

Since it is such an easy fix, perhaps you would like to help out? I'd be happy to help you get started.

To answer your question, however, our "problem" is that this is not the only important work that we have to do. We have to do our best to make the highest impact that we can with the engineering time that we have available to us.

I think that the problem needs to be reported to various security institutes

Be my guest. I have to admit to some curiosity about what security institutes you have in mind.

Comment 36

[Robin Steuber (she/her) [:bytesized]]

(In reply to Thomas D. (:thomas8) from comment #30)

• Looking at your last review comment on the other patch, is this just lacking a test for completion?

It's been a while since I've looked at this, but that sounds about right to me. However, it's possible that this patch would no longer apply cleanly. Someone would have to give it a try to be sure.

• Given that volunteer contributors have gone (maybe not a good-first-bug in its entirety after all), would you be able to finish this off?

I would like nothing more than for this to be the highest priority work available to me. Unfortunately, it is not. So it will have to wait until the higher priority work is done, or a volunteer is able to finish it.

---

Also, as :nalexander already said, please don't change around the flags on bugs the way you just did. Doing this is a clear violation of the first "Changing Fields Guideline" in the Bugzilla Etiquette rules.

Comment 39

[Max]

Attachment: Bug 1643309 - About dialog falsely claims App "is up to date" when offline, r=bytesized

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e87383813223
About dialog falsely claims App "is up to date" when offline, r=bytesized,application-update-reviewers,fluent-reviewers,settings-reviewers,bhearsum,flod

Comment 41

[Norisz Fay [:noriszfay]]

Backed out for causing bc failures on browser_aboutDialog_fc_check_cantApply.js

Backout link

Push with failures

Failure log

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/850c9756f206
About dialog falsely claims App "is up to date" when offline, r=bytesized,application-update-reviewers,fluent-reviewers,settings-reviewers,bhearsum,flod

Comment 44

[Max]

Wow! Thanks for linking this really old issue, Kai. It was an interesting read.

I am carefully optimistic, that the problem is finally fixed by the latest revision.

Comment 45

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/850c9756f206