Closed Bug 1333431 Opened 3 years ago Closed 3 years ago

ERROR_PARSE thrown on 304 response when fetching profile

Categories

(Firefox :: Sync, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: eoger, Assigned: mayank, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(2 files, 1 obsolete file)

See https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsProfileClient.jsm#160

If we get a 304 response (not modified) to a /profile request, the server will not send a body and we'll try to parse it anyway which makes us throw an ERROR_PARSE exception.
It is not a big problem because we actually catch the exception here https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsProfile.jsm#107 and handle the 304 case specifically, but it isn't semantically correct to consider this as a parsing exception.
See Also: → 1332993
Mentor: eoger
Keywords: good-first-bug
(In reply to Edouard Oger [:eoger] from comment #0)
> See
> https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/
> FxAccountsProfileClient.jsm#160
> 
> If we get a 304 response (not modified) to a /profile request, the server
> will not send a body and we'll try to parse it anyway which makes us throw
> an ERROR_PARSE exception.
> It is not a big problem because we actually catch the exception here
> https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/
> FxAccountsProfile.jsm#107 and handle the 304 case specifically, but it isn't
> semantically correct to consider this as a parsing exception.

Hi, I am trying to work on this bug.
So, what you mean to say is, should we check if body is not NULL in the try block itself, and only proceed with parsing if body is not NULL?

Thanks
What you could do instead is directly compare the value of request.response.status before even accessing or parsing the body.

I am not really a big fan of using exceptions as control flow, so I think what we could do now is returning a null object (which is not really beautiful too, but a bit better).
You will also have to update the JSdoc in FxAccountsProfileClient to explain that we may return null if we hit a 304 and add a comment explaining why are you comparing to null in fetchAndCacheProfileInternal, so people don't get burned by this.
Finally, you will have to add/update some tests in test_profile_client.js and test_profile.js.
(In reply to Edouard Oger [:eoger] from comment #2)
> What you could do instead is directly compare the value of
> request.response.status before even accessing or parsing the body.
> 
> I am not really a big fan of using exceptions as control flow, so I think
> what we could do now is returning a null object (which is not really
> beautiful too, but a bit better).
> You will also have to update the JSdoc in FxAccountsProfileClient to explain
> that we may return null if we hit a 304 and add a comment explaining why are
> you comparing to null in fetchAndCacheProfileInternal, so people don't get
> burned by this.
> Finally, you will have to add/update some tests in test_profile_client.js
> and test_profile.js.

I have made changes in the code. I am new to this, having trouble in updating/writing tests. Could you please guide on this?

Thanks.
Flags: needinfo?(eoger)
For test_profile, I would suggest creating a new test fetchAndCacheProfile_alreadyCached based on fetchAndCacheProfile_sendsETag.
Instead of resolving an object in the client.fetchProfile mock, you resolve null, then you must check that you do get a result in _fetchAndCacheProfile() BUT that you never called FxAccountsProfile._cacheProfile (because you never had a response body).

For test_profile_client, you can create a test based on successfulResponse called successful304Response (or something like this), where the response object doesn't have a body but the |status| field is set to 304. Then you just have to check that fetchProfile() resolves to null.

Good luck!
Assignee: nobody → mayanksri18
Status: NEW → ASSIGNED
Flags: needinfo?(eoger)
Priority: -- → P3
Attached patch bug-1333431.patch (obsolete) — Splinter Review
Attaching patch. Please review
Thanks
Attachment #8834510 - Flags: review?(eoger)
Comment on attachment 8834510 [details] [diff] [review]
bug-1333431.patch

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

Good job overall Mayank! This needs a little bit more work but the main approach is good here.
Don't be discouraged by the number of review comments, most of them are pretty easy to fix (< 2 minutes).
I'll be waiting on your next patch iteration :)

::: services/fxaccounts/FxAccountsProfile.jsm
@@ +99,5 @@
>          return this.client.fetchProfile(etag);
>        })
>        .then(response => {
> +        // The response could be null if profile
> +        // is not modified

Can you reword that a little bit, explaining that the profile is not modified because we sent an etag?

@@ +101,5 @@
>        .then(response => {
> +        // The response could be null if profile
> +        // is not modified
> +        if (response != null) {
> +          return this._cacheProfile(response);

I would be ok just returning with a ternary statement like return response ? this._cacheProfile(response) : null;

@@ +107,3 @@
>        })
>        .then(body => { // finally block
>          onFinally();

Add some comment here saying body could be null, better safe than sorry I guess!

::: services/fxaccounts/FxAccountsProfileClient.jsm
@@ +127,4 @@
>     * @param {String} token
>     * @param {String} etag
>     * @return Promise
> +   *         Resolves: {body: Object, etag: Object} Successful response from the Profile server or null if 304 is hit.

Ditto, can you reword that a little bit to imply that 304 is hit because we're sending an etag?

@@ +154,5 @@
>  
>          let body = null;
>          try {
> +          if (request.response.status == 304) {
> +            return resolve({

You are still resolving an object here, not null, which means you will never hit that line you added on FxAccountsProfile.jsm:104

::: services/fxaccounts/tests/xpcshell/test_profile.js
@@ +455,5 @@
>  }
> +
> +add_test(function fetchAndCacheProfile_alreadyCached() {
> +  let fxa = mockFxa();
> +  fxa.profileCache = { profile: {}, etag: "bogusETag" };

Add some fake profile object (eg. profile: { avatar: cachedUrl }), and check that it's still the same after calling _fetchAndCacheProfile() before run_next_test();

@@ +461,5 @@
> +  client.fetchProfile = function(etag) {
> +    do_check_eq(etag, "bogusETag");
> +    return Promise.resolve({ body: null });
> +  };
> +  let profile = CreateFxAccountsProfile(fxa, client);

You have to check that you never called FxAccountsProfile._cacheProfile, which means adding a mock of this method and throwing/failing the test if it's called. Check fetchAndCacheProfile_ok to see how the mock is set-up.

@@ +465,5 @@
> +  let profile = CreateFxAccountsProfile(fxa, client);
> +
> +  return profile._fetchAndCacheProfile()
> +    .then(result => {
> +      run_next_test();

You should check that result is null here

::: services/fxaccounts/tests/xpcshell/test_profile_client.js
@@ +437,5 @@
>    }
>    throw new Error("Validation helper error");
>  }
> +
> +add_test(function successful304Response() {

Good test!

@@ +442,5 @@
> +  let client = new FxAccountsProfileClient(PROFILE_OPTIONS);
> +  let response = {
> +    success: true,
> +    status: 304,
> +    headers: { etag:"bogusETag" },

Nit: add a space between etag: and the value
Attachment #8834510 - Flags: review?(eoger)
Attaching the updated patch. Please review.
Thanks
Attachment #8834510 - Attachment is obsolete: true
Attachment #8834997 - Flags: review?(eoger)
Comment on attachment 8835045 [details]
Bug 1333431 - Fixed ERROR_PARSE thrown on 304 response when fetching profile.

https://reviewboard.mozilla.org/r/110738/#review112110
Attachment #8835045 - Flags: review?(eoger) → review+
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81e2137205f1
Fixed ERROR_PARSE thrown on 304 response when fetching profile. r=eoger
Thank you Mayank. I corrected some nitpicks on your patch and submitted it through autoland, let's see if that sticks!
https://hg.mozilla.org/mozilla-central/rev/81e2137205f1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
(In reply to Edouard Oger [:eoger] from comment #11)
> Thank you Mayank. I corrected some nitpicks on your patch and submitted it
> through autoland, let's see if that sticks!

Thanks Edouard!
Comment on attachment 8834997 [details] [diff] [review]
bug-1333431.patch

Forgot to clear the review flag on that one
Attachment #8834997 - Flags: review?(eoger) → review+
You need to log in before you can comment on or make changes to this bug.