Closed Bug 1371367 Opened 7 years ago Closed 6 years ago

Installing an en-US lang pack on an en-US Firefox produces invalid codes

Categories

(Firefox :: Search, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 60
Tracking Status
firefox59 + verified
firefox60 --- verified

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(1 file)

If you somehow end up with an English lang pack on an English builds, the yhs- values for purpose don't work correctly.

They all show up as yhs-invalid.

You have to delete your search lz4 file to see this immediately.
So the problem is here:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#2256

This code works fine when general.useragent.locale is set for a language other than the default language, but when you install a language pack that is the same as the default language, it fails because there is no user value. So we return false for _isDefault

And incidentally, this code doesn't kick in:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#2253

Because the load paths have [profile] in them:

jar:[profile]/extensions/langpack-en-US@firefox.mozilla.org.xpi!browser/yahoo.xml

Is there something better we can do here?
Flags: needinfo?(florian)
(In reply to Mike Kaply [:mkaply] from comment #1)

> Is there something better we can do here?

Can we prevent loading langpacks for the default language?
Flags: needinfo?(florian)
See Also: → 1423811
Given what Pike said in bug 1423811, it seems the answer to comment 2 is 'no'.

So I think we'll need to fix the _isDefault getter in the search service. The current idea I have for a way to do this is to cache the path to the list.json file we parse when initializing the search service, and compare the path of engines with the path of list.json. Then a default engine becomes an engine in the same folder.
This is also causing reporting problems - we report Google in langpacks as "other-Google" even though technically the langpack has taken over the set of default engines.

I think we should investigate whether the engines in the langpacks should be marked as default engines (at least until we remove engines from langpacks).
What about simply removing this check?

if (Services.locale.defaultLocale == Services.locale.getRequestedLocale() &&

Extensions can't override resource://search-plugins

Which of the checks are costly?

I'd really like to get something in for 59 on this. Come to find out this is causing serious problems with our reporting (and none of these searches have codes).
Another thought. If _isDefault is expensive (per the comment in the code), why don't we memoize the getter? Whether or not an engine is default will not change while the browser is running.

We would have to fix a couple tests that thing _isDefault is dynamic, but this shouldn't be a problem in practice, should it?
(In reply to Mike Kaply [:mkaply] from comment #5)
> What about simply removing this check?
> 
> if (Services.locale.defaultLocale == Services.locale.getRequestedLocale() &&
> 
> Extensions can't override resource://search-plugins

At the time this was written, add-ons could still register chrome (and so resource prefixes).

It would probably be OK remove this check now.


> Which of the checks are costly?

None of this code seems really fast. How do you feel about the idea from comment 3? It seems that would simplify this code a lot.

Another completely different idea: could we just use this._readOnly to decide if we include the partner codes?
Read-only engines are engines found in omni.ja, in distributions, and in .xpi files in the user profile (that used to include engines from add-ons, but doesn't include engines from WebExtensions).

(In reply to Mike Kaply [:mkaply] from comment #6)
> Another thought. If _isDefault is expensive (per the comment in the code),
> why don't we memoize the getter? Whether or not an engine is default will
> not change while the browser is running.

Sounds like a good idea. We could also just compute this value when initializing the engine objects... it's actually what lead me to the _readOnly idea, which seems to be exactly that.
> None of this code seems really fast. How do you feel about the idea from comment 3? It seems that would simplify this code a lot.

I was looking at that, but I wasn't sure how to turn resource://search-plugins into the locale path. When I called getSubstution on the resource protocol handler, it took me to the generic chrome version, not the version that included the locale path.

> Sounds like a good idea. We could also just compute this value when initializing the engine objects... it's actually what lead me to the _readOnly idea, which seems to be exactly that.

Yes, it does. The reason I want to keep _isDefault, though, is because I want these other-Google to be properly reported as Google. I'll take a look.

I've opened a separate bug which is that we should probably just get these engines out of the langpacks.
This is the simplest fix that I could try to get into 59.

I also removed the second section after running all tests and seeing no failures.

This patch shouldn't affect performance in any common cases because we will bail out in the first two cases for most search engines.
(In reply to Mike Kaply [:mkaply] from comment #10)
> This is the simplest fix that I could try to get into 59.
> 
> I also removed the second section after running all tests and seeing no
> failures.

Do you have a try link for this? I suspect this was about the non-search xpcshell tests that somehow use the search service: https://searchfox.org/mozilla-central/search?q=Services.search&case=false&regexp=false&path=test_
> Do you have a try link for this? I suspect this was about the non-search xpcshell tests that somehow use the search service: https://searchfox.org/mozilla-central/search?q=Services.search&case=false&regexp=false&path=test_

I ran search tests. I've started a try build from MozReview.
Would be great if you could land that asap.
> Would be great if you could land that asap.

Working on it now. Try build was completely busted on cert stuff last night.
So I seem some failures, but they don't seem related to this change.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29ee5e14253b8b7291ea6ff1fa8f3dc8246f30cd&selectedJob=162186629

I explicitly checked docshell/test/unit/test_nsDefaultURIFixup_info.js and it still fails without my change.

So I think we're good here.
Comment on attachment 8950784 [details]
Bug 1371367 - Treat langpack search engines as default.

https://reviewboard.mozilla.org/r/220028/#review226442

Your xpcshell failures have this message: JavaScript error: jar:file:///Z:/task_1518635046/build/application/firefox/omni.ja!/components/nsSearchService.js, line 2179: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsISubstitutingProtocolHandler.resolveURI]
Attachment #8950784 - Flags: review?(florian) → review-
Which log specifically? I didn't see that failure (I guess I missed it).

I'll remove the second part of the patch.
Comment on attachment 8950784 [details]
Bug 1371367 - Treat langpack search engines as default.

https://reviewboard.mozilla.org/r/220028/#review226456

If you keep that test, it should really be conditioned on gEnvironment.get("XPCSHELL_TEST_PROFILE_DIR")

But given only 2 tests are failing, I think it would be cleaner to just fix them (and I emailed you a simple fix).
Attachment #8950784 - Flags: review?(florian) → review-
Comment on attachment 8950784 [details]
Bug 1371367 - Treat langpack search engines as default.

https://reviewboard.mozilla.org/r/220028/#review226516
Attachment #8950784 - Flags: review?(florian) → review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/3fcca926d1b5
Treat langpack search engines as default. r=florian
https://hg.mozilla.org/mozilla-central/rev/3fcca926d1b5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment on attachment 8950784 [details]
Bug 1371367 - Treat langpack search engines as default.

Approval Request Comment
[Feature/Bug causing the regression]: Search engines in langpacks are not marked default so they are reported incorrectly and we don't send codes
[User impact if declined]: None. Had to do with search reporting.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: See bug.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Removed unneeded code
[String changes made/needed]:

I'm marking this as release as well. If a dot release for 58 goes out, I would like to have this in it.

This change is having a material impact on our search.
Attachment #8950784 - Flags: approval-mozilla-release?
Attachment #8950784 - Flags: approval-mozilla-beta?
Comment on attachment 8950784 [details]
Bug 1371367 - Treat langpack search engines as default.

Fix for search reporting that may have regressed in 57. 
Let's uplift for 59 beta 11. 

Ritu, FYI, you may want to consider this for 58 especially if we have any other drivers.
Flags: needinfo?(rkothari)
Attachment #8950784 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Mike, can you say a bit more about how QE can test/verify the fix in beta?
Flags: qe-verify+
Flags: needinfo?(mozilla)
> Mike, can you say a bit more about how QE can test/verify the fix in beta?

Sure

The easiest way to see the problem is to install a language pack of the same language on top of an existing Firefox. So en-US langpack on to en-US firefox. Then do a google search. You'll notice that the Google search does not have any codes.

You can also go to about:telemetry and type "search" in the upper right box. You'll see that Google is misidentified as "other-Google"

After this fix, you will see codes when you search and in telemetry, the search engine will be properly identified as "Google"
Flags: needinfo?(mozilla)
I was able to reproduce the issue using Firefox 58 under Win 10 and Mac OS X 10.13, but couldn't see it under Ubuntu 14.04 32-bit.

The issue is now fixed on Firefox 59 beta 11 and latest Nightly 60.0a1.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Comment on attachment 8950784 [details]
Bug 1371367 - Treat langpack search engines as default.

So far, no 58 dot release is planned, and we're a week and a half away from 59 release. I don't think this will make it to 58.
Attachment #8950784 - Flags: approval-mozilla-release? → approval-mozilla-release-
Dropping the needinfo to Ritu based on comment 31.
Flags: needinfo?(rkothari)
You need to log in before you can comment on or make changes to this bug.