Closed Bug 1585367 Opened 5 years ago Closed 5 years ago

[Win][Acer] The code in the search is wrongly displayed after Firefox update

Categories

(Firefox :: Distributions, defect, P1)

70 Branch
Desktop
Windows
defect
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 71
Iteration:
71.3 - Sept 30 - Oct 13
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 70+ verified
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 + verified
firefox71 --- verified

People

(Reporter: mberlinger, Assigned: standard8)

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

Prerequisites

Affected platforms

  • Windows 10x64
  • Windows 7x32

Steps to reproduce

  1. Install an older version of Beta (eg 68.0b5 32bit), but do not start it.
  2. Unzip the file acer_distribution.zip into the C:\Program Files (x86)\Mozilla Firefox directory.
  3. Launch Firefox.
  4. Update Firefox to latest version .
  5. Perform a search from the URL bar.

Expected result

  • The code in the search is pc=MOZD

Actual result

  • The code in the search is pc=MOZI

Additional notes

  • If I’ve updated from 70.0b5 the code in the search is pc=MOZD
  • This is not limited to a particular locale, I've reproduced with en-US and ar
  • On RC builds the issue isn’t reproducible

Mike can you help out here? We could get a patch into beta 12 potentially.

Flags: needinfo?(mozilla)

Setting the severity higher since we don't know if this is intended behavior or not and it involves search codes.

Severity: normal → critical

Was this working on earlier beta? We're looking at https://bugzilla.mozilla.org/show_bug.cgi?id=1574944 as a potential cause.

It went in 8 days ago.

Flags: needinfo?(mozilla)
Flags: needinfo?(maria.berlinger)

Hello Mike,

I’ve updated from 70.0b5 to 70.0b8 and I’ve got MOZD, but when I’ve updated from 70.0b8 to 70.0b9 I’ve got MOZI instead of MOZD.
We’ve tested on 69.0b9 a while ago and we’ve got MOZD, here’s the test plan: https://docs.google.com/document/d/1L-2VYZg_aZeq8wxDKn34JTaGfnOxRkBzmrdTdkltGd4/edit#heading=h.fynafjbd5n14

Anyway another stranger scenario that I’ve met was if I’ve updated from 70.0b5 to 70.0b9 but I didn’t open the 70.0b5 and i’ve updated to 70.0b9 I’ve got MOZI and if I’ve opened the 70.0b5 and then updated to 70.0b9 I’ve got MOZD.
I’ve performed the update with pave over since using update channels will end up to beta 11.

Flags: needinfo?(maria.berlinger)

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression

Maria, is it possible that you could get a log of this failure for me? I can't currently reproduce, but I have some ideas.

What I need: When you create the profile, set browser.search.log to true. Then, get the log from the browser console when you start the latest version of FF which shows the wrong code.

Flags: needinfo?(maria.berlinger)

A few other questions as well:

  • what's the browser.search.region preference value before & after upgrade?
  • Are you testing with a VPN?
  • What timezone is your computer set to?

My suspicion is that this is related to the region check kicking-in after upgrade. However, that shouldn't happen, unless it didn't get set on first run, or there's some sort of issue with saving it or the value being removed.

I can currently reproduce this with a fresh profile on 70b9, but not with an upgrade. I think I know what the issue is for the new profile, but I'd like to understand why this is triggering on upgrade as, in theory, it shouldn't generally be happening.

The patch I've attached should fix the issue that I can see. I think it'll probably fix the main issue as well.

I've done a try push that is based on the latest beta:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc290eca1f61fc096644df40686b5c5b1ed4a827

Maria, if you could try testing with that as well, that would be useful.

Assignee: mozilla → standard8
Status: NEW → ASSIGNED
Attached file console-export-2019-10-4_12-15-25.txt (obsolete) —

Hi Mark,

Maria is in PTO so I'll try to fill the details for her.

(In reply to Mark Banner (:standard8) from comment #6)

What I need: When you create the profile, set browser.search.log to true. Then, get the log from the browser console when you start the latest version of FF which shows the wrong code.

  • Attaching the log file.

A few other questions as well:

  • what's the browser.search.region preference value before & after upgrade?
  • The browser.search.region has the value of RO
  • Are you testing with a VPN?
  • No VPN was used for this testing.
  • What timezone is your computer set to?
  • UTC +02:00

I can currently reproduce this with a fresh profile on 70b9, but not with an upgrade. I think I know what the issue is for the new profile, but I'd like to understand why this is triggering on upgrade as, in theory, it shouldn't generally be happening.

  • I have managed to reproduce this by both triggering the update or upgrade and by just installing 70.0b9 directly as well via zip and installer (without performing an update or upgrade) inside the C:\Program Files\Mozilla Firefox path

  • I can't reproduce this on the provided try build ( from comment 9) with a note that I can't perform a pave over since only the zip format seems to work.

Flags: needinfo?(maria.berlinger)
Attachment #9098795 - Attachment is obsolete: true
Attached file console-export.txt

Adding the correct log file.

Thank you for the additional info. That makes me pretty sure this is the same issue as what I found, so I'm going to go ahead and get this landed.

Iteration: --- → 71.3 - Sept 30 - Oct 13
Points: --- → 3
Priority: -- → P1
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d31bf4ac2cb6
Ensure that the search service doesn't load built-in engines over the top of distribution engines. r=mikedeboer
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71

Comment on attachment 9098396 [details]
Bug 1585367 - Ensure that the search service doesn't load built-in engines over the top of distribution engines.

Beta/Release Uplift Approval Request

  • User impact if declined: Probably no significant impact on users. However, a user with a distribution pack can get into the situation where we're providing the wrong codes for the search engine (the Firefox ones, rather than the distribution ones).
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See 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): Small change that only affects preventing distribution engines from having their settings overridden.
  • String changes made/needed: None
Attachment #9098396 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Hi, Can someone please reupload the files we need to retest this issue, the acer distribution file returns a 404.

Flags: needinfo?(maria.berlinger)

Sorry about that. Files are back.

Flags: needinfo?(maria.berlinger)

Rares or Maria, if you can verify tomorrow that will be in time for the beta 14 build :) Thanks!

Flags: needinfo?(rares.doghi)
Flags: needinfo?(maria.berlinger)

Hello,

This is verified fixed using Firefox 71.0a1 (20191008214557) on Windows 10x64 and Windows 7x32.

Flags: needinfo?(rares.doghi)
Flags: needinfo?(maria.berlinger)

Comment on attachment 9098396 [details]
Bug 1585367 - Ensure that the search service doesn't load built-in engines over the top of distribution engines.

Fix for search issue, verified in nightly, OK for beta 14 uplift.

Attachment #9098396 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

This is also verified fixed using Firefox 70.0b14 on Windows 10x64 and Windows 7x32.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

Comment on attachment 9098396 [details]
Bug 1585367 - Ensure that the search service doesn't load built-in engines over the top of distribution engines.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Needed for uplift of bug 1574944
  • User impact if declined: Little impact.
  • Fix Landed on Version: 70
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Includes automated tests for this scenario
  • String or UUID changes made by this patch: None
Attachment #9098396 - Flags: approval-mozilla-esr68?
Attached patch Possible ESR Patch (obsolete) — Splinter Review

Mike, I've been trying this patch out in combination with the patch I've just attached to bug 1574944.

The strange thing is, the newly added test toolkit/components/search/tests/xpcshell/test_maybereloadengine_update.js doesn't fail with the SearchService.jsm part commented out (as per this version of the patch).

This makes me concerned if a) there's some other patch we're missing, b) if this isn't an issue on ESR even with bug 1574944 fixed, c) something else.

What I haven't had time to do yet, is to run both patches with the distribution set up as mentioned in this bug - when I ran them before, I copied the extracted distribution directory into objdir-ff/dist/Nightly.app/Contents/Resources and tested like that.

If that still works with bug 1574944 applied, and without this applied, then maybe for some reason that is ok to ship like that... just strange and I wonder what changed.

Flags: needinfo?(mdeboer)
Attachment #9100991 - Attachment description: Possible Beta Patch → Possible ESR Patch

Comment on attachment 9098396 [details]
Bug 1585367 - Ensure that the search service doesn't load built-in engines over the top of distribution engines.

Moving request to new patch.

Flags: needinfo?(mdeboer)
Attachment #9098396 - Flags: approval-mozilla-esr68?
Attached patch ESR PatchSplinter Review

Please see comment 1585367.

Manual testing of the patch shows that it correctly fixes the issue here on ESR after bug 1574944 is applied as well.

The automatic test doesn't actually detect the issue for ESR. I suspect this is something to do with changed setup requirements between release and ESR, but I haven't debugged what yet. I don't think this is enough to delay shipping though.

Attachment #9100991 - Attachment is obsolete: true
Attachment #9101580 - Flags: approval-mozilla-esr68?
Comment on attachment 9101580 [details] [diff] [review]
ESR Patch

Fixes a regression from bug 1574944. Approved for 68.2esr.
Attachment #9101580 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Flags: qe-verify+

I just pushed a test-only fix to mozilla-release as the original uplift (comment 21) missed the fact that the test added to a differently named xpcshell ini file that isn't present in 70.

https://hg.mozilla.org/releases/mozilla-release/rev/cfcae5cd47646c9b9f58d25d228987ec4f7553d4

No bad consequences, just the test wasn't being run.

This is also verified fixed using Firefox 68.2.0esr on Windows 10x64 and Windows 7x32.

Flags: qe-verify+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.