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)
Firefox
Search
Tracking
()
VERIFIED
FIXED
Firefox 60
People
(Reporter: mkaply, Assigned: mkaply)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
florian
:
review+
lizzard
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-release-
|
Details |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
(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)
Comment 3•6 years ago
|
||
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.
Assignee | ||
Comment 4•6 years ago
|
||
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).
Assignee | ||
Comment 5•6 years ago
|
||
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).
Assignee | ||
Comment 6•6 years ago
|
||
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?
Comment 7•6 years ago
|
||
(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.
Assignee | ||
Comment 8•6 years ago
|
||
> 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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
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.
Comment 11•6 years ago
|
||
(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®exp=false&path=test_
Assignee | ||
Comment 12•6 years ago
|
||
> 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®exp=false&path=test_
I ran search tests. I've started a try build from MozReview.
status-firefox59:
--- → affected
tracking-firefox59:
--- → +
Comment 13•6 years ago
|
||
Would be great if you could land that asap.
Assignee | ||
Comment 14•6 years ago
|
||
> Would be great if you could land that asap.
Working on it now. Try build was completely busted on cert stuff last night.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•6 years ago
|
||
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 17•6 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 18•6 years ago
|
||
Which log specifically? I didn't see that failure (I guess I missed it). I'll remove the second part of the patch.
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 22•6 years ago
|
||
mozreview-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+
Comment 23•6 years ago
|
||
Pushed by mozilla@kaply.com: https://hg.mozilla.org/integration/autoland/rev/3fcca926d1b5 Treat langpack search engines as default. r=florian
Comment 24•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3fcca926d1b5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Assignee | ||
Comment 25•6 years ago
|
||
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 26•6 years ago
|
||
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+
Comment 27•6 years ago
|
||
Mike, can you say a bit more about how QE can test/verify the fix in beta?
Flags: qe-verify+
Flags: needinfo?(mozilla)
Assignee | ||
Comment 28•6 years ago
|
||
> 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)
Comment 29•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/7315e8d7ce9c
Comment 30•6 years ago
|
||
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 31•6 years ago
|
||
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-
Comment 32•6 years ago
|
||
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.
Description
•