1565779 - Various search engines are using the wrong encoding for search parameters after switch to WebExtensions

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: Completely broken

Firefox67 ja build works as expected.
Firefox68 ja build is broken.

Reproducible : always

Steps to Reproduce:

1. Enable search bar
2. type japanese text e.g,"パソコン"
3. Click ヤフオク or 楽天市場 on-off search button

Actual results:
Unable search in japanese text due to Mojibake.

Expected Results:
Search should successfully perform.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c77962add953a48bdbcbbcd3d157d63d1267cd8e&tochange=a632dfa60af6edd4f7dd03d404d898b6222b56c3

Regressed by:Bug 1496075

Dale Harvey,
your bunch of patch seems to cause the regression. Can you please look into this?

Comment 2

[Francesco Lodolo [:flod]]

Yahoo Auctions: https://searchfox.org/mozilla-central/source/browser/components/search/extensions/yahoo-jp-auctions
Rakuten: https://searchfox.org/mozilla-central/source/browser/components/search/extensions/rakuten

Looking at ESR60, where we still had XML files
https://hg.mozilla.org/releases/mozilla-esr60/file/tip/browser/locales/searchplugins/yahoo-jp-auctions.xml
https://hg.mozilla.org/releases/mozilla-esr60/file/tip/browser/locales/searchplugins/rakuten.xml

They both used EUC-JP encoding, not UTF-8. Where's that in the webextension version?

Comment 3

[Alice0775 White]

Attachment: about:support raw data of Firefox68 ja

{
"name": "ヤフオク!",
"version": "1.0",
"isActive": true,
"id": "[e-mail]"
},
{
"name": "楽天市場",
"version": "1.0",
"isActive": true,
"id": "[e-mail]"
},

Comment 4

[Dale Harvey (:daleharvey)]

Attachment: Bug 1565779 - Add encoding to non UTF-8 engines.

Comment 5

[Dale Harvey (:daleharvey)]

I missed the need to translate the InputEncoding field of the original engines, we default to UTF-8 @ https://searchfox.org/mozilla-central/source/toolkit/components/search/SearchService.jsm#2242 so filed in all the non utf-8 versions

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f65f89559b5
Add encoding to non UTF-8 engines. r=Standard8

Comment 7

[Mark Banner (:standard8)]

Overall this affects:

• sv-SE: Allaannonser
• ca: DIEC2
• kn: Kannada Store
• ru: OZON.ru
• pt-PT: Priberam
• ja/ja-JP-macos:
  • rakuten (楽天市場)
  • yahoo-jp-auctions (ヤフオク!)
• hu: Vatera.hu
• sk: Zoznam

Comment 9

[Dale Harvey (:daleharvey)]

Comment on attachment 9077933 [details]
Bug 1565779 - Add encoding to non UTF-8 engines.

Beta/Release Uplift Approval Request

• User impact if declined: Built in search engines in particular locales will be broken, list of engines @ https://bugzilla.mozilla.org/show_bug.cgi?id=1565779#c7 will be broken
• 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: STR listed in https://bugzilla.mozilla.org/show_bug.cgi?id=1565779#c0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Adds a parameter to search engine requests that was missed in an automated conversion
• String changes made/needed:

Comment 10

[Dale Harvey (:daleharvey)]

Comment on attachment 9077933 [details]
Bug 1565779 - Add encoding to non UTF-8 engines.

As https://bugzilla.mozilla.org/show_bug.cgi?id=1565779#c9

Comment 11

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/0f65f89559b5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9077933 [details]
Bug 1565779 - Add encoding to non UTF-8 engines.

Fixes broken search engines in some locales. Approved for 69.0b5.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/593102394614

Comment 14

[Jorg K (CEST = GMT+2)]

Any reason you're using non-standard encoding names?

ISO-8859-1 is just a label of windows-1252 and WINDOWS-1251, WINDOWS-1250 should be lowercase according to the standard: https://encoding.spec.whatwg.org/

Comment 15

[Jorg K (CEST = GMT+2)]

Alice, is this an issue for search in TB 68 beta 4? Then we'd need to port this to TB in a new bug.

Comment 16

[Alice0775 White]

(In reply to Jorg K (GMT+2) from comment #15)

Alice, is this an issue for search in TB 68 beta 4? Then we'd need to port
this to TB in a new bug.

I filed a new Bug 1566200

Comment 17

[Mark Banner (:standard8)]

(In reply to Jorg K (GMT+2) from comment #14)

Any reason you're using non-standard encoding names?

ISO-8859-1 is just a label of windows-1252 and WINDOWS-1251, WINDOWS-1250 should be lowercase according to the standard: https://encoding.spec.whatwg.org/

We've simply restored what we were using before, which considering the patch is going to a dot release is probably the safest thing to do.

I'm guessing in this case it probably doesn't matter seeing as I think we handle the encoding internally and that doesn't seem to complain. Also I couldn't actually see in the spec where it said they must be lowercase. I saw some references to case-insensitive, but nothing appeared directly relevant to the case of the encoding labels.

Comment 18

[Alice0775 White]

Build Id 20190716001037
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:70.0) Gecko/20100101 Firefox/70.0

I just test Noghtly70.0a1 ja build with clean new profile.
However, the problem is not foxed.
I can still reproduce the issue.

Dale Harvey, What's going on?

Comment 19

[Alice0775 White]

Attachment: sessionstore.jsonlz4 15-jul-2019 (without this fix)

Comment 20

[Alice0775 White]

Attachment: search.json.mozlz4 (20190715094048 without this fix)

Comment 21

[Alice0775 White]

Attachment: search.json.mozlz4 (20190716001037 with this fix)

Comment 22

[Mark Banner (:standard8)]

I can confirm this doesn't seem to be fixed on the latest jp build on Mac. I think I see what the issue is - the encoding has been added at the wrong place in the manifest files. I'm trying to confirm.

Comment 23

[Mark Banner (:standard8)]

Attachment: Bug 1565779 - Move encoding definitions to the correct location in the search engine manifest files. r?daleharvey

Comment 24

[Mark Banner (:standard8)]

Dale said when he tested the script the encodings were in the right place, but somehow ended up in the wrong place. I missed that in reviewing.

I've tested this locally for Rakuten and yahoo-jp-auctions by adding a section to list.json and using that.

Comment 25

[Dale Harvey (:daleharvey)]

Yeh this was my mistake, I tested in the correct place, check where it was supposed to be, then for some reason wrote the final patch wrong, r+ fix is good

Comment 26

[Mark Banner (:standard8)]

Landing triggered, but trees are currently closed.

Comment 27

[Mark Banner (:standard8)]

Comment on attachment 9078340 [details]
Bug 1565779 - Move encoding definitions to the correct location in the search engine manifest files. r?daleharvey

Beta/Release Uplift Approval Request

• User impact if declined: See comment 9, this is a follow-on patch.
• 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: See comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Corrects the initial patch. Unfortunately we missed getting the parameter in the right place, testing the different engines is hard at the moment, and we want to make that easier (bug 1566377).
• String changes made/needed: None

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60884ef3024b
Move encoding definitions to the correct location in the search engine manifest files. r=daleharvey

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9078340 [details]
Bug 1565779 - Move encoding definitions to the correct location in the search engine manifest files. r?daleharvey

Follow-up fix to the previous patch. Approved for 69.0b6.

Comment 30

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/60884ef3024b

Comment 31

[Julien Cristau [:jcristau]]

Comment on attachment 9077933 [details]
Bug 1565779 - Add encoding to non UTF-8 engines.

search fix, approved for 68.0.1 and 68.1esr

Comment 32

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dab6b5b99278

Comment 33

[Mark Banner (:standard8)]

I've just done a spot check of the two ja-JP-mac locale engines and they seem to be working fine now on latest nightly.

Comment 34

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/9cd385896b79
https://hg.mozilla.org/releases/mozilla-release/rev/035cb9d1676b

Comment 35

[Ryan VanderMeulen [:RyanVM]]

default (68.1esr):
https://hg.mozilla.org/releases/mozilla-esr68/rev/bad97d6baab14c022cf973826f64dabf3c894825
https://hg.mozilla.org/releases/mozilla-esr68/rev/2c3cf1bd7e6cc62ecc732ed337e1a05bb9b1fc46

FIREFOX_ESR_68_0_X_RELBRANCH (68.0.1esr):
https://hg.mozilla.org/releases/mozilla-esr68/rev/56133138bc14aa82633bf05b8046445f06afb1f6
https://hg.mozilla.org/releases/mozilla-esr68/rev/64b3352a762ab22e1e8638b178cac65d65dc7421

Comment 36

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 70.0a1 (2019-07-17) ja build on Windows 10 x 64, Mac OS X 10.14 and on Ubuntu 18.04 x64.

Comment 37

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox 68.0.1esr and on Firefox 68.0.1 ja build on Windows 10 x 64, Mac OS X 10.14 and on Ubuntu 18.04 x64.

Comment 38

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox 69.b06 ja build on Windows 10 x 64, Mac OS X 10.14 and on Ubuntu 18.04 x64.