Closed Bug 1362099 Opened 7 years ago Closed 7 years ago

FxAccountsClient.getDeviceList truncates responses longer than around 8192 bytes

Categories

(Firefox :: Sync, enhancement, P1)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

Details

Attachments

(1 file)

Bug 1362099 - Make RESTRequest handle GET's where the response is > 8192 bytes and we know the charset
59 bytes, text/x-review-board-request
markh
: review+
Details 
This leads to invalid JSON. I'm fairly confident the server is sending the data correctly (invoking the request via curl gets everything), but I don't know substantially more than that.
Here's what I get if I copy the response from Firefox: http://v14d.com/i/590a56650632c.jpg
:vladikoff sounds like you're able to repro?  If so can you please post STR here for completeness?
Flags: needinfo?(vlad)
### STR

0. Configure Firefox Nightly to sync with FxA latest (https://latest.dev.lcip.org/) 
OR start the fxa-dev-launcher[0] with `FIREFOX_DEBUGGER=true FIREFOX_BIN=/Applications/FirefoxNightly.app/Contents/MacOS/firefox FXA_ENV=latest npm start`

1. sign in into sync with account `signin0.17990885698236525@restmail.net` // `password`

### Expected

Firefox should properly fetch the device list.

### Actual

Firefox mangles the device list response[1] in different ways, example:

```
  {  
      "id":"69f0dead6a304ull,"      pushPublicKey":null,
      "pushAuthKey":null,
      "isCurrentDevice":false,
      "lastAccessTimeFormatted":""
   },
```

# Details

You can see the 2 chunks arrive by setting a breakpoint at: http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/services/common/rest.js#534

The first chunk (8190 bytes) concatenated with the second one does not produces correct JSON.
If the request is copied from the browser toolbox and resent via `curl` the JSON is valid.

 



[0] - https://github.com/vladikoff/fxa-dev-launcher#basic-usage-example-in-os-x
[1] - response: https://gist.githubusercontent.com/vladikoff/75c6ba066e6450ef2e5762f7aab06051/raw/9b61c458120e8a7f5eb15ba4766df69c21ef5938/cool.md
Flags: needinfo?(vlad)
If the you have logs properly enabled you should see something like this:

```
1493931548142	FirefoxAccounts	DEBUG	writing plain storage: ["email","sessionToken","uid","verified","deviceId","deviceRegistrationVersion","oauthTokens","profileCache"]
1493931548145	FirefoxAccounts	DEBUG	writing secure storage: ["kA","kB"]
1493931554573	FirefoxAccounts	ERROR	json parse error on response: ** MANGLED JSON HERE **    [1]


```

[1] - response:
https://gist.githubusercontent.com/vladikoff/75c6ba066e6450ef2e5762f7aab06051/raw/9b61c458120e8a7f5eb15ba4766df69c21ef5938/cool.md
When I do the above process, I also get lines like this in my sync log:

"""
1493944035659	Sync.Store.Tabs	DEBUG	Adding remote tabs from vladikoff’s Nightly on Vlads-MBP
"""

Which look like a corruption of the quote character from the actual device name.  Maybe separate logging bug, or maybe related?
(In reply to Ryan Kelly [:rfkelly] from comment #5)
> When I do the above process, I also get lines like this in my sync log:
> 
> """
> 1493944035659	Sync.Store.Tabs	DEBUG	Adding remote tabs from vladikoff’s
> Nightly on Vlads-MBP
> """
> 
> Which look like a corruption of the quote character from the actual device
> name.  Maybe separate logging bug, or maybe related?

To isolate the issue, I removed the default `getDefaultDeviceName()` and made it return "cake".

Still getting mangled parse: https://gist.githubusercontent.com/vladikoff/725835e4398c0c5e4ce0ccc973316261/raw/8cf3b419bf5914f7f6cfb0cf7fed0a80c0d4f866/resp2.md

Corrupted JSON:

```
   {  
      "id":"763b9842ee22cd6bfd47143f5e36cd29",
      "lastAccessTime":null,
      "name":"wow0.5523885115981102",
      "type":"mobile",
,
      "lastAccessTime":null,
      "name":"wow0.249236122937873",
      "type":"mobile",
      "pushCallback":null,
      "pushPublicKey":null,
      "pushAuthKey":null,
```

Here's the missing bit from "wow0.5523885115981102" to "wow0.249236122937873" (via curl):

https://gist.github.com/vladikoff/b5634aca928f9c78a4dde2c83bc09861
(In reply to Ryan Kelly [:rfkelly] from comment #5)
> When I do the above process, I also get lines like this in my sync log:
> 
> """
> 1493944035659	Sync.Store.Tabs	DEBUG	Adding remote tabs from vladikoff’s
> Nightly on Vlads-MBP
> """
> 
> Which look like a corruption of the quote character from the actual device
> name.  Maybe separate logging bug, or maybe related?

I believe that bug is the fact that the log viewer in Firefox isn't set to display utf-8 data. If I open the actual log from on disk in an editor (configure to use utf-8) it appears correctly. IOW, I think this is an about:sync-log bug and probably unrelated.
Assignee: nobody → tchiovoloni
Status: NEW → ASSIGNED
The handling in the attached patch is consistent with the way we use converter-input-stream elsewhere in the tree (specifically mostly based on https://dxr.mozilla.org/mozilla-central/source/devtools/client/jsonview/converter-child.js#79-87)
:eoger and :vladikoff also had a fix for this via

```
diff --git a/services/common/rest.js b/services/common/rest.js
index 0a7c48e6cbf4..e6873e6e4605 100644
--- a/services/common/rest.js
+++ b/services/common/rest.js
@@ -528,9 +528,13 @@ RESTRequest.prototype = {
                                 this._converterStream.DEFAULT_REPLACEMENT_CHARACTER);

      try {
-        let str = {};
-        let num = this._converterStream.readString(count, str);
-        if (num != 0) {
+        while (count) {
+          let str = {};
+          let bytesRead = this._converterStream.readString(count, str);
+          if (!bytesRead) {
+            break;
+          }
+          count -= bytesRead;
          this.response.body += str.value;
        }
      } catch (ex) {
```

However :tcsc sent the mozreview 5 minutes earlier :D
Priority: -- → P1
Comment on attachment 8865000 [details]
Bug 1362099 - Make RESTRequest handle GET's where the response is > 8192 bytes and we know the charset

https://reviewboard.mozilla.org/r/136660/#review140884

Thanks - I agree this fixes the issue with data > 8kb, but as we discussed in IRC, I'm still a little concerned we might do bad things if a utf-8 char is split between 2 onDataAvilable calls. I opened bug 1363581 for that, but think we might as well land this now (as this scenario is more likely than the split utf-8)
Attachment #8865000 - Flags: review?(markh) → review+
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/39ca824921c4
Make RESTRequest handle GET's where the response is > 8192 bytes and we know the charset r=markh
https://hg.mozilla.org/mozilla-central/rev/39ca824921c4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: