Closed Bug 1299162 Opened 3 years ago Closed 3 years ago

Preferences are applied too late to effect initial creation of user agent (and initial http request)

Categories

(Firefox for Android :: Android partner distribution, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox48 --- fixed
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

Details

Attachments

(2 files)

If a homepage is set by a distribution, when it is loaded by Fenenc, it doesn't get the user agent modified by preferences in the distribution.

This is because the prefs are loaded in browser.js after the request has already been made:

I/Kaply   (23898): InitUserAgentComponents
I/Kaply   (23898): InitUserAgentComponents - use_device is FALSE
I/Kaply   (23898): PrefsChanged - use_device has changed
I/Kaply   (23898): PrefsChanged - use_device is FALSE, clearing ID
I/Kaply   (23898): PrefsChanged - Marking user agent dirty
I/Kaply   (23898): UserAgent - User agent is dirty, rebuilding
I/Kaply   (23898): BuildUserAgent
I/Kaply   (23898): BuildUserAgent - Device ID is empty
D/GeckoKaply(23898): applyPrefs <--------------   prefs applied here.
I/Kaply   (23898): PrefsChanged - use_device has changed
I/Kaply   (23898): PrefsChanged - use_device is TRUE, setting new ID
I/Kaply   (23898): PrefsChanged - Marking user agent dirty
I/Kaply   (23898): UserAgent - User agent is dirty, rebuilding
I/Kaply   (23898): BuildUserAgent
I/Kaply   (23898): BuildUserAgent - Adding device ID
A secondary problem here is why we're getting a PrefsChanged notification for use_device before we actually apply the prefs.

That's odd as well.
To answer my first question:

#define PREF_CHANGED(p) ((pref == nullptr) || !PL_strcmp(pref, p))

There's an implicit assumption that any preferences referenced here will have default values set.

We don't, so we go through this initial case.
Attached patch Fix for problemSplinter Review
The race condition was actually created by the reading of the preferences.json file in browser.js.

So my solution was to read the JSON file in distribution.java and pass it with the Distribution:Set command that was already happening.

Then just use that instead of reading the JSON file in browser.js
Attachment #8786495 - Flags: review?(rnewman)
Attachment #8786495 - Flags: feedback?(mconnor)
Attachment #8786495 - Flags: feedback?(mconnor)
Comment on attachment 8786495 [details] [diff] [review]
Fix for problem

Review of attachment 8786495 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java
@@ +219,5 @@
>              @Override
>              public void run() {
>                  boolean distributionSet = distribution.doInit();
>                  if (distributionSet) {
> +                    String preferencesJSON = "";

Shouldn't this be either null or valid JSON?

@@ +224,5 @@
> +                    try {
> +                        final File descFile = distribution.getDistributionFile("preferences.json");
> +                        preferencesJSON = FileUtils.getFileContents(descFile);
> +                    } catch (IOException e) {
> +                        // Just send empty string

Log.

::: mobile/android/chrome/content/browser.js
@@ +7067,5 @@
>  
>    getPrefs: function dc_getPrefs() {
> +    if (this._preferencesJSON) {
> +        this.applyPrefs(this._preferencesJSON);
> +        return;

And unset this._preferencesJSON.

@@ +7074,2 @@
>      // Get the distribution directory, and bail if it doesn't exist.
>      let file = FileUtils.getDir("XREAppDist", [], false);

Do we need to keep the rest of this function at all?
Attachment #8786495 - Flags: review?(rnewman) → review+
> Do we need to keep the rest of this function at all?

It's used for OTA distributions (Distribution:Changed).

I'll fix the other stuff. Thanks!
> Shouldn't this be either null or valid JSON?

notifyObservers only takes strings, so I'm checking for the empty string on the other end.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c60fb4ea2674a1154dcd00ec0924708977ba62e4
Bug 1299162 - Pass preferences.json to browser.js to avoid disk read. r=rnewman
Backed out for Android 4.0 build bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/db6ce29aa0bb

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c60fb4ea2674a1154dcd00ec0924708977ba62e4
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=35039603&repo=mozilla-inbound

09:29:07     INFO -  /builds/slave/m-in-and-api-15-00000000000000/build/src/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java:229: error: cannot find symbol
09:29:07     INFO -                          preferencesJSON = FileUtils.getFileContents(descFile);
09:29:07     INFO -                                                     ^
09:29:07     INFO -    symbol:   method getFileContents(File)
09:29:07     INFO -    location: class FileUtils
09:29:09     INFO -  Note: Some input files use or override a deprecated API.
09:29:09     INFO -  Note: Recompile with -Xlint:deprecation for details.
Flags: needinfo?(mozilla)
API changed under the covers and I failed to rebuild. Fixing. Sorry about that.
Flags: needinfo?(mozilla)
FileUtils changed in Firefox 49 and I didn't rebuild (I was tested on release for partner).
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbe54d5199812f9e6dab95ed92738ba43b792527
Bug 1299162 - Pass preferences.json to browser.js to avoid disk read. r=rnewman
Comment on attachment 8786495 [details] [diff] [review]
Fix for problem

Approval Request Comment
[Feature/regressing bug #]: Allow partner homepage to work correctly.
[User impact if declined]: Partner doesn't work correctly.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low - distribution only.
[String/UUID change made/needed]:

I am asking for this for release because we are looking to have a one off done for a partner with this fix and bug 1299350

Note that the fist patch is for release. Beta and earlier requires the first patch plus the second patch.
Attachment #8786495 - Flags: approval-mozilla-release?
Attachment #8786495 - Flags: approval-mozilla-beta?
Attachment #8786495 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/bbe54d519981
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
> [Feature/regressing bug #]: Allow partner homepage to work correctly.
Can you share the bug number?
Is there a test plan? We are 1 week from release right now for 49.
(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> > [Feature/regressing bug #]: Allow partner homepage to work correctly.
> Can you share the bug number?

I thought that field was for providing more detail. There is no bug that caused the regression. It didn't work correctly from the beginning. 

Is there a test plan? We are 1 week from release right now for 49.

The partner is testing this as am i. We don't have an easy mechanism to test this as it requires a device with distribution customizations preloaded which can't be done on a non rooted device. 

Our plan is to deliver to the partner with a custom 48 so we are pushing for 49 because we want it to work on upgrade.
Comment on attachment 8786495 [details] [diff] [review]
Fix for problem

Seems like a partner ask, ok with me, Aurora50+
Attachment #8786495 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
> I thought that field was for providing more detail. There is no bug that caused the regression. It didn't 
> work correctly from the beginning.
The bug in which the feature has been implemented.

> Is there a test plan? We are 1 week from release right now for 49.
One more reason to come up with a solution for our QE to test ;)

> The partner is testing this as am i. We don't have an easy mechanism to test this as it requires a device 
> with distribution customizations preloaded which can't be done on a non rooted device. 
That doesn't seems a blocker to me. And clearly, having proper QE (and yes, a test plan) would have cough that in 48.
(In reply to Sylvestre Ledru [:sylvestre] from comment #19)
> > I thought that field was for providing more detail. There is no bug that caused the regression. It didn't 
> > work correctly from the beginning.
> The bug in which the feature has been implemented.

We would be going back a long time for that - bug 834681. Bug 1262591 is what the customer used to find it, but any pref that modified the user agent would have shown it.

> > Is there a test plan? We are 1 week from release right now for 49.
> One more reason to come up with a solution for our QE to test ;)

I will work to put something together, but this is a pretty serious edge case.

> > The partner is testing this as am i. We don't have an easy mechanism to test this as it requires a device 
> > with distribution customizations preloaded which can't be done on a non rooted device. 
> That doesn't seems a blocker to me. And clearly, having proper QE (and yes,
> a test plan) would have cough that in 48.

This would not have been caught in any sort of normal testing. This bugs relies on modifying the user agent via a distribution on the device, having a homepage set via the distribution, and having that homepage rely on the change to the user agent.

And as far as a blocker goes, if we put the patch in 48, we need it in 49 so that Firefox is not downgraded.

I don't like things going in this late either. I've been working hard to not have laste minute changes like this and bug 1299350 but unfortunately if we want to get distribution on Fennec, sometimes we have to take risks to work with a partner's schedule.

Barbara has already signed off on this change and is willing to take the risk for Fennec.
Comment on attachment 8786495 [details] [diff] [review]
Fix for problem

Both patches, let's put them in fennec beta 10.
Attachment #8786495 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Mike, could you make sure everything got uplifted correctly to beta? I think I got everything correct, but I had to manually add commit metadata since the patches were just raw diffs, and I might have missed something.
Flags: needinfo?(mozilla)
Looking now. Sorry about the commit data.
Looks good.
Flags: needinfo?(mozilla)
Attachment #8786495 - Flags: approval-mozilla-release? → approval-mozilla-release+
You need to log in before you can comment on or make changes to this bug.