Closed Bug 1388830 Opened 5 years ago Closed 4 years ago

Sync failure with "Sync.Engine.Extension-Storage ERROR Syncing chrome-gnome-shell@gnome.org: request failed: Error: HTTP 503; SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data"

Categories

(WebExtensions :: Storage, defect, P5)

defect

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: dholbert, Assigned: glasserc)

Details

(Whiteboard: [storage])

Attachments

(1 file)

I'm getting repeated Sync errors (on two different machines) with this in my error logs at about:sync-log:
{
1502302295323	Sync.Engine.Extension-Storage	ERROR	Syncing chrome-gnome-shell@gnome.org: request failed: Error: HTTP 503; SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data (resource://services-common/kinto-http-client.js:2353:21) JS Stack trace: processResponse@kinto-http-client.js:2351:44

1502302295323	Sync.Engine.Extension-Storage	WARN	Syncing failed: Error: HTTP 503; SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data (resource://services-common/kinto-http-client.js:2353:21) JS Stack trace: processResponse@kinto-http-client.js:2351:44

1502302295324	Sync.Status	DEBUG	Status for engine extension-storage: error.engine.reason.unknown_fail

1502302295324	Sync.Status	DEBUG	Status.service: success.status_ok => error.sync.failed_partial

1502302295324	Sync.ErrorHandler	DEBUG	extension-storage failed: Error: HTTP 503; SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data (resource://services-common/kinto-http-client.js:2353:21) JS Stack trace: processResponse@kinto-http-client.js:2351:44
}

This is happening on two different machines (a laptop and a desktop), both running Ubuntu, both on yesterday's Nightly.
The supposed JSON Data with the unexpected first character is, in fact, the following:
<html>
<head><title>503 Service Temporarily Unavailable</title></head>
<body bgcolor="white">
<center><h1>503 Service Temporarily Unavailable</h1></center>
<hr><center>nginx</center>
</body>
</html>

I determined this by editing kinto-http-client.js to append the 'text' JSON data to the exception that it throws.

Looks like:
 (a) some Sync-related server is down (wherever this data would be coming from), and that needs some investigation.

 (b) In the sync client, we should avoid trying to parse Error 503 responses as if they were JSON, so that we don't print this nonsensical error about a bogus first character (for "<" of "<html>" which is clearly not JSON)
(In reply to Daniel Holbert [:dholbert] from comment #1)
> Looks like:
>  (a) some Sync-related server is down (wherever this data would be coming
> from), and that needs some investigation.

In IRC, Ethan says this is ongoing maintenance. 
> 
>  (b) In the sync client, we should avoid trying to parse Error 503 responses
> as if they were JSON, so that we don't print this nonsensical error about a
> bogus first character (for "<" of "<html>" which is clearly not JSON)

Yes, this should definitely be done. Sync itself already had good support for this, including the ability to back-off etc, but the WebExtensions team implemented their own engine using Kinto, so is likely to suffer from many of these kinds of edge-cases. I'm shifting the component so it gets on the radar of the team which maintains this special engine as it seems unlikely they can benefit from Sync's infrastructure for this.
Component: Sync → WebExtensions: General
Product: Firefox → Toolkit
Priority: -- → P5
Whiteboard: [storage]
Component: WebExtensions: General → WebExtensions: Storage
I've been doing some work over in kinto-http.js to try to firm up our contracts on what constitutes an error and how we handle it. I've just pushed to mozreview a commit that pulls in https://github.com/Kinto/kinto-http.js/releases/tag/v4.4.0, whose major improvement is the introduction of several exception classes that correspond to different failure cases, namely NetworkTimeoutError, UnparseableResponseError, and ServerResponse.

Although comment 2 describes back-offs and other related load-mitigation concerns, I believe that issue should be considered separately in bug 1415222. Therefore the only concern in this bug is about what we do when we get 503 non-JSON errors. Kinto server errors are expected to contain a JSON response body explaining what went wrong, so a non-JSON error being served by nginx is a violation of the client's expectations. For this reason I think it's acceptable to throw a "didn't understand server" error, in this case the new UnparseableResponseError. However, one improvement that I tried to make is to include the non-JSON error as part of the error message, so that a user can see the response without resorting to any gymnastics.

If you don't agree with me that an UnparseableResponseError is correct, I'm curious what you believe the correct behavior should be. We could catch this error in ExtensionStorageSync.jsm, and perhaps rethrow it as something different, but fundamentally this was a server error, as indicated by the 503 status code, so I don't think it makes sense to silence this error and pretend everything went OK.
Comment on attachment 8929047 [details]
Bug 1388830 - upgrade kinto-http.js to 4.5.3.

https://reviewboard.mozilla.org/r/200354/#review205564

There are a bunch of problems with this that I discovered while trying to use it for bug 1415222. Among others, it uses Error.captureStackTrace (which doesn't exist in Gecko), it doesn't export the error classes in the fx build, and the babel transpilation isn't necessary for Gecko.
Attachment #8929047 - Flags: review-
Attachment #8929047 - Flags: review?(MattN+bmo)
(In reply to Ethan Glasser-Camp (:glasserc) from comment #4)
> Therefore the only concern in this bug is about what we do when we
> get 503 non-JSON errors.

Which part of the JSON in a 50X is actually usable? The headers have the backoff/retry info, so ISTM the entire response is purely diagnostic in practice and the first kB or so can be logged as a string.

I expect it's even easier to just ensure better diagnostics are rolled up in bug 1415222, and close this as a dupe of that?
Comment on attachment 8929047 [details]
Bug 1388830 - upgrade kinto-http.js to 4.5.3.

https://reviewboard.mozilla.org/r/200354/#review226136

rs=me
Attachment #8929047 - Flags: review?(MattN+bmo) → review+
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/684f8415581e
upgrade kinto-http.js to 4.5.3. r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/684f8415581e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(eglassercamp)
Flags: needinfo?(eglassercamp) → qe-verify-
Product: Toolkit → WebExtensions
Assignee: nobody → eglassercamp
You need to log in before you can comment on or make changes to this bug.