Closed Bug 1492459 Opened 6 years ago Closed 6 years ago

Avast/AVG deleting language packs rendering the browser unusable

Categories

(Toolkit :: Add-ons Manager, defect)

62 Branch
Unspecified
Windows
defect
Not set
blocker

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 blocking verified
firefox63 --- verified
firefox64 --- verified

People

(Reporter: philipp, Assigned: kmag)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

Attached image screenshot
we got numerous user reports in the past few hours that avast/avg software was deleting language pack addons in firefox rendering the browser unusable as only a yellow background and the following text is displayed:
><window id="main-window"
>^

as a side note: others at https://old.reddit.com/r/firefox/comments/9h3kui/avg_warnings_for_unknown_firefox_addon/ also reported it was deleting system addons like "telemetry coverage" that are part of firefox.
Lukas, can you help get us connected to someone on Avast's end for this issue? Bricking people's Firefox installations is pretty unacceptable.
Flags: needinfo?(rypacek)
Should we hold 62.0.2 until we figure this out? Removing a language pack shouldn't brick the browser, it should fall back to English.
Yes that's probably a good idea.
Avast is also deleting the Telemetry Coverage Add-on on computers it is deployed on.
I can confirm that the bug is easy to reproduce:
* Install Firefox 62
* Install a language pack, e.g. fr, enabled it by setting intl.locale.requested = fr
* Remove the .xpi file from the profile. It doesn't matter if Firefox is open or not.

In my case, the first time the window started but was bricked. Running from command line with -purgecaches gave me the same error.
Andrew, David, can the addons team take a look at this and see if there's something we can do to have better fallback?
Flags: needinfo?(ddurst)
Flags: needinfo?(aswan)
I can't reproduce on 61.0.2, following steps from comment 6. Same behavior was reported by philipp on IRC.
Notes based on debugging/testing:

Does not happen on Firefox 61. Seems similar to the Acer problem.

Deleting addonStartup.json.lz4 fixes the problem.

Errors are like this:

JavaScript error: resource://gre/modules/AddonManager.jsm, line 850: NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIStringBundle.GetStringFromName]
(In reply to Julien Cristau [:jcristau] from comment #7)
> Andrew, David, can the addons team take a look at this and see if there's
> something we can do to have better fallback?

I think this is more about language handling which I'm not so familiar with.
Flags: needinfo?(l10n)
Flags: needinfo?(gandalf)
Flags: needinfo?(aswan)
My suspicion is that addonStartup.json.lz4 contains data that we don't validate enough, and then ask the chrome registry to do stuff it can't.

I dug a bit into it, and I didn't find anything close that changed recently (the code reading it is from 55-57)
Flags: needinfo?(l10n)
Flags: needinfo?(gandalf)
I'm not sure what to say, to verify that the targets we're adding to the chrome registry actually exist would mean doing extra I/O during startup, which we've worked very hard to avoid.
Maybe Kris has a clever idea.
Flags: needinfo?(kmaglione+bmo)
But this worked in 61, so clearly something changed...
So, looking into the file that flod shared on slack, this is the data about the langpack:

{"langpack-fr@firefox.mozilla.org":{"dependencies":[],"enabled":true,"hasEmbeddedWebExtension":false,"lastModifiedTime":1537368073000,"path":"langpack-fr@firefox.mozilla.org.xpi","runInSafeMode":false,"signedState":2,"telemetryKey":"langpack-fr%40firefox.mozilla.org:62.0buildid20180830143136","version":"62.0buildid20180830143136","type":"webextension-langpack","startupData":{"chromeEntries":[["locale","necko","fr","chrome/fr/locale/fr/necko/"],["locale","pdf.js","fr","browser/chrome/fr/locale/pdfviewer/"],["locale","global","fr","chrome/fr/locale/fr/global/"],["locale","onboarding","fr","browser/features/onboarding@mozilla.org/fr/locale/fr/"],["locale","devtools","fr","browser/chrome/fr/locale/fr/devtools/client/"],["locale","pipnss","fr","chrome/fr/locale/fr/pipnss/"],["locale","alerts","fr","chrome/fr/locale/fr/alerts/"],["locale","devtools-startup","fr","browser/chrome/fr/locale/fr/devtools/startup/"],["locale","branding","fr","browser/chrome/fr/locale/branding/"],["locale","autoconfig","fr","chrome/fr/locale/fr/autoconfig/"],["locale","devtools-shared","fr","browser/chrome/fr/locale/fr/devtools/shared/"],["locale","global-platform","fr","chrome/fr/locale/fr/global-platform/mac/"],["locale","browser-region","fr","browser/chrome/fr/locale/browser-region/"],["locale","mozapps","fr","chrome/fr/locale/fr/mozapps/"],["locale","pluginproblem","fr","chrome/fr/locale/fr/pluginproblem/"],["locale","formautofill","fr","browser/features/formautofill@mozilla.org/fr/locale/fr/"],["locale","weave","fr","chrome/fr/locale/fr/services/"],["locale","pocket","fr","browser/features/firefox@getpocket.com/fr/locale/fr/"],["locale","places","fr","chrome/fr/locale/fr/places/"],["locale","passwordmgr","fr","chrome/fr/locale/fr/passwordmgr/"],["locale","pippki","fr","chrome/fr/locale/fr/pippki/"],["locale","webcompat-reporter","fr","browser/features/webcompat-reporter@mozilla.org/fr/locale/fr/"],["locale","browser","fr","browser/chrome/fr/locale/browser/"]],"langpackId":"langpack-fr-browser","l10nRegistrySources":{"browser":"browser/"},"languages":["fr"]}}},"staged":{},"path":"/Users/flodolo/Library/Application Support/Firefox/Profiles/uvu63qf9.bug1492459/extensions"}

https://dxr.mozilla.org/mozilla-release/source/toolkit/components/extensions/Extension.jsm#1921-1925 adds that to the chrome registry?

The chrome registry doesn't do runtime fallback, so once we tell it to use this chrome, it'll just die merrily.
(In reply to Mike Kaply [:mkaply] from comment #13)
> But this worked in 61, so clearly something changed...

Prior to that, we loaded the chrome registry entries from the chrome.manifest in the extension. Now we load them directly from addonStartup.json.lz4, along with the locale service registrations.
Flags: needinfo?(kmaglione+bmo)
> Prior to that, we loaded the chrome registry entries from the chrome.manifest in the extension. Now we load them directly from addonStartup.json.lz4, along with the locale service registrations.

Firefox 61 langpacks didn't have chrome.manifest, they were the new manifest.json format. We haven't had chrome.manifest since Firefox 58.
(In reply to Mike Kaply [:mkaply] from comment #16)
> > Prior to that, we loaded the chrome registry entries from the chrome.manifest in the extension. Now we load them directly from addonStartup.json.lz4, along with the locale service registrations.
> 
> Firefox 61 langpacks didn't have chrome.manifest, they were the new
> manifest.json format. We haven't had chrome.manifest since Firefox 58.

No idea, then. Nothing significant changed in that timeframe.
Assignee: nobody → kmaglione+bmo
For most extension types, a missing or changed XPI file is not a serious
issue, since a failure to start it does not cause any real problems, and can
be rectified after startup. For language packs, though, we need to eagerly
register the resources that they provide, and if those resources are missing,
the browser becomes unusable.

This patch changes the startup modification checks to always include language
packs, even in profile directories. This will be a slight performance hit, but
language pack usage is low enough that it shouldn't affect most users.
Comment on attachment 9010370 [details]
Bug 1492459: Always check langpacks for modification at startup. r=aswan

Andrew Swan [:aswan] has approved the revision.
Attachment #9010370 - Flags: review+
This is a pretty big patch for a dot release, so relman will probably want some sense of risk.
Component: Other → Add-ons Manager
Flags: qe-verify+
Product: External Software Affecting Firefox → Toolkit
Version: unspecified → 62 Branch
i tested those inbound builds locally and the steps from comment #6 no longer reproduce :)
https://hg.mozilla.org/mozilla-central/rev/3f7206269517
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment on attachment 9010370 [details]
Bug 1492459: Always check langpacks for modification at startup. r=aswan

Approval Request Comment
[Feature/Bug causing the regression]: 
[User impact if declined]: Firefox not working if files are deleted.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Working on it.
[Needs manual test from QE? If yes, steps to reproduce]: Steps are in bug.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Leaving to Kris to assess risk.
[Why is the change risky/not risky?]:
[String changes made/needed]:

Kris: please give a risk assessment. Thanks!
Flags: needinfo?(kmaglione+bmo)
Attachment #9010370 - Flags: approval-mozilla-release?
Attachment #9010370 - Flags: approval-mozilla-beta?
This needs QA ASAP. I'll send you both a link to the doc with more details.
Flags: needinfo?(brindusa.tot)
Flags: needinfo?(andrei.vaida)
(In reply to Mike Kaply [:mkaply] from comment #24)
> [Is the change risky?]: Leaving to Kris to assess risk.
> [Why is the change risky/not risky?]:
> [String changes made/needed]:
> 
> Kris: please give a risk assessment. Thanks!

Low-risk. This change only affects the handling of deleted add-ons at startup, primarily language packs. There's a chance it may change some minor aspects of how we handle extensions being deleted from global install locations, but those changes would be more likely to fix existing bugs than to add new ones. It won't affect in-profile add-ons other than langpacks at all.
Flags: needinfo?(kmaglione+bmo)
Confirmed that this grafts cleanly to mozilla-release and looks good on Try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebbe1724f85ed65feb1ceb161cb21ec0b1cf67eb
Comment on attachment 9010370 [details]
Bug 1492459: Always check langpacks for modification at startup. r=aswan

approved for 63.0b8 and 62.0.2

pascal fyi
Flags: needinfo?(pascalc)
Attachment #9010370 - Flags: approval-mozilla-release?
Attachment #9010370 - Flags: approval-mozilla-release+
Attachment #9010370 - Flags: approval-mozilla-beta?
Attachment #9010370 - Flags: approval-mozilla-beta+
Severity: normal → blocker
We reproduced the issues on 62.0 RC with help from steps posted in comment 6. We verified that using latest Nightly after this fix landed (Windows 10 32/64bit, Windows 7 32/64bit) the issue does not reproduce anymore. Just to be on the safe side we also did a basic smoke check after deleting the lang pack to see that nothing broke.

One issues was found though, bug 1492838 but it's not something that this bug caused because it also affects older Firefox versions on different platforms (mac and linux as well).

For more details about our testing please see the following testplan: https://docs.google.com/document/d/1qyijSb_8Ztl6lRu2nCmXYrXLfHPrulxGTvrqefEYlh0/edit#heading=h.c1k9kvic8ruu
Flags: needinfo?(brindusa.tot)
Flags: needinfo?(andrei.vaida)
Flags: needinfo?(pascalc)
Also verified that both Firefox 63.0b8 and Firefox 62.0.2rc are not affected by this behavior anymore.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(rypacek)
Flags: needinfo?(ddurst)
Depends on: 1524679
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: