761877 - RESTRequest does not handle UTF-8

Status: RESOLVED FIXED

(Cloud Services :: Firefox: Common, defect)

Description

[Jason Smith [:jsmith]]

Steps:

1. Start firefox with aitc enabled on profile #1
2. Go to marketplace.mozilla.org and login with account #1
3. Install Lord of Ultima, FabCam, Pixect, and Calculator
4. Close Firefox
5. Start firefox with profile #2 and enable aitc and myapps
6. Go to myapps and login with account #1

Expected:

4 apps should appear on the dashboard - Lord of Ultima, FabCam, Pixect, Calculator.

Actual:

No apps appear here.

Additional Notes:

Need to pull logs here to find out what's going on here. I've tried this on 3 distinct browser ID accounts and have consistently reproduced this behavior with those 4 particular apps.

Comment 1

[Jason Smith [:jsmith]]

Nominating for k9o - There's no reason why these apps should not sync to the cloud, and one of the particular apps affected here happens to be a tier 1 app.

Comment 2

[Jason Smith [:jsmith]]

Talking with Anant, this is expected for AITC - it's enforcing the MIME type and encoding to allow for apps to sync to the cloud. However, note that the mozapps API is currently not doing this - bug 741526. We need to fix that bug to avoid an inconsistency where one implementation follows the MIME type and encoding check, but not the other. Resolving as invalid here, as this is not a problem with AITC specifically.

Comment 3

[Jason Smith [:jsmith]]

Reopening. Checking Lord of Ultima, it looks like it is serving the correct content-type, so this problem has nothing to do with that.

Comment 4

[Jason Smith [:jsmith]]

HTTP Headers for Lord of Ultima:

http://www.lordofultima.com/static/app/mozilla/manifest.webapp

GET /static/app/mozilla/manifest.webapp HTTP/1.1
Host: www.lordofultima.com
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip, deflate
Connection: keep-alive
Cookie: language=en; is_returning=1; __utmx=30514082.00020640553490418842:3:; __utmxx=30514082.00020640553490418842:1333257610:2592000; __utma=74606733.131971208.1335249011.1335312740.1336923013.6; __utmz=74606733.1335249011.1.1.utmcsr=(direct)|utmccn=(direct)|utmcmd=(none); mybb[lastvisit]=1335248408; mybb[lastactive]=1335248408

HTTP/1.0 200 OK
Date: Wed, 06 Jun 2012 14:33:11 GMT
Server: Apache
Last-Modified: Fri, 25 May 2012 06:52:40 GMT
Accept-Ranges: bytes
Content-Length: 2219
X-Orig-Server: 265277-web01.rspc-iad.ea.com
Connection: close
Content-Type: application/x-web-app-manifest+json; charset=utf-8
----------------------------------------------------------

Comment 5

[Jason Smith [:jsmith]]

HTTP Headers for FabCam:

http://fabcam.net/manifest.webapp

GET /manifest.webapp HTTP/1.1
Host: fabcam.net
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip, deflate
Connection: keep-alive

HTTP/1.1 200 OK
Date: Wed, 06 Jun 2012 14:37:35 GMT
Server: LiteSpeed
Accept-Ranges: bytes
Connection: close
Etag: "376-4f9eabc2-0"
Last-Modified: Mon, 30 Apr 2012 15:12:02 GMT
Content-Type: application/x-web-app-manifest+json
Content-Length: 886
----------------------------------------------------------

Comment 6

[Anant Narayanan [:anant]]

RESTRequest, Y U NO UTF-8?

As I suspected earlier, the root cause for this bug is RESTRequest in services-common. It does not handle UTF-8 responses correctly. Patch incoming.

Comment 7

[Anant Narayanan [:anant]]

Attachment: Make RESTRequest handle different charsets

If the response headers contain a Content-Type header with a charset value, we must interpret the bytes accordingly. This patch decides to use a converter input stream instead of a generic input stream if this is the case.

All the tests pass, including the one added to specifically test this case.

Comment 8

[Anant Narayanan [:anant]]

Ignore the logging modification, it was a local change that got rolled into the patch inadvertently.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 632222 [details] [diff] [review]
Make RESTRequest handle different charsets

Review of attachment 632222 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this looks good. I almost wonder if we should use NetUtil.readInputStreamToString() instead. Although, I wonder if that handles binary sequences properly. Docs say the default charset encoding is Latin-1. This would be a follow-up bug, regardless.

::: services/common/tests/unit/test_restrequest.js
@@ +163,5 @@
> + * Test HTTP GET with UTF-8 content, and custom Content-Type.
> + */
> +add_test(function test_get_utf8() {
> +  let response = "Hello World or Καλημέρα κόσμε or こんにちは 世界";
> +  let contentType = "application/x-web-app-manifest+json; charset=UTF-8";

Is it really JSON? Can we just use "text/plain; charset=UTF-8"?

Comment 10

[Anant Narayanan [:anant]]

https://hg.mozilla.org/services/services-central/rev/d8e9c8848a2a

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/d8e9c8848a2a

Comment 12

[Jason Smith [:jsmith]]

Testing the test case comment 0 did not work - I ended up with no apps showing up the 2nd profile after I synced. I then tried going back to the first profile with the 4 apps identified in comment 0 and installing an additional app and jumping back to the 2nd profile - this time that app installed + 1 additional app showed up. Something definitely isn't right here...

Going to dig into what's going wrong, but either this bug isn't fixed, or another problem has shown up.

Comment 13

[Jason Smith [:jsmith]]

Attachment: View Four Apps from Comment 0 After 1st Patch Applied Log

Comment 14

[Jason Smith [:jsmith]]

Looks like I can do this consistently too. This time, doing test case in comment 0 resulted in only one app showing up in the 2nd profile. There's definitely errors showing up in the log too:

1341554621924	Service.AITC.Storage	WARN	_getManifest got invalid JSON response: {

1341554621932	Service.AITC.Storage	WARN	_getManifest got invalid JSON response: { 

1341554621950	Service.AITC.Storage	WARN	_getManifest got invalid JSON response: {

1341554621950	Service.AITC.Storage	WARN	Could not fetch manifest for undefined

Sounds like the same bug, but someone should confirm. Same or different bug? Anant - Any ideas?

Comment 15

[Anant Narayanan [:anant]]

Do you happen to know the manifest URIs for Pixect, Calculator and Fabcam? Lord of Ultima seems to be the only one fixed, I want to investigate why these other ones are broken.

Comment 16

[Anant Narayanan [:anant]]

The Content-Type header for Fabcam doesn't have a charset:

$ curl -vv http://fabcam.net/manifest.webapp
* About to connect() to fabcam.net port 80 (#0)
*   Trying 69.175.41.102... connected
> GET /manifest.webapp HTTP/1.1
> User-Agent: curl/7.22.0 (x86_64-pc-linux-gnu) libcurl/7.22.0 OpenSSL/1.0.1 zlib/1.2.3.4 libidn/1.23 librtmp/2.3
> Host: fabcam.net
> Accept: */*
> 
< HTTP/1.1 200 OK
< Date: Fri, 06 Jul 2012 06:31:00 GMT
< Server: LiteSpeed
< Accept-Ranges: bytes
< Connection: close
< ETag: "376-4f9eabc2-0"
< Last-Modified: Mon, 30 Apr 2012 15:12:02 GMT
< Content-Type: application/x-web-app-manifest+json
< Content-Length: 886

I suspect the other manifests are similar. I think we should infer all content as UTF-8 by default, but I'm not sure of the full implications of that. Will try a test patch in any case.

Comment 17

[Anant Narayanan [:anant]]

Attachment: Default to UTF-8 if no charset is specified

This patch makes RESTRequest treat content as UTF-8 by default.

Comment 18

[Anant Narayanan [:anant]]

The unit tests all pass, so hopefully this shouldn't have any adverse affect. Trying out a try build!

Comment 19

[Anant Narayanan [:anant]]

Well, the autoland bot never came by. TBPL: https://tbpl.mozilla.org/?tree=Try&rev=ad3728a29383 and builds should be available later at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ad3728a29383

Comment 20

[Jason Smith [:jsmith]]

(In reply to Anant Narayanan [:anant] from comment #19)
> Well, the autoland bot never came by. TBPL:
> https://tbpl.mozilla.org/?tree=Try&rev=ad3728a29383 and builds should be
> available later at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/anarayanan@mozilla.
> com-ad3728a29383

Much better. No issues this time testing the test case in comment 0 to install those 4 particular apps and viewing them on a different profile:

1341614560862	Service.AITC.Manager	INFO	Attempting to getApps
1341614560924	Service.AITC.Client	INFO	getApps succeeded and got 4
1341614560924	Service.AITC.Storage	INFO	Server check got 4 apps
1341614560924	Service.AITC.Storage	INFO	Applying 4 installs to registry
1341614561207	Service.AITC.Storage	DEBUG	1/4 apps processed
1341614561212	Service.AITC.Storage	DEBUG	2/4 apps processed
1341614561354	Service.AITC.Storage	DEBUG	3/4 apps processed
1341614561535	Service.AITC.Storage	DEBUG	4/4 apps processed
1341614561535	Service.AITC.Manager	INFO	processApps completed successfully, changes applied

Comment 21

[Anant Narayanan [:anant]]

Comment on attachment 639592 [details] [diff] [review]
Default to UTF-8 if no charset is specified

Thanks for the test Jason. r? so we can land this.

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 639592 [details] [diff] [review]
Default to UTF-8 if no charset is specified

Review of attachment 639592 [details] [diff] [review]:
-----------------------------------------------------------------

It is potentially dangerous to change the default encoding like this.

Since this issue is separate from what the bug was originally for, please file a separate bug to deal with handling the default encoding. We'll discuss further there.

Comment 23

[Gregory Szorc [:gps]]

Please file a separate bug to track default encoding handling.