Last Comment Bug 645970 - Search engines in fennec language packs don't work, search service doesn't unpack jar:jar:
: Search engines in fennec language packs don't work, search service doesn't un...
Status: VERIFIED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: 4.0 Branch
: ARM Android
: -- normal (vote)
: ---
Assigned To: Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
:
Mentors:
: 643048 (view as bug list)
Depends on:
Blocks: packedxpi 601018
  Show dependency treegraph
 
Reported: 2011-03-29 00:52 PDT by Cristian Nicolae (:xti)
Modified: 2011-05-25 11:43 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
affected


Attachments
Patch (982 bytes, patch)
2011-03-29 05:45 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
gavin.sharp: review+
Details | Diff | Splinter Review
Patch + test (10.92 KB, patch)
2011-04-13 09:50 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch + test (v0.2) (16.74 KB, patch)
2011-04-13 10:16 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
gavin.sharp: review-
Details | Diff | Splinter Review
Patch + test (v0.3) (11.75 KB, patch)
2011-04-13 10:51 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
gavin.sharp: review+
Details | Diff | Splinter Review

Description Cristian Nicolae (:xti) 2011-03-29 00:52:04 PDT
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 [pto-Aug-30][:Pike] 2011-03-29 03:06:57 PDT
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?) 2011-03-29 05:03:50 PDT
Cristian - Have you tested other locales? I am wondering if this happens everywhere, or just in Arabic.
Comment 3 Axel Hecht [pto-Aug-30][:Pike] 2011-03-29 05:18:04 PDT
I tested with the german langpack, too.

chrome://browser/locale/searchplugins/ shows the german content allright then.
Comment 4 Ioana Chiorean 2011-03-29 05:31:38 PDT
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) 2011-03-29 05:45:19 PDT
Created attachment 522651 [details] [diff] [review]
Patch

Per IRC this happen at least on Arabic, German and French, and this probably happens for each langs :(
Comment 6 Axel Hecht [pto-Aug-30][:Pike] 2011-03-29 05:51:15 PDT
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?) 2011-03-29 06:16:58 PDT
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?) 2011-03-29 06:20:05 PDT
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: gavin@gavinsharp.com] 2011-04-12 21:04:06 PDT
Comment on attachment 522651 [details] [diff] [review]
Patch

I wish we had tests :(
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-12 21:11:13 PDT
Actually I suppose it would be relatively easy to test this with an xpcshell test.
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2011-04-12 21:13:32 PDT
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) 2011-04-13 09:50:37 PDT
Created attachment 525703 [details] [diff] [review]
Patch + test

Same patch with a simple test.
Comment 13 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-04-13 09:52:18 PDT
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) 2011-04-13 10:16:20 PDT
Created attachment 525716 [details] [diff] [review]
Patch + test (v0.2)

From irc comments, the patch with more files (and a fix)
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-13 10:31:12 PDT
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) 2011-04-13 10:51:52 PDT
Created attachment 525726 [details] [diff] [review]
Patch + test (v0.3)
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-13 11:28:25 PDT
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 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-13 11:29:13 PDT
*** Bug 643048 has been marked as a duplicate of this bug. ***
Comment 19 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-04-13 11:31:24 PDT
(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) 2011-04-15 04:00:57 PDT
http://hg.mozilla.org/mozilla-central/rev/15b8b01a965e

I have removed the comment, poor jarjar!
Comment 21 :Ms2ger (⌚ UTC+1/+2) 2011-04-15 04:11:44 PDT
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 [:kbrosnan] 2011-04-15 12:16:30 PDT
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 2011-05-19 08:54:15 PDT
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 [pto-Aug-30][:Pike] 2011-05-19 09:17:51 PDT
Thomas, does this block fx mobile 5?
Comment 25 Thomas Arend [:tarend] 2011-05-19 10:38:13 PDT
Not blocking Beta for me.
Comment 26 Thomas Arend [:tarend] 2011-05-19 10:38:55 PDT
I added this bug to "Known Issues" in our Beta rel notes
Comment 27 Christopher Blizzard (:blizzard) 2011-05-23 15:10:12 PDT
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] 2011-05-25 11:43:36 PDT
Not tracked for fennec5, so not for trunk either.

Note You need to log in before you can comment on or make changes to this bug.