1438878 - After prefs parser rewrite, the Firefox Privacy Notice url keeps opening up in a background tab

Status: RESOLVED DUPLICATE of bug 107264

(Core :: Preferences: Backend, defect)

Description

[Jukka Jylänki]

Recently, different people have begun reporting that their automated test harnesses are now opening up the URL https://www.mozilla.org/en-US/privacy/firefox/ whenever they launch Firefox for the test suite. This is occurring in Emscripten test suite, emrun Firefox driver (https://github.com/juj/emrun), and Unity has mentioned they are seeing the same in their test automation.

First I thought this was due to some new feature that we have added, and was looking for a newly added pref that we could insert into programmatically created Firefox profiles for the test harness (https://github.com/juj/emrun/blob/master/emrun.py#L206), but I could not find anything to control this.

Finally, running mozregression to see when the Privacy Notice started to appear led to this bisection window:

 9:48.44 INFO: Narrowed inbound regression window from [96796753, e8b798a5] (3 builds) to [fc36a210, e8b798a5] (2 builds) (~1 steps left)
 9:48.44 INFO: No more inbound revisions, bisection finished.
 9:48.44 INFO: Last good revision: fc36a210edd7e44ecb0e7e0c60ea3226d438ab17
 9:48.45 INFO: First bad revision: e8b798a5205ad27d067b91210efb46df7ddab45d
 9:48.45 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fc36a210edd7e44ecb0e7e0c60ea3226d438ab17&tochange=e8b798a5205ad27d067b91210efb46df7ddab45d

which contains

[e-mail]
Wed Jan 31 22:47:35 2018 +0000

e8b798a5205a	Nicholas Nethercote — Bug 1423840 - Rewrite the prefs parser. r=glandium,Manishearth

e500592d3551	Nicholas Nethercote — Bug 1423840 - Remove extraneous semicolons in all.js. r=glandium

suggesting that this was not a feature addition, but that something changed behaviorally in a refactoring.

STR:

1. wget https://raw.githubusercontent.com/juj/emrun/master/emrun.py
2. python emrun.py /path/to/firefox.exe --safe_firefox_profile http://www.google.com

Observed: Firefox opens up with two tabs, http://www.google.com and https://www.mozilla.org/en-US/privacy/firefox/ opened.

Expected: Only http://www.google.com should show up.

Before the rewrite, only the given URL would open up.

I wonder if there's some subtle bug that occurred in the refactoring in disguise, or was that an intentional change to happen while the refactoring took place?

Comment 1

[Jukka Jylänki]

It looks like the pref to opt out from the extra Privacy Notice tab from opening up is

> user_pref("datareporting.policy.dataSubmissionEnabled", false);

However the issue in emrun is that it has the following lines in the prefs.js before it

> user_pref("app.update.lastUpdateTime.addon-background-update-timer", 4102437600);
> user_pref("app.update.lastUpdateTime.background-update-timer", 4102437600);
> user_pref("app.update.lastUpdateTime.blocklist-background-update-timer", 4102437600);
> user_pref("app.update.lastUpdateTime.browser-cleanup-thumbnails", 4102437600);
> user_pref("app.update.lastUpdateTime.experiments-update-timer", 4102437600);
> user_pref("app.update.lastUpdateTime.search-engine-update-timer", 4102437600);
> user_pref("app.update.lastUpdateTime.xpi-signature-verification", 4102437600);
> user_pref("extensions.getAddons.cache.lastUpdate", 4102437600);
> user_pref("media.gmp-eme-adobe.lastUpdate", 4102437600);
> user_pref("media.gmp-gmpopenh264.lastUpdate", 4102437600);
> user_pref("datareporting.healthreport.nextDataSubmissionTime", 4102437600439);

when the new parser reaches any of these lines, it seems to abort reading any of the subsequent prefs in the file. That is, a prefs.js file with e.g.

> user_pref("app.update.lastUpdateTime.addon-background-update-timer", 4102437600);
> user_pref("datareporting.policy.dataSubmissionEnabled", false);

A prefs.js with only the second line present, and not the first one, will avoid the Privacy Notice tab from showing up. I think the old parser accepted those lines, since we did have prefs after these lines and applying them did work. I wonder if this is a regression with the new parsing code that somehow the above lines cause it to trip up?

Comment 2

[Nicholas Nethercote [inactive]]

Thanks for the sleuthing!

Integer prefs are int32_t. These values are larger than 2^31 and so overflow. (They're mostly less than 2^32, except for the last one which is ~1000 times larger

The old prefs parser would silently let that pass, resulting in a bogus value, while the new parser throws an error.

rstrong: do you know what units these timestamps are using? 4102437600 seconds is 130 years, so it doesn't look like these are measuring time since 1970, which some other prefs do.

And the last one is 1000x bigger, so presumably it is measuring milliseconds.

Comment 3

[Nicholas Nethercote [inactive]]

> > user_pref("app.update.lastUpdateTime.addon-background-update-timer", 4102437600);
> > user_pref("app.update.lastUpdateTime.background-update-timer", 4102437600);
> > user_pref("app.update.lastUpdateTime.blocklist-background-update-timer", 4102437600);
> > user_pref("app.update.lastUpdateTime.browser-cleanup-thumbnails", 4102437600);
> > user_pref("app.update.lastUpdateTime.experiments-update-timer", 4102437600);
> > user_pref("app.update.lastUpdateTime.search-engine-update-timer", 4102437600);
> > user_pref("app.update.lastUpdateTime.xpi-signature-verification", 4102437600);
> > user_pref("extensions.getAddons.cache.lastUpdate", 4102437600);
> > user_pref("media.gmp-eme-adobe.lastUpdate", 4102437600);
> > user_pref("media.gmp-gmpopenh264.lastUpdate", 4102437600);
> > user_pref("datareporting.healthreport.nextDataSubmissionTime", 4102437600439);

Thinking some more... this is really strange. As I said in comment 2, libpref stores integer values as int32_t, in PrefValue::mIntVal. When saving the prefs.js file, we convert the integer to a string here:

https://searchfox.org/mozilla-central/rev/cac28623a15ace458a8f4526e107a71db1519daf/modules/libpref/Preferences.cpp#677

It uses nsTSubstring::AppendInt(), which is defined here:

https://searchfox.org/mozilla-central/rev/cac28623a15ace458a8f4526e107a71db1519daf/xpcom/string/nsTSubstring.h#420-423

I don't know how printing an int32_t can result in a string like "4102437600" or "4102437600439".

Comment 4

[Nicholas Nethercote [inactive]]

Another odd thing: in prefs.js in the default profile on my Mac, all these update prefs have values like 1518775931, which is 48.1 years, i.e. it's a timestamp counting seconds since 1970.

Comment 5

[Nicholas Nethercote [inactive]]

One other thing to note: the prefs parser outputs parse error messages to the browser console, if you want to double-check that they are occurring and why.

Comment 6

[Jukka Jylänki]

> I don't know how printing an int32_t can result in a string like
> "4102437600" or "4102437600439".

These pref settings originally came to emrun by going to about:support in a new Firefox profile and copy-pasting the changed prefs that were listed in there. This was done several years ago, perhaps it has been some old bug that has since been fixed. In any case, if the code flow looks good to you, I can definitely regard the emrun initialization side as suspect and adhere to a user_pref("string", int32_t); type format requirement, and throw away these erroneous ones.

> The old prefs parser would silently let that pass, resulting in a bogus value, while the new parser throws an error.

It read like this is the "regression" and in fact an intentional change, so looks like everything is in order here. Btw, is it possible to do a

> try { user_pref("foo", 1231); } catch(e) {}

in order to make any possible errors be suppressed? I wonder if a pref that gets deleted would throw an error, causing all subsequent lines suddenly to start to be ignored, or if a type of a pref might change from a string to an int, perhaps that could then start throwing errors - it would be good to have some kind of way to init a prepopulated prefs.js in a way that was resilient to errors in a future proof way, and each line would either be processed or ignored, but no danger of aborting in the middle?

Comment 7

[Nicholas Nethercote [inactive]]

> These pref settings originally came to emrun by going to about:support in a
> new Firefox profile and copy-pasting the changed prefs that were listed in
> there. This was done several years ago, perhaps it has been some old bug
> that has since been fixed.

I just looked at the equivalent code from March 2015 and it still looks printing a value like that should be impossible...


> It read like this is the "regression" and in fact an intentional change, so
> looks like everything is in order here. Btw, is it possible to do a
> 
> > try { user_pref("foo", 1231); } catch(e) {}
> 
> in order to make any possible errors be suppressed?

The old parser doesn't do that so the new one also doesn't. But this is not the first time the idea of error recovery has come up, so I will look into implementing it.

Comment 8

[Nicholas Nethercote [inactive]]

In case it's not clear: I won't add `try` blocks; I will just make the parser automatically do error recovery -- any time it hits a syntax error it would just skip forward to the next ';' and then start parsing again.

Comment 9

[Nicholas Nethercote [inactive]]

> But this is not the first time the idea of error recovery has come up,
> so I will look into implementing it.

I'm doing this work in bug 107264.

Comment 10

[Nicholas Nethercote [inactive]]

> I'm doing this work in bug 107264.

That has landed.