visibleDefaultEngines ignored error on beta with browser.search.log

VERIFIED FIXED in Firefox -esr60

Status

()

defect
P1
major
VERIFIED FIXED
4 months ago
3 months ago

People

(Reporter: mkaply, Assigned: mkaply)

Tracking

(Regression)

unspecified
Firefox 67
Points:
---

Firefox Tracking Flags

(firefox-esr6067+ fixed, firefox65 wontfix, firefox66blocking verified, firefox67+ verified)

Details

Attachments

(3 attachments)

Because we're translating the new Google engines after setting up the jarNames list, the engines aren't found in jarNames.

So absearch is getting ignored on beta right now.

Simple fix is to read regionOverrides so that the new engines are in the list.

I'll investigate why this wasn't found by a test as well.

[Tracking Requested - why for this release]: This is needed or we won't read absearch properly.

This will need a different patch for beta due to refactoring of search.

Attachment #9048659 - Attachment description: Bug 1532761 - Add regionOverrides engine to jarNames. → Bug 1532761 - Add regionOverrides engine to jarNames. (beta patch)

Is anything happening here for 66? Or can this be aimed at 67?

Flags: needinfo?(mozilla)

Trying to find a reviewer. We really need this for 66. There's a possibility of something requiring absearch. I'm shaking all the trees.

Flags: needinfo?(mozilla)
Attachment #9048659 - Attachment description: Bug 1532761 - Add regionOverrides engine to jarNames. (beta patch) → Bug 1532761 - Add regionOverrides engine to jarNames.
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/c7d8b2c0ada2
Add regionOverrides engine to jarNames. r=daleharvey,adw

Comment on attachment 9048659 [details]
Bug 1532761 - Add regionOverrides engine to jarNames.

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1510296
  • User impact if declined: Changes from absearch won't be used by Firefox.
  • 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: _parseListJSON: ignoring visibleDefaultEngines value because google-b-1-d is not in the jar engines we have found

does not appear in the console.

  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This was already broken, and this adds a very simple additional check.

I know you are going to ask about automated tests for this, and I'm looking into this, but we need to get this in to make sure absearch is working. Sorry for the lateness, found only in testing and had some review issues.

  • String changes made/needed:
Attachment #9048659 - Attachment description: Bug 1532761 - Add regionOverrides engine to jarNames. → Bug 1532761 - Add regionOverrides engine to jarNames. (beta patch)
Attachment #9048659 - Flags: approval-mozilla-beta?
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

From discussion with Mike this morning, this is a release blocker.

If this was regressed by bug 1510296, does this issue also affect esr60?

Flags: needinfo?(mozilla)

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #10)

If this was regressed by bug 1510296, does this issue also affect esr60?

Probably, but I was focused on release. I'm not as concerned their.

I'm still investigating how we could have caught this with a test. The problem is that this is specifically an interaction with the absearch server which we can't check via automated tests (and the error message only shows when we are in debug)

Flags: needinfo?(mozilla)

Does this also affect Fennec? We should likely also test there.

Does this also affect Fennec? We should likely also test there.

The search code is the same between desktop and Fennec, so it would be fixed there as well.

The browser.search.log thing should work for Fennec as well, but you'd have to look at an adb log to see it.

From the email thread, it sounds like we can punt on this until 67. Tracking for ESR60 accordingly.

Attachment #9048659 - Flags: approval-mozilla-release+
Attachment #9048659 - Flags: approval-mozilla-beta?
Attachment #9048659 - Flags: approval-mozilla-beta-
Flags: qe-verify+

Reproduction scenario:

Go to about:config, flip browser.search.log to true

Restart the browser

Bring up the JS console. Search for visibleDefaultEngines

You'll see this error:

_parseListJSON: ignoring visibleDefaultEngines value because google-b-1-d is not in the jar engines we have found

After the fix, that error will not be there.

QA Whiteboard: [qa-triaged]
Severity: normal → major

I reproduced this issue using Fx 67.0a1 (2019-03-05), on Windows 10.
I can confirm this issue is fixed, I verified using Fx 67.0a1 (2019-03-14)and Fx 66.0-build2, on Windows 10 x64, macOS 10.13 and Ubuntu 16.04 LTS

Status: RESOLVED → VERIFIED
Flags: qe-verify+

Hello, I can confirm that this issue cannot be reproduced on Fennec on the latest Nightly 67.0a1 with Samsung Galaxy J7(Android 6.0).

Can you also request uplift to ESR please?

Flags: needinfo?(mozilla)
Posted patch ESR patchSplinter Review

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes ability to get server search engines
  • User impact if declined: Changes from absearch won't be used by Firefox.
  • Fix Landed on Version: 67
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Low touch, easy to test.
  • String or UUID changes made by this patch: None
Flags: needinfo?(mozilla)
Attachment #9057398 - Flags: approval-mozilla-esr60?
Comment on attachment 9057398 [details] [diff] [review]
ESR patch

Search code fix, verified in nightly, ok for ESR uplift.
Attachment #9057398 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Attachment #9048659 - Attachment description: Bug 1532761 - Add regionOverrides engine to jarNames. (beta patch) → Bug 1532761 - Add regionOverrides engine to jarNames.
You need to log in before you can comment on or make changes to this bug.