1012532 - Replace translation engine usage of RESTRequest with Http.jsm

Status: RESOLVED FIXED

(Firefox :: Translations, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

With Http.jsm we use XHR instead of XPCOM, and we may also get streaming utf8 encoding and JSON/xml parsing.

Http.jsm API needs to be augmented for our usage:
 a) Choose Content-Type of POST requests, which it currently forces to application/x-www-form-urlencoded
 b) Set overrideMimeType to get responseJson and responseXML

When we work on this, maybe a) and b) can be split into their own bugs, and the swapping of RESTRequest to Http.jsm done in this bug

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Hey Iaroslav, I think this would be an interesting bug to work on, if you're interested!

I just saw that translation is broken on e10s and debugging it I found out that the problem is the RESTRequest module, so this bug is now doubly more useful because it would fix translation for e10s.

Comment 2

[Iaroslav Sheptykin]

> Hey Iaroslav, I think this would be an interesting bug to work on, if you're
> interested!
> 
> I just saw that translation is broken on e10s and debugging it I found out
> that the problem is the RESTRequest module, so this bug is now doubly more
> useful because it would fix translation for e10s.

Thanks Felipe! The bug sounds interesting. I will work on it.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Hello Iaroslav, I recall you had a work in progress for this bug. Any updates?

Comment 5

[Iaroslav Sheptykin]

Hi Felipe!
I somewhat forgot about existence of this bug. I somewhat thought that I broke it down to smaller parts and fixed them all. But it is not true. If working on this issue still makes sense I will continue right away!

Comment 6

[leichixian]

As it relates to e10s, if you want to push e10s version with inner translator, maybe you need to fix it. Or our users would say "Hey what's mozilla doing? Why can't I use translator like what they have said on their release note?"

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Hey Iaroslav! Yeah I think this bug still makes sense because we identified in bug 1144135 that RESTRequest isn't working properly with e10s and then causing Translation to not work on e10s. Instead of focusing efforts into fixing RESTRequest, since we already plan to replace it with Http.jsm, I marked bug 1144135 as a "duplicate" of this in the hope that this change will make Translation work with e10s.

From what I remember, RESTRequest calls _chunkCompleted but the response data is broken and only contains a part of the data returned.

Comment 8

[Iaroslav Sheptykin]

Attachment: bug1012532.patch

Hi everybody!

Here is my first attempt to replace the HTTPRequest. I completely replaced the use of HTTPRequest with calls to httpRequest from Http.jsm. I updated bing.sjs to handle contentType properly. All translation tests pass on my local setup. Please, take a look at my code and let me know if this is going in the right direction.

However, I have no experience in e10s testing. I have no idea if these changes solve the problems related to e10s. Could you give me some hints of how to do e10s testing?

Comment 9

[leichixian]

(In reply to Iaroslav Sheptykin from comment #8)
> Created attachment 8580691 [details] [diff] [review]
> bug1012532.patch
> 
> Hi everybody!
> 
> Here is my first attempt to replace the HTTPRequest. I completely replaced
> the use of HTTPRequest with calls to httpRequest from Http.jsm. I updated
> bing.sjs to handle contentType properly. All translation tests pass on my
> local setup. Please, take a look at my code and let me know if this is going
> in the right direction.
> 
> However, I have no experience in e10s testing. I have no idea if these
> changes solve the problems related to e10s. Could you give me some hints of
> how to do e10s testing?
Go to setting-general and you can see enable e10s, if your ime setting disables this, you need change browser. Tabs.xxxxx1(I forgot the value name) to another value, restart Firefox and do as I described former. This can make sure you are in e10s, for I don't know whether new e10s window icon opens an really e10s window.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8580691 [details] [diff] [review]
bug1012532.patch

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

Hi Iaroslav! First, you're awesome! :) I have just tested your patch with e10s, and it works! There were two issues that made it not work with Bing, but I've debugged them and I'll comment below. To test that I used a clientId/apiKey for Bing that I created myself. Have you done that before?

Also, to test e10s it's very simple. Actually, by default Nightly builds will run with e10s activated (but not the tests, unless you pass --e10s to mach). If you launch Nightly and you see tabs with the title underlined, they are e10s tabs, and that's where Translation wasn't working before. To disable e10s, just go to Preferences > General and uncheck "Enable e10s (multi-process)".

::: browser/components/translation/BingTranslator.jsm
@@ +324,1 @@
>        let utf8 = CommonUtils.encodeUTF8(requestString);

because Http.jsm sets charset=utf-8 in the header, it appears that this is not necessary. If we leave it here, the text content is utf8 encoded twice and some characters are returned messed up. I think we can simply remove this, but I'll double check with Florian.

@@ +392,5 @@
>      let params = [
> +      ["grant_type", "client_credentials"],
> +      ["scope", "http://api.microsofttranslator.com"],
> +      ["client_id",
> +      getUrlParam("%BING_API_CLIENTID%", "browser.translation.bing.clientIdOverride")],

getUrlParam calls encodeURLComponent, but Http.jsm also does that, and with this double encoding we fail to get a valid Auth token. You can pass false to the 3rd param here, but since everyone using it is now passing false, I guess we can simply remove that.

or, better, let's not remove it, but make the default value for that optional param to be false instead of true.

::: browser/components/translation/test/bing.sjs
@@ +150,5 @@
>    // First, we'll see if we're dealing with an XML body:
>    let contentType = req.hasHeader("Content-Type") ? req.getHeader("Content-Type") : null;
>    log("contentType: " + contentType);
>  
> +  if (/text\/xml/i.test(contentType)) {

why was this change needed? i.e., what is the contentType here, if not "text/xml"? Is it in uppercase?

Comment 11

[Iaroslav Sheptykin]

Attachment: bug1012532.patch

Hi Felipe!

I'm happy to hear that you could get the e10s working!

> but I've debugged them and I'll comment below. To test that I used a
> clientId/apiKey for Bing that I created myself. Have you done that before?

I could get myself as far as adding browser.translation.bing.apiKeyOverride and browser.translation.bing.clientIdOverride
to the about:config. But I believe the values I used for these keys were incorrect because I always got "Translation is not available at the moment. Please try later" message. I used "testFirefox" as client id and the key from the translation wiki page. I guess I need to follow these steps to get my own key:
https://msdn.microsoft.com/en-us/library/hh454950.aspx

> Also, to test e10s it's very simple. Actually, by default Nightly builds
> will run with e10s activated (but not the tests, unless you pass --e10s to
> mach). If you launch Nightly and you see tabs with the title underlined,
> they are e10s tabs, and that's where Translation wasn't working before. To
> disable e10s, just go to Preferences > General and uncheck "Enable e10s
> (multi-process)".

Alright! I ran translation mochitest with --e10s and they pass.

> ::: browser/components/translation/BingTranslator.jsm
> @@ +324,1 @@
> >        let utf8 = CommonUtils.encodeUTF8(requestString);
> 
> because Http.jsm sets charset=utf-8 in the header, it appears that this is
> not necessary. If we leave it here, the text content is utf8 encoded twice
> and some characters are returned messed up. I think we can simply remove
> this, but I'll double check with Florian.

Ok. I removed this.

> @@ +392,5 @@
> >      let params = [
> > +      ["grant_type", "client_credentials"],
> > +      ["scope", "http://api.microsofttranslator.com"],
> > +      ["client_id",
> > +      getUrlParam("%BING_API_CLIENTID%", "browser.translation.bing.clientIdOverride")],
> 
> getUrlParam calls encodeURLComponent, but Http.jsm also does that, and with
> this double encoding we fail to get a valid Auth token. You can pass false
> to the 3rd param here, but since everyone using it is now passing false, I
> guess we can simply remove that.
> 
> or, better, let's not remove it, but make the default value for that
> optional param to be false instead of true.

Done.

> ::: browser/components/translation/test/bing.sjs
> @@ +150,5 @@
> >    // First, we'll see if we're dealing with an XML body:
> >    let contentType = req.hasHeader("Content-Type") ? req.getHeader("Content-Type") : null;
> >    log("contentType: " + contentType);
> >  
> > +  if (/text\/xml/i.test(contentType)) {
> 
> why was this change needed? i.e., what is the contentType here, if not
> "text/xml"? Is it in uppercase?

This is needed because Http.jsm adds "; utf-8" to the content type for the translation requests. The content type in a such request looks like this "text/xml; utf-8". Previously, the server would expect getting "text/xml" and if not would assume it is a request for the access key. I though adding a regex would make the server a little more flexible.

I will push the patch to the try server if you are ok with the changes.

Comment 12

[Iaroslav Sheptykin]

(In reply to leichixian from comment #9)
Hey leichixian!

> Go to setting-general and you can see enable e10s, if your ime setting
> disables this, you need change browser. Tabs.xxxxx1(I forgot the value name)
> to another value, restart Firefox and do as I described former. This can
> make sure you are in e10s, for I don't know whether new e10s window icon
> opens an really e10s window.

Thanks for the hints. I am running firefox nightly and e10s seems to be on by default. It looks like I now need to get my own api key to play with the translation component.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8581338 [details] [diff] [review]
bug1012532.patch

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

::: browser/components/translation/BingTranslator.jsm
@@ +183,4 @@
>      let results;
>      try {
> +      let xhr = bingRequest.networkRequest;
> +      let doc = xhr.responseXML;

:D

@@ +384,5 @@
>     */
>    _getNewToken: function() {
>      let url = getUrlParam("https://datamarket.accesscontrol.windows.net/v2/OAuth2-13",
>                            "browser.translation.bing.authURL",
>                            false);

the false here could have been removed. But no need to update the patch, I'll do it when pushing.

Comment 14

[Iaroslav Sheptykin]

Hi there! I pushed the patch to the try server.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ebe3f388c60

Comment 15

[Iaroslav Sheptykin]

Hi again!

The translations tests passed. Some unrelated tests failed, but these seem to be known issues. Are we ready for [checkin-needed]?

Comment 16

[Iaroslav Sheptykin]

(In reply to Iaroslav Sheptykin from comment #15)
> Hi again!
> 
> The translations tests passed. Some unrelated tests failed, but these seem
> to be known issues. Are we ready for [checkin-needed]?

Oh, apologies, 2 jobs are still running.

Comment 17

[Iaroslav Sheptykin]

Alright. Now it is safe to say:

The translations tests passed. Some unrelated tests failed, but these seem to be known issues. Are we ready for [checkin-needed]?

Comment 18

[Patrick Cloke [:clokep]]

Comment on attachment 8581338 [details] [diff] [review]
bug1012532.patch

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

::: browser/components/translation/BingTranslator.jsm
@@ +183,4 @@
>      let results;
>      try {
> +      let xhr = bingRequest.networkRequest;
> +      let doc = xhr.responseXML;

Drive-by nit: xhr is only used once, this could more clearly be inlined as:
let doc = bingRequest.networkRequest.responseXML;

@@ +445,5 @@
>  /**
>   * Fetch an auth token (clientID or client secret), which may be overridden by
>   * a pref if it's set.
>   */
> +function getUrlParam(paramValue, prefName, encode = false) {

I don't think it will matter, but I just wanted to point out that the encoding done in Http.jsm is *slightly* different: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Http.jsm#9-13

::: browser/components/translation/test/bing.sjs
@@ +150,5 @@
>    // First, we'll see if we're dealing with an XML body:
>    let contentType = req.hasHeader("Content-Type") ? req.getHeader("Content-Type") : null;
>    log("contentType: " + contentType);
>  
> +  if (/text\/xml/i.test(contentType)) {

Seems like this would be clearer with a call to startsWith. I didn't fully follow the conversation about how Http.jsm sets this to be utf-8. Is this a bug in Http.jsm? Did you talk to Florian about this?

Comment 19

[leichixian]

We need to pay attention on encoding, for wrong encoding may cause bugs only on localized builds. So maybe we should ensure all the requests use same set of encoding.

Comment 20

[Iaroslav Sheptykin]

Attachment: bug1012532.patch

(In reply to Patrick Cloke [:clokep] from comment #18)

Thanks for reviewing my patch, Patrick!
 
> Drive-by nit: xhr is only used once, this could more clearly be inlined as:
> let doc = bingRequest.networkRequest.responseXML;

You right. Felipe too pointed this out. I removed let xhr ...

> I don't think it will matter, but I just wanted to point out that the
> encoding done in Http.jsm is *slightly* different:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Http.jsm#9-13

Maybe Florian can decide here.
 
> Seems like this would be clearer with a call to startsWith. I didn't fully
> follow the conversation about how Http.jsm sets this to be utf-8. Is this a
> bug in Http.jsm? Did you talk to Florian about this?

XHR sets this "contentType: text/xml; charset=UTF-8". I don't think this is a bug in Http.jsm. It seems to me it is the way XHR works. Here [1] XHR documentation talks about setRequestHeader() "If this method is called several times with the same header, the values are merged into one single request header." I guess XHR uses charset=UTF-8 by default and it is merged with the contentType that BingTranslator sets.

1. https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#setRequestHeader%28%29

(In reply to :Felipe Gomes from comment #13)

> the false here could have been removed. But no need to update the patch,
> I'll do it when pushing.

I removed 'false' from the function call. I had to update the patch anyways.

Comment 21

[Florian Quèze [:florian]]

(In reply to Iaroslav Sheptykin from comment #20)

> > I don't think it will matter, but I just wanted to point out that the
> > encoding done in Http.jsm is *slightly* different:
> > https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Http.jsm#9-13
> 
> Maybe Florian can decide here.

I don't think this matters. Do we still need the 'encode' parameter at all on the getUrlParam function?

Comment 22

[Iaroslav Sheptykin]

(In reply to Florian Quèze [:florian] [:flo] from comment #21)

> I don't think this matters. Do we still need the 'encode' parameter at all
> on the getUrlParam function?

I removed the parameter and the tests pass. At this point it looks like BingTranslator does not require it to operate. I cannot think of any use case where having this parameter is beneficial in future. But that could be due to my lack of experience. In comment 10 Felipe wrote:

> or, better, let's not remove it, but make the default value for that 
> optional param to be false instead of true.

So lets ask him.

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8581599 [details] [diff] [review]
bug1012532.patch

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

::: browser/components/translation/BingTranslator.jsm
@@ -178,5 @@
>     * @returns boolean      True if parsing of this chunk was successful.
>     */
>    _parseChunkResult: function(bingRequest) {
>      let domParser = Cc["@mozilla.org/xmlextras/domparser;1"]
>                        .createInstance(Ci.nsIDOMParser);

I just noticed that the domParser can be removed

@@ +442,5 @@
>  /**
>   * Fetch an auth token (clientID or client secret), which may be overridden by
>   * a pref if it's set.
>   */
> +function getUrlParam(paramValue, prefName, encode = false) {

Yeah, encode is no longer necessary. I hadn't asked for it to be removed before to minimize the changes requested. But we might as well do a last update to make the patch as clean as possible! No need to re-send it to try

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Florian, I just wanted to confirm that the line:

>  let utf8 = CommonUtils.encodeUTF8(requestString);

Can be removed given how Http.jsm sets the charset to utf-8 here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Http.jsm#88

I wasn't sure at first if that charset is only telling the server what charset encoding we're sending, but it seems it actually makes Firefox perform the encoding. And therefore we would be doing it twice.

Comment 25

[Iaroslav Sheptykin]

Attachment: bug1012532.patch

(In reply to :Felipe Gomes from comment #23)

Hey Felipe! I updated the patch according to your feedback.

> ::: browser/components/translation/BingTranslator.jsm
> @@ -178,5 @@
> >     * @returns boolean      True if parsing of this chunk was successful.
> >     */
> >    _parseChunkResult: function(bingRequest) {
> >      let domParser = Cc["@mozilla.org/xmlextras/domparser;1"]
> >                        .createInstance(Ci.nsIDOMParser);
> 
> I just noticed that the domParser can be removed

Removed it.

> @@ +442,5 @@
> >  /**
> >   * Fetch an auth token (clientID or client secret), which may be overridden by
> >   * a pref if it's set.
> >   */
> > +function getUrlParam(paramValue, prefName, encode = false) {
> 
> Yeah, encode is no longer necessary. I hadn't asked for it to be removed
> before to minimize the changes requested. But we might as well do a last
> update to make the patch as clean as possible! No need to re-send it to try

I removed the encode parameters and updated the function body. I would like to re-send the patch to the try server just to give the sheriffs another bit of confidence for checking this patch in.

Comment 26

[Iaroslav Sheptykin]

Hi everybody!

Quick update from the try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e09fabde9c7a

To me the report looks good.

If florian confirms that encoding works fine, are we ready for checkin-needed?

Comment 27

[:Felipe Gomes (needinfo for replies!)]

Yep! Let's wait for Florian, and I will push it if no sheriffs pick it up from the checkin-needed flag

Comment 28

[Florian Quèze [:florian]]

(In reply to :Felipe Gomes from comment #24)
> Florian, I just wanted to confirm that the line:
> 
> >  let utf8 = CommonUtils.encodeUTF8(requestString);
> 
> Can be removed given how Http.jsm sets the charset to utf-8 here:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Http.jsm#88

This instruction will only be executed if the post data given as an option is a JS array.

In the case you are interested in, I think the post data is a string, so we are not going through this code path.

Anyway, it seems using UTF8 is the default behavior for XHR, so things should be fine here :).

Comment 29

[Florian Quèze [:florian]]

Comment on attachment 8582309 [details] [diff] [review]
bug1012532.patch

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

While looking at the patch, I found some very tiny coding style issues:

::: browser/components/translation/BingTranslator.jsm
@@ +328,5 @@
> +          deferred.reject(xhr);
> +        },
> +        postData: requestString,
> +        headers: headers
> +      }

missing ';'

@@ +391,5 @@
>      ];
>  
>      let deferred = Promise.defer();
> +    let options = {
> +        onLoad: function(responseText, xhr) {

The indentation is 4 space instead of 2 here.

@@ +419,1 @@
>        }

missing ';'.

Comment 30

[Iaroslav Sheptykin]

Attachment: bug1012532.patch

(In reply to Florian Quèze [:florian] [:flo] from comment #29)

Thanks for the feedback Florian! I updated the patch.

> While looking at the patch, I found some very tiny coding style issues:

You guys have insane "looking" skills! Are you wearing google glass with js lint scanning everything realtime or something? I would not have noticed this in a million years. Seriously, do you run patches through lint-like tool?

> ::: browser/components/translation/BingTranslator.jsm
> @@ +328,5 @@
> > +          deferred.reject(xhr);
> > +        },
> > +        postData: requestString,
> > +        headers: headers
> > +      }
> 
> missing ';'

Added.

> @@ +391,5 @@
> >      ];
> >  
> >      let deferred = Promise.defer();
> > +    let options = {
> > +        onLoad: function(responseText, xhr) {
> 
> The indentation is 4 space instead of 2 here.

Corrected.

> @@ +419,1 @@
> >        }
> 
> missing ';'.

Added.

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Thanks Iaroslav!

https://hg.mozilla.org/integration/fx-team/rev/603d839658dc

Comment 32

[Iaroslav Sheptykin]

(In reply to :Felipe Gomes from comment #31)

Thanks for pointing me to this bug, Felipe! It was fun working on it. I like working on translation. Anything else I could do? otherwise I will just search bugzilla for translation bugs.

Comment 33

[leichixian]

That's great! I'm looking forward to seeing this patch works on nightly v40 on Mar 30th!

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/603d839658dc