1826234 - Crash in [@ mozilla::Preferences::InitializeUserPrefs] after creating /etc/firefox/defaults/pref to attempt to set default prefs

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect)

Description

[Daniel Holbert [:dholbert]]

Crash report: https://crash-stats.mozilla.org/report/index/b8f28591-c563-474b-a529-448b00230403

Reason: SIGSEGV / SEGV_MAPERR

Top 9 frames of crashing thread:

0  libxul.so  mozilla::Preferences::InitializeUserPrefs  modules/libpref/Preferences.cpp:3929
1  libxul.so  XREMain::XRE_mainRun  toolkit/xre/nsAppRunner.cpp:5343
2  libxul.so  XREMain::XRE_main  toolkit/xre/nsAppRunner.cpp:5843
3  libxul.so  XRE_main  toolkit/xre/nsAppRunner.cpp:5899
4  firefox-bin  do_main  browser/app/nsBrowserApp.cpp:226
4  firefox-bin  main  browser/app/nsBrowserApp.cpp:423
5  libc.so.6  __vfwscanf_internal.cold  stdio-common/stdio-common/vfscanf-internal.c:526
6  libc.so.6  __vfwscanf_internal.cold  stdio-common/stdio-common/vfscanf-internal.c:526
7  firefox-bin  _start  

In a debug build, I get this fatal assertion instead:

Assertion failure: !gAccessCounts, at modules/libpref/Preferences.cpp:3674

My STR:

1. On Ubuntu 22.04, create a file /etc/firefox/defaults/pref with these contents:

pref('browser.startup.homepage', 'https://www.yahoo.com');

2. Start Firefox Nightly, with your existing profile or a fresh profile (doesn't seem to matter for me)

ACTUAL RESULT:
Instant startup crash, with this [@ mozilla::Preferences::InitializeUserPrefs] signature.

Comment 1

[Daniel Holbert [:dholbert]]

[Tracking Requested - why for this release]: Startup crash, prevents users from starting Firefox.

Note, this is the same crash signature as in bug 1664614, but I've filed this separately since I'm not sure if it's the same root issue or if there are other things going on there, and because this only started being an issue for official Mozilla-provided builds quite recently. It appears that the /etc/firefox/defaults/pref file has only started doing something for official builds as of bug 1170092, which landed on March 2nd. (Before that point, it looks like this was a flatpak/snap-specific feature before that point?)

So from the perspective of official Mozilla-provided builds, this bug manifests as a regression from bug 1170092; marking as such. (Nightly 2023-03-02 is fine with the STR here, while Nightly 2023-03-03 insta-crashes. Confirmed with mozregression.)

Comment 2

[Daniel Holbert [:dholbert]]

I actually hit this same crash even if /etc/firefox/defaults/pref is just an entirely-empty file. No prefs needed at all.

So there's nothing pref-specific here.

Comment 3

[Daniel Holbert [:dholbert]]

Aha, looking at bug 1170092's patch more closely, it looks like Firefox is expecting /etc/firefox/defaults/pref to be a directory, and it must be getting quite confused when it finds a file instead, I guess? :)

Anyway: we should fail gracefully here (just ignoring the pref file) instead of crashing.

Comment 4

[Daniel Holbert [:dholbert]]

(If I perform the STR, but I put my pref values in /etc/firefox/defaults/pref/test.js instead of /etc/firefox/defaults/pref, then I get expected results -- i.e. I can start Firefox with a fresh profile, and it successfully uses my provided default value.)

Comment 5

[Daniel Holbert [:dholbert]]

jhorak, maybe you could take a look at this as a followup to bug 1170092? We should try to fix this ASAP, since it's in Firefox 112 which ships to release in 1 week.

Otherwise [extrapolating from my own experience] any Linux user who happens to inadvertently create [or have-previously-created] a file called /etc/firefox/defaults/pref (due to misreading, or having-misread-in-the-past, instructions for setting default preferences) will have Firefox become entirely inoperable.

Comment 6

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

Set release status flags based on info from the regressing bug 1170092

Comment 7

[Daniel Holbert [:dholbert]]

Actually I figured I'd just take a look at this myself. Here's what happens.

We reach the relevant code here, added in bug 1170092, which culminates in the pref_LoadPrefsInDir call here:
https://searchfox.org/mozilla-central/rev/98397ff4eac3d32b815fbb33bff147297fb972d7/modules/libpref/Preferences.cpp#5006-5009,5027-5033

rv = pref_LoadPrefsInDir(defaultSystemPrefDir, nullptr, 0);
if (NS_FAILED(rv)) {
  NS_WARNING("Error parsing application default preferences.");
}
...
nsCOMPtr<nsIObserverService> observerService = services::GetObserverService();
NS_ENSURE_SUCCESS(rv, rv);

observerService->NotifyObservers(nullptr, NS_PREFSERVICE_APPDEFAULTS_TOPIC_ID,
                                 nullptr);

return NS_OK;

pref_LoadPrefsInDir ends up returning an error code, NS_ERROR_FILE_DESTINATION_NOT_DIR, which is stored in rv, and it doesn't load any preferences. That's ~fine.

After that point, nothing modifies or uses rv at all except for one early-return, in the NS_ENSURE_SUCCESS(rv,rv) check, much further down. Proximity and hg-blame suggests that this NS_ENSURE_SUCCESS expression was intended to check for GetObserverService failure. But these days that API seems to be infallible (or at least we don't capture its error state using rv). So we end up inadvertently doing an early-return due to the fact that rv still has our error code.

So: we probably want to reset rv inside of our NS_WARNING case, and perhaps to just remove this NS_ENSURE_SUCCESS statement since it's effectively a no-op except when it misfires like this.

Comment 8

[Daniel Holbert [:dholbert]]

Historical note: this NS_ENSURE_SUCCESS(rv,rv) seems to date back to a typo in 2017, in this chunk:
https://hg.mozilla.org/mozilla-central/rev/ed7b5443cf8b98c807cd1fa29857b51511cd0076#l1.322

There's no reason we should have been checking rv there as a measure for whether GetObserverService() failed; that was probably copypasta from elsewhere in that patch where rv was indeed involved.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Bug 1826234: Fix GetObserverService error-handling check in Preferences::InitInitialObjects. r?mkaply,#xpcom-reviewers

In mozilla-central before this patch, we had a boilerplate
NS_ENSURE_SUCCESS(rv, rv) after our call to GetObserverService()

This expression does not actually test whether GetObserverService() succeeded
though, since GetObserverService doesn't touch rv! When GetObserverService()
fails, it just returns nullptr. So we should be testing it for success with a
null-check.

In fact, we did used to test for success with a null-check, but that was
replaced by this ineffective NS_ENSURE_SUCCESS by accident in Bug 1276488, as
part of a large patch that was geared at adding diagnostics.

So: let's just restore the null-check. It's unlikely to be tripped, since
GetObserverService only returns nullptr at xpcom shutdown time (and this code
runs at startup); but explicitly handling that by returning a failure code
seems to be the right thing to do. And more importantly, the NS_ENSURE_SUCCESS
can be made to misfire if we happen to have a stray error code from some
earlier call remaining in 'rv'.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 9326746 [details]
Bug 1826234: Fix GetObserverService error-handling check in Preferences::InitInitialObjects. r?mkaply,#xpcom-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Startup crash for users if they happen to have a file (rather than a folder) at /etc/firefox/defaults/pref
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See end of comment 0.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This restores some error-handling code to the way it used to be, replacing a stray check for a stale error code with a more-direct check for an API-call (with it only being possible for the API to fail during shutdown).
• String changes made/needed: None
• Is Android affected?: Unknown

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d57d5bbd058
Fix GetObserverService error-handling check in Preferences::InitInitialObjects. r=xpcom-reviewers,emilio

Comment 12

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/3d57d5bbd058

Comment 13

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Hello! I tried to reproduce/ verify this today but it seems that I am not able to. From what I understand I need to create a file named /etc/firefox/defaults/pref anywhere on Ubuntu 22. Unfortunately, I am not able to create such a file because the character / is not accepted in a file name on my Ubuntu 22 machine. I tried doing that online but downloading the file will change the / character to _. Also, I tried using Unicode but I cannot reproduce the issue.
However, I reproduced another crash with Firefox 112.0RC1 by having a file inside the /etc/firefox/defaults/pref path named something.js that contains an invalid command like test. Firefox 112.0 RC will crash with this crash report. I cannot reproduce this crash with Firefox 113.0a1 (2023-04-04).
Could you please guide me on how to reproduce the crash using steps from comment 0 or this verification is enough? Thank you!

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Alexandru Trif, Desktop QA [:atrif] from comment #13)

Hello! I tried to reproduce/ verify this today but it seems that I am not able to. From what I understand I need to create a file named /etc/firefox/defaults/pref anywhere on Ubuntu 22. Unfortunately, I am not able to create such a file because the character / is not accepted in a file name on my Ubuntu 22 machine. I tried doing that online but downloading the file will change the / character to _. Also, I tried using Unicode but I cannot reproduce the issue.

Sorry, what I meant by that was: "create a file named pref inside of the /etc/firefox/defaults/ directory. (That file's fully-qualified path is /etc/firefox/defaults/pref, which means it's a file called pref that lives in /etc/firefox/defaults/.)

In particular:
(1) Ensure that /etc/firefox/defaults/ is a directory that exists. If not, create it: sudo mkdir -p /etc/firefox/defaults/
(2) Step into that directory:
cd /etc/firefox/defaults
(3) Use ls to be sure there isn't an existing directory there called pref. If there is, delete it (or move it elsewhere).
(4) Create an empty file named pref, inside that directory:
sudo touch /etc/firefox/defaults/pref

Does that help and reproduce the issue for you, in affected builds? (e.g. Firefox 112 release candidate 1)

Comment 15

[Dianna Smith [:diannaS]]

Comment on attachment 9326746 [details]
Bug 1826234: Fix GetObserverService error-handling check in Preferences::InitInitialObjects. r?mkaply,#xpcom-reviewers

Approved for 112.0rc2

Comment 16

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-release/rev/7c480935830c

Comment 17

[Alexandru Trif, Desktop Test Engineering [:atrif]]

(In reply to Daniel Holbert [:dholbert] from comment #14)

(In reply to Alexandru Trif, Desktop QA [:atrif] from comment #13)

Hello! I tried to reproduce/ verify this today but it seems that I am not able to. From what I understand I need to create a file named /etc/firefox/defaults/pref anywhere on Ubuntu 22. Unfortunately, I am not able to create such a file because the character / is not accepted in a file name on my Ubuntu 22 machine. I tried doing that online but downloading the file will change the / character to _. Also, I tried using Unicode but I cannot reproduce the issue.

Sorry, what I meant by that was: "create a file named pref inside of the /etc/firefox/defaults/ directory. (That file's fully-qualified path is /etc/firefox/defaults/pref, which means it's a file called pref that lives in /etc/firefox/defaults/.)

In particular:
(1) Ensure that /etc/firefox/defaults/ is a directory that exists. If not, create it: sudo mkdir -p /etc/firefox/defaults/
(2) Step into that directory:
cd /etc/firefox/defaults
(3) Use ls to be sure there isn't an existing directory there called pref. If there is, delete it (or move it elsewhere).
(4) Create an empty file named pref, inside that directory:
sudo touch /etc/firefox/defaults/pref

Does that help and reproduce the issue for you, in affected builds? (e.g. Firefox 112 release candidate 1)

Thank you very much for the detailed information. I was able to reproduce it this way with Firefox 112.0 RC1 (20230403163424) on Ubuntu 22.04 and Ubuntu 20.04 by creating a file named pref inside /etc/firefox/defaults/ path. This is the crash report: https://crash-stats.mozilla.org/report/index/56b03519-6f3a-45bd-bd8a-963e70230406

I can no longer reproduce the crash by following the above steps with Firefox 113.0a1 (20230404134922) and 112.0 (20230405182649) from comment 16. Thank you!

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Alexandru Trif, Desktop QA [:atrif] from comment #17)

Thank you very much for the detailed information. I was able to reproduce it this way with Firefox 112.0 RC1 (20230403163424) on Ubuntu 22.04 and Ubuntu 20.04 by creating a file named pref inside /etc/firefox/defaults/ path. This is the crash report: https://crash-stats.mozilla.org/report/index/56b03519-6f3a-45bd-bd8a-963e70230406

Interesting -- that has a different crash signature ([@ nsWindowWatcher::CalculateChromeFlagsForSystem ]) from the ones that I was getting, but it is a crash in a query of the preferences service, so it makes sense that it might've been a version of this bug. Maybe the crash location varies depending on release channel (due to us exercising some pref-dependent code sooner in one configuration vs. the other).

I'll add that crash signature to the signature list here.

Thanks for verifying the fix!