Closed Bug 977844 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
fennec 29+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file)

No description provided.
Duplicate of this bug: 977837
Attached patch patchSplinter Review
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.
Attachment #8383340 - Flags: review?(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.
Attachment #8383340 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/6de7f6039a68
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
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
Attachment #8383340 - Flags: approval-mozilla-aurora?
Attachment #8383340 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.