1812235 - Massive error spike syncing credit-cards on Fenix.

Status: RESOLVED FIXED

(Fenix :: Accounts and Sync, defect)

Description

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

Mobile clients connected to a desktop that has bug 1667257 applied fail to sync credit cards. That bug bumped the sync version of the payload to 4, but the mobile clients reject anything not 3.

What's the chance of backing that out? Even if we make the same change in app-services, it will need to ride the trains, so any release or beta Fenix's will remain broken.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1667257

Comment 2

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

oops, sorry, I got the patch author wrong.

Comment 3

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

Instead of a backout, I think what is going to need to happen here is:

• Change the version back to 3.
• As a kind of migration, go through all credit-cards locally, and if they have version 4, drop it back to 3, possibly re-add the field (see below) and arrange for them to be re-synced.
• Check what happens with mobile if no cc-type is in the payload - we'll need to know if app-services itself will get upset ([edit] I think it will, so will need to be re-added as an empty string) and also check whether Fenix will get upset if it's not a "known" value (in which case I'm not sure what action to take)
• Uplift that to beta.

Comment 4

[Dimi Lee [:dimi]]

Attachment: Bug 1812235 - Rollback credit card record version from 4 to 3 r=sgalich

Mobile clients reject syncing when credit card record version is not 3, so this patch
reverts the credit card record version back to 3 (with cc-type field).

The auto-detect beahvior still exists, the only difference is now we
will still save the cc-type to storage to comply with the epectation
of mobile clients.

Comment 5

[Dimi Lee [:dimi]]

Attachment: Bug 1812235 - Rollback credit card record version from 4 to 3 r=sgalich

Mobile clients reject syncing when credit card record version is not 3, so this patch
reverts the credit card record version back to 3 (with cc-type field).

The auto-detect network type behavior implemented in bug 1667257 still applies.
The changes made in this commit are:

1. Save the cc-type to storage to comply with the expectation of mobile clients.
   (This is the behavior for v3 credit record)
2. When a v4 record is found, rollback to v3 and make sure _sync.changeCounter is set
   so we upload the downgraded record to the sync server

Differentiac Revision: https://phabricator.services.mozilla.com/D167790

Comment 6

[Dimi Lee [:dimi]]

Attachment: Bug 1812235 - Rollback credit card record version from 4 to 3 r=sgalich

Mobile clients reject syncing when credit card record version is not 3, so this patch
reverts the credit card record version back to 3 (with cc-type field).

The auto-detect network type behavior implemented in bug 1667257 still applies.
The changes made in this commit are:

1. Save the cc-type to storage to comply with the expectation of mobile clients.
   (This is the behavior for v3 credit record)
2. When a v4 record is found, rollback to v3 and make sure _sync.changeCounter is set
   so we upload the downgraded record to the sync server

Differentiac Revision: https://phabricator.services.mozilla.com/D167790

Comment 7

[Dimi Lee [:dimi]]

Attachment: Bug 1812235 - Rollback credit card record version from 4 to 3 r=markh,sgalich

Mobile clients reject syncing when credit card record version is not 3, so this patch
reverts the credit card record version back to 3 (with cc-type field).

The auto-detect network type behavior implemented in bug 1667257 still applies.
The changes made in this commit are:

1. Save the cc-type to storage to comply with the expectation of mobile clients.
   (This is the behavior for v3 credit record)
2. When a v4 record is found, rollback to v3 and make sure _sync.changeCounter is set
   so we upload the downgraded record to the sync server

Comment 8

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

FTR, release desktop is also impacted if a beta or nightly desktop is syncing with it:

1674691198473 Sync.Engine.CreditCards.Store WARN Failed to apply incoming record 4b1b1c81d4c5: Error: Got unknown record version 4; want 3(resource://autofill/FormAutofillStorageBase.jsm:321:13) JS Stack trace: [e-mail]:321:13

Comment 9

[Pascal Chevrel:pascalc (PTO until April 26)]

Will that patch require uplifting to beta?

Comment 10

[David Durst [:ddurst]]

(In reply to Pascal Chevrel:pascalc from comment #9)

Will that patch require uplifting to beta?

Yes

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d9e6839b65e
Rollback credit card record version from 4 to 3 r=markh,sgalich

Comment 12

[Narcis Beleuzu [:NarcisB]]

Backed out for xpc failure on test_sync_deprecate_credit_card_v4.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/67bc0669fc1698a804f8afa3e9f60bd5c5958176
Log link: https://treeherder.mozilla.org/logviewer?job_id=403784276&repo=autoland&lineNumber=1821

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30dcad75e37f
Rollback credit card record version from 4 to 3 r=markh,sgalich

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/30dcad75e37f

Comment 15

[Dimi Lee [:dimi]]

Hi Pascal, I'm not sure why I can't see uplift request option in the attachment, so just manually add one:

BetaUplift Approval Request

• User impact if declined: Users will not be able to sync credit cards,
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): The code only affect users who have credit card saved in firefox. Once user have migrated back to credit card version 3, the change made in this patch will not run anymore
• String changes made/needed: None

Comment 16

[Pascal Chevrel:pascalc (PTO until April 26)]

It might be because the bug is filed in the Fenix component which is on Github while the patch actually landed on m-c.

Comment 17

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

Attachment: Bug 1812235 - Rollback credit card record version from 4 to 3

Mobile clients reject syncing when credit card record version is not 3, so this patch
reverts the credit card record version back to 3 (with cc-type field).

The auto-detect network type behavior implemented in bug 1667257 still applies.
The changes made in this commit are:

1. Save the cc-type to storage to comply with the expectation of mobile clients.
   (This is the behavior for v3 credit record)
2. When a v4 record is found, rollback to v3 and make sure _sync.changeCounter is set
   so we upload the downgraded record to the sync server

Original Revision: https://phabricator.services.mozilla.com/D167814

Comment 18

[Phabricator Automation]

Uplift Approval Request

• User impact if declined: Credit card syncing broken
• Needs manual QE test: no
• Fix verified in Nightly: yes
• Code covered by automated testing: yes
• Explanation of risk level: risk limited to credit-card functionality
• Is Android affected?: no
• Risk associated with taking this patch: low
• Steps to reproduce for manual QE testing: n/a
• String changes made/needed: None

Comment 19

[Pascal Chevrel:pascalc (PTO until April 26)]

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

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 21

[Delia Pop]

Removing the qe-verify+ label since it appears that manual qe isn't needed. However, when attempting to reproduce the issue, we cannot meet the condition from 1667257 and the fix cannot be confirmed on our side.

Comment 22

[Delia Pop]

Attempted to reproduce the issue in beta 110.0b6 as well, but with the same result as above. However, Credit Card sync works as expected.