977844 - Make sure snippets response is valid JSON before caching it (or trying to use it)

Status: RESOLVED FIXED

(Firefox for Android :: General, defect)

Comment 2

[:Margaret Leibovic]

Attachment: patch

To test this locally, I just hacked the value of resposeText to some bogus value, and the error got reported as expected, and nothing ended up getting cached.

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 8383340 [details] [diff] [review]
patch

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

::: mobile/android/components/Snippets.js
@@ +119,5 @@
>  function updateSnippets() {
>    _httpGetRequest(gSnippetsURL, function(responseText) {
> +    try {
> +      let messages = JSON.parse(responseText);
> +      cacheSnippets(messages);

Can you just pass in responseText directly here? At this point, you know it's valid since you already did JSON.parse().

@@ +135,3 @@
>   */
> +function cacheSnippets(messages) {
> +  let data = gEncoder.encode(JSON.stringify(messages));

...that way, you don't need to re-stringify the string you just parsed.

Comment 4

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/6de7f6039a68

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6de7f6039a68

Comment 6

[:Margaret Leibovic]

Comment on attachment 8383340 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): enabling snippets
User impact if declined: snippets can get in busted state if server returns bad value
Testing completed (on m-c, etc.): baked on nightly since 2/28 and tested locally, since we don't have any active snippets on Nightly right now
Risk to taking this patch (and alternatives if risky): low-risk, makes snippet caching more robust
String or IDL/UUID changes made by this patch: none

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6249bc120eb6