1816969 - MOZ_SERVICES_SYNC is still used in the code

Status: REOPENED

(Firefox :: Sync, defect)

Description

[Pier Angelo Vendrame]

MOZ_SERVICES_SYNC is still used in services/moz.build and in other files, such as AppConstants.jsm.
However, setting it to False in browser/moz.configure results in a build failure, because @RESPATH@/components/SyncComponents.manifest cannot be found anymore.
Restoring the #ifdef in browser/installer/package-manifest.in leads to other errors in the console and in the UI, but they are easily solvable.

Comment 1

[Pier Angelo Vendrame]

The latest nightly it builds, but the runtime/console errors are still there, and also the context menu is full of spurious items, such as video playback.
On the other hand, 102esr is broken.
Anyway, a similar patch could probably land on nightly, I will provide one.

Comment 2

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

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

Comment 3

[Pier Angelo Vendrame]

Attachment: Bug 1816969 - Fix runtime errors with MOZ_SERVICES_SYNC=False. r=mossop

Some checks on MOZ_SERVICES_SYNC have been removed in Bug 1227361, but
it is still used in a few places in the codebase.
Trying to set it to False results in some runtime errors, that this
commit fixes.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ea51bb77cfc
Fix runtime errors with MOZ_SERVICES_SYNC=False. r=mossop

Comment 5

[Pier Angelo Vendrame]

I have just noticed the patch I have sent is missing a small piece 😞️! I am really sorry about that.

diff --git a/toolkit/modules/moz.build b/toolkit/modules/moz.build
index 858bfc466ef6..26a6edad5170 100644
--- a/toolkit/modules/moz.build
+++ b/toolkit/modules/moz.build
@@ -286,6 +286,7 @@ for var in (
 
 for var in (
     "MOZ_ALLOW_ADDON_SIDELOAD",
+    "MOZ_SERVICES_SYNC",
     "MOZ_SYSTEM_NSS",
     "MOZ_SYSTEM_POLICIES",
     "MOZ_UNSIGNED_APP_SCOPE",

So, I expect it will be backed out because of tests.
Is there a way to fix it?

Comment 6

[Marian-Vasile Laza]

Backed out changeset 9ea51bb77cfc (Bug 1816969) for causing multiple services-sync bc failures, devtools and xpcshell failures.
Backout link
Push with failures
Failure Log
Also bc5
Also bc6
Also bc1
Also bc1
Also dt8
Also X3
Also ss
Also X1 ... + multiple other

Comment 7

[Pier Angelo Vendrame]

Sorry, it took me a while to get back to this.
Also, I have requested try access to run all the various tests. The new revision should not break anything 🙂️.
I had a couple of failures, but they seem known (still unresolved) bugs: https://treeherder.mozilla.org/push-health/push?repo=try&revision=cdac8f274c0dbc9d28e3d9562aaf868a5725e3ba&tab=tests.

Comment 8

[Pier Angelo Vendrame]

We found some additional errors downstream when MOZ_SERVICES_SYNC is False.
So, I will need more time to improve the patch, but I don't know how to set "planned changes" in Phabricator now.
I apologize and thank you for your patience.

Comment 9

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

The severity field is not set for this bug.
:lougenia, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 10

[Lougenia Bailey]

Hey Pier, I'm going to set the severity for this bug to S3. If that's not accurate, please feel free to change it.

Comment 11

[Pier Angelo Vendrame]

Works for me, thank you!

Comment 12

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

Its probably too late for 112 for this but wondering if there a plan to target esr102? If not, could you set that flag to wontfix?

Comment 13

[Pier Angelo Vendrame]

No, we don't have a plan to target esr102.
Thanks for asking.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Did you mean to close this bug? It looks like the patch hasn't actually landed yet? Also note that bugs are typically resolved automatically once the patch is merged to mozilla-central. Under normal circumstances, you shouldn't have to manually resolve the bug yourself.

Comment 15

[Pier Angelo Vendrame]

Oops, no, it was not my intention, I just didn't notice a resolution button remained pressed 😅️. Thanks for reopening.
The patch needs to be modified, but I haven't had time to, yet.

Comment 16

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

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:pierov, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.