645970 - Search engines in fennec language packs don't work, search service doesn't unpack jar:jar:

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Cristian Nicolae (:xti)]

Build id : Mozilla/5.0 (Android;Linux armv7l;rv:2.2a1pre)Gecko/20110328
Firefox/4.2a1pre Fennec /4.1a1pre
Device: Sony Ericsson Xperia X10
OS: Android 2.1 update 1

Steps to reproduce:
1. Open Fennec App
2. Tap URL Bar
3. Tap on Magnifying Glass button
4. Install the arabic plugin by clicking on the first .xpi link (fennec-5.1a1pre.ar.langpack.xpi): http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/latest-mozilla-central-linux-l10n/ 
6. Repeat step 2 and 3.

Expected result:
After step 3 and 6 there will be displayed the list of default installed search engines.

Actual result:
Unable to tap on Magnifying Glass button because is grayed out. This is due to the fact that the search engines are not translated into arabic.

Comment 1

[Axel Hecht [:Pike]]

I reproduced this on the mac desktop build, with the arab and the german langpack.

This has nothing to do with the langpack itself, the list.txt and the xml files are in there. Also, there's an error message about chrome/ar.manifest not being found, but I fixed that locally and it doesn't have an effect.

I get tons of "Invalid chrome URI: /" in the error console.

If I turn on search logging, restart, I get

_findJAREngines: failed to get chromeFile for chrome://browser/locale/searchplugins/: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: file:///Users/axelhecht/Downloads/Fennec.app/Contents/MacOS/components/nsSearchService.js :: <TOP_LEVEL> :: line 2822"  data: no]

That's http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#2873.

Why? No idea. Gavin, do you?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Cristian - Have you tested other locales? I am wondering if this happens everywhere, or just in Arabic.

Comment 3

[Axel Hecht [:Pike]]

I tested with the german langpack, too.

chrome://browser/locale/searchplugins/ shows the german content allright then.

Comment 4

[Ioana Chiorean]

For the localizations present in the multilanguage build in Preferences, the search engines work and also are personalized (each language has the one mostly used in those country)

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Per IRC this happen at least on Arabic, German and French, and this probably happens for each langs :(

Comment 6

[Axel Hecht [:Pike]]

This is likely a regression from not unpacking .xpi's anymore when installing add-ons.

Thus we end up with double jar urls, and the search service unpacks only once.

Moving to Firefox:Search, as there doesn't seem to be a toolkit search component.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Hmm, I want to block fennec 4.0.1 on this bug, but can't set the flag here

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Oh, I just read comment 4 and I don't think we should block Fennec 4.0.1 on this. We'll take the patch when it's ready though.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 522651 [details] [diff] [review]
Patch

I wish we had tests :(

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Actually I suppose it would be relatively easy to test this with an xpcshell test.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Vivien, can you try to make a test? I'd like to get this in FF5 for the l10n changes we have planned.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch + test

Same patch with a simple test.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 525703 [details] [diff] [review]
Patch + test

Oups, I've tried :gavin as the reviewer.... Sorry for spamming.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch + test (v0.2)

From irc comments, the patch with more files (and a fix)

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 525716 [details] [diff] [review]
Patch + test (v0.2)

Looks good, but this test fails in Firefox for a couple of reasons:
- the test also needs to set gPrefService.setBoolPref("browser.search.loadFromJars", true);
- the Firefox searchengine defaults get loaded, so there end up being 7 engines instead of 2 (the duplicate google.xml in the JAR is not loaded)

You can address the latter by just checking that the JAR engine loaded (using e.g. searchService.getEngineByName()). Probably also would be good to remove the duplicate google.xml in the test JAR, rename the JAR to searchTest.jar rather than ar.jar, and rename the engine inside it to something unique (e.g. "Test for bug 645970" rather than "wikipedia-ar").

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch + test (v0.3)

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 525726 [details] [diff] [review]
Patch + test (v0.3)

>diff --git a/toolkit/components/search/tests/xpcshell/test_645970.js b/toolkit/components/search/tests/xpcshell/test_645970.js

>+function run_test() {

>+  // Assign our new registered chrome uri as the Naboo's planet

I do not understand this comment - remove it? :)

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #17)
> Comment on attachment 525726 [details] [diff] [review]
> Patch + test (v0.3)
> 
> >diff --git a/toolkit/components/search/tests/xpcshell/test_645970.js b/toolkit/components/search/tests/xpcshell/test_645970.js
> 
> >+function run_test() {
> 
> >+  // Assign our new registered chrome uri as the Naboo's planet
> 
> I do not understand this comment - remove it? :)

http://www.starwars.com/databank/character/jarjarbinks/ :)

But I can remove it if needed :)

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mozilla-central/rev/15b8b01a965e

I have removed the comment, poor jarjar!

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 525726 [details] [diff] [review]
Patch + test (v0.3)

>diff --git a/toolkit/components/search/tests/Makefile.in b/toolkit/components/search/tests/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/search/tests/Makefile.in
>@@ -0,0 +1,49 @@
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is mozilla.org code.
>+#
>+# The Initial Developer of the Original Code is
>+# Mozilla.org.

"Mozilla.org" doesn't exist. This should be "the Mozilla Foundation".

>+# Portions created by the Initial Developer are Copyright (C) 2005

2005?

>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/search/tests/xpcshell/head_search.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/search/tests/xpcshell/test_645970.js
>@@ -0,0 +1,63 @@
>+ * The Initial Developer of the Original Code is POTI Inc.
>+ * Portions created by the Initial Developer are Copyright (C) 2007
>+ * the Initial Developer. All Rights Reserved.

Really?

Comment 22

[Kevin Brosnan [Ex-Mozilla]]

Verified using https://hg.mozilla.org/mozilla-central/rev/ec809c159ad2 Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110415 Firefox/6.0a1 Fennec/6.0a1

Comment 23

[Kevin Brosnan]

This affects Firefox mobile 5 beta. Should we fix this on beta as well or just let go till Firefox mobile 6?

Comment 24

[Axel Hecht [:Pike]]

Thomas, does this block fx mobile 5?

Comment 25

[Thomas Arend [:tarend]]

Not blocking Beta for me.

Comment 26

[Thomas Arend [:tarend]]

I added this bug to "Known Issues" in our Beta rel notes

Comment 27

[Christopher Blizzard (:blizzard)]

Do you want us to track this for 5 final or not?  We don't know if you're OK during beta as well as final or only beta?

Comment 28

[JP Rosevear [:jpr]]

Not tracked for fennec5, so not for trunk either.