1818349 - TPS - HTTP POST send empty body to /storage/tabs?batch=true&commit=true

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Clare So]

One of the Firefox iOS tests, test_sync_tabs_from_desktop, passed the last time on Jan 21, 2023. This set of tests uses TPS for synchronizing data between the browsers (desktop & mobile).

In particular, an HTTP POST to /tabs?batch=true&commit=true is supposed to contain a body according to the sync log:

1674321037934 Sync.Collection DEBUG POST success 200 https://stage.sync.nonprod.cloudops.mozgcp.net/1.5/6562162/storage/tabs?batch=true&commit=true
1674321037934 Sync.Collection TRACE POST body: {"modified":1674321037.89,"success":["KxsGIwX0dtLt"],"failed":{}}

After the date, the same HTTP POST call does not include a body:

1675789923800 Sync.Collection DEBUG POST fail 412 https://stage.sync.nonprod.cloudops.mozgcp.net/1.5/6562821/storage/tabs?batch=true&commit=true
1675789923800 Sync.Collection TRACE POST body:
1675789923800 Sync.Collection WARN POST request to https://stage.sync.nonprod.cloudops.mozgcp.net/1.5/6562821/storage/tabs?batch=true&commit=true failed with status 412

The test in question is supposed to synchronize a tab from Firefox desktop to the server
var tabs1 = [
{ uri: "http://example.com/",
profile: "Fennec on iOS"
}
];
See https://github.com/clarmso/firefox-ios/blob/main/SyncIntegrationTests/test_tabs_desktop.js#L14 for the js file used by TPS.

Here is the test result from the last time the test passed.
https://storage.googleapis.com/mobile-reports/public/firefox-ios-M1/result_385/SyncIntegrationTests/results/index.html

Here is a test result describing the failure.
https://storage.googleapis.com/mobile-reports/public/firefox-ios-M1/result_417/SyncIntegrationTests/results/index.html

Comment 1

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

Sammy, can you please have a look at this? A good first step is probably to ensure that test works on desktop TPS.

Comment 2

[Sammy Khamis [:skhamis]]

Attachment: Log from the TPS test

Tried to run TPS on the latest `bookmarks/central` and doesn't seem to be passing -- though unclear why at this moment:

```

Comment 3

[Sammy Khamis [:skhamis]]

Tried to run TPS on the latest bookmarks/central and doesn't seem to be passing -- though unclear why at this moment:

TEST-UNEXPECTED-FAIL | test_tabs.js | [phase phase2] RunNextTestAction failed - Error: ASSERTION FAILED! error locating tab(resource://tps/logger.sys.mjs:78:11) JS Stack trace: [e-mail]:78:11

	phase1: PASS
	phase2: FAIL
	phase3: unknown

This was from test_tabs.js but running all the TPS tests is showing that failure is occurring in various other tests that verify tabs

TEST-UNEXPECTED-FAIL | test_bug535326.js | [phase phase2] RunNextTestAction failed - Error: ASSERTION FAILED! error locating tab(resource://tps/logger.sys.mjs:78:11) JS Stack trace: [e-mail]:78:11

	phase1: PASS
	phase2: FAIL

TEST-UNEXPECTED-FAIL | test_bug546807.js | [phase phase2] RunNextTestAction failed - Error: ASSERTION FAILED! tab found which was expected to be absent(resource://tps/logger.sys.mjs:78:11) JS Stack trace: [e-mail]:78:11

	phase1: PASS
	phase2: FAIL

TEST-UNEXPECTED-FAIL | test_privbrw_tabs.js | [phase phase2] RunNextTestAction failed - Error: ASSERTION FAILED! error locating tab(resource://tps/logger.sys.mjs:78:11) JS Stack trace: [e-mail]:78:11

	phase1: PASS
	phase2: FAIL
	phase3: unknown
	phase4: unknown

Will look deeper into this but figured I'd post preliminary research

Comment 4

[Sammy Khamis [:skhamis]]

So it turns out that this is regressed by 1800186. I opened an issue in a-s https://github.com/mozilla/application-services/issues/5404 to resolve this.

Note: this is only for test_tabs.js failing, turns out the other tests have been failing for a separate reason and probably doesn't affect the firefox-ios workflow.

The summary is: We try to store the client data in a DB that doesn't exist and thus when trying to retrieve tabs, we won't be able to find any "recent" client data, thus failing the tests. This only occurs in specific scenarios like TPS tests and possibly new user first sync (which will be corrected after subsequent syncs).

We'll need to get the fix in application-services and then vendor it into moz-central. However this will most likely not be done till early next week.

For now :markh and I found a workaround where we could add an extra action to https://searchfox.org/mozilla-central/source/services/sync/tests/tps/test_tabs.js#40-42 to phase 2 & phase 3 so it instead does [Sync], [Sync] the tests will start passing. @Clare, if y'all have access to change those tests -- that would temporarily unblock you faster. The hack can be removed once the official fix is linked.

Comment 5

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

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

Comment 6

[Clare So]

Yes, I've access to the .js file required for the test
https://github.com/mozilla-mobile/firefox-ios/blob/main/SyncIntegrationTests/test_tabs_desktop.js#L21

As a workaround, I created "phase2" in the following manner.

var phases = { "phase1": "profile1", "phase2": "profile1" };

(scroll down...)

// sync and verify tabs
Phase("phase1", [
  [Tabs.add, tabs1],
  [Sync]
]);

Phase("phase2", [
  [Sync]
  [Tabs.verify, tabs1],
  [Sync]
]);

Would such a workaround fail when the fix is in?

Comment 7

[Sammy Khamis [:skhamis]]

If that test is now passing it should continue to pass after the fix is in! It's just an "extra" sync that is needed before the verify. Though I would've expected phase2 to need:

[Sync], [Sync], [Tabs.verify, tabs1], [Sync]

but either way I hope to have the actual fix landed early this week!

Comment 8

[Sammy Khamis [:skhamis]]

Attachment: Bug 1818349 Part 1: Vendor new version of application-services r?markh

Comment 9

[Sammy Khamis [:skhamis]]

Attachment: Bug 1818349 Part 2: Update tabs TPS tests to allow using data: schema r?markh

Depends on D171181

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57b2cda7e0db
Part 1: Vendor new version of application-services r=markh
https://hg.mozilla.org/integration/autoland/rev/b05343a5b533
Part 2: Update tabs TPS tests to allow using data: schema r=markh

Comment 11

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

:skhamis Hi, could you set a severity on this issue so we can better understand impact for 111? Do we want to backport to 111?

Comment 12

[Sammy Khamis [:skhamis]]

S3 due to it happening only to users where no DB was created (first sync ever on device) but would be fixed in subsequent syncs. I believe we're confident this fixed it, however unsure of impact right now so not really sure if it's worth backporting or not.

Comment 13

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/57b2cda7e0db
https://hg.mozilla.org/mozilla-central/rev/b05343a5b533

Comment 14

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

The patch landed in nightly and beta is affected.
:skhamis, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox111 to wontfix.

For more information, please visit auto_nag documentation.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 1819723.
https://hg.mozilla.org/mozilla-central/rev/2147c5c81edf9cd56c19e7e92dfebfcd77329978

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3326192e8db6
Part 1: Vendor new version of application-services r=markh
https://hg.mozilla.org/integration/autoland/rev/021c18645209
Part 2: Update tabs TPS tests to allow using data: schema r=markh

Comment 17

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/3326192e8db6
https://hg.mozilla.org/mozilla-central/rev/021c18645209