1024811 - hawkclient will fail if no response headers

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

When we try and find retry related headers, the code assumes response.headers is non null - see http://hg.mozilla.org/mozilla-central/file/95cc116fac5a/services/common/hawkclient.js#l238

Note that the current tip of that file does *not* have the problem - it was fixed independently in revision fe1432e7b895.

Also worrying is the fact that the exception caused by this is not reported anywhere - we had to deduce this from other log entries - see bug 1006360 comment 61 and 62 - but I'll open yet another bug for that as trunk *is* still affected by that, just not by the "null headers" problem here.

(Note I'm not 100% sure about the status flags below - the fix is in revision fe1432e7b895 - so I *think* that means 32 and 33 are already fixed.)

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1024811-hawkclient-now-checks-if-headers-are-non.patch

This patch checks we have headers before attempting to use them, and also fixes a use of the log.  This patch is against beta - the same patch landed on central in revision fe1432e7b895 as part of bug 988469 and is already in Aurora.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Marco, please add this to the current iteration

Comment 3

[Marco Mucci [:MarcoM]]

Added to Iteration 33.1

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8439637 [details] [diff] [review]
0001-Bug-1024811-hawkclient-now-checks-if-headers-are-non.patch

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

Looks sane to me.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8439637 [details] [diff] [review]
0001-Bug-1024811-hawkclient-now-checks-if-headers-are-non.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FxA sync
User impact if declined: Sync may work incorrectly on some network errors related to auth.
Testing completed (on m-c, etc.): This same patch has already landed on m-c and Aurora via bug 988469 - this patch is to add the same code to beta.
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

As above, this needs to land on beta only - central and aurora already have the fix.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ffa808d00464

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Landed on 31, which is the only place it needed to, so fixed.