1622012 - Never run Marionette code if Marionette is not enabled

Status: VERIFIED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

If Marionette hasn't been enabled we shouldn't run any of its code. Right now this happens because the component registers the profile-after-change observer notification. And as such we register for various other notifications, and in the worst case quit Firefox if eg. a XML parse error happens. See bug 1616856 for details.

I would suggest that we read the value of the marionette.enabled pref only during startup, and never update it when a user changes the pref during runtime. That way we can safely skip all the observer registrations, and don't have to worry about inconsistent states.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1622012 - [marionette] Read the "marionette.enabled" pref only during startup, and don't allow to change its value.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1622012 - [marionette] Only set environment prefs if Marionette is enabled.

Depends on D67035

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1622012 - [marionette] Only initialize Marionette during startup if explicitely enabled.

Depends on D67036

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc0a548be86d640d15ddfbf90b1bf6058d9de6cf

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0656956fe08b
[marionette] Read the "marionette.enabled" pref only during startup, and don't allow to change its value. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/5d11cda6e6ad
[marionette] Only set environment prefs if Marionette is enabled. r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/cd3e1f0bdeeb
[marionette] Only initialize Marionette during startup if explicitely enabled. r=marionette-reviewers,maja_zf

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0656956fe08b
https://hg.mozilla.org/mozilla-central/rev/5d11cda6e6ad
https://hg.mozilla.org/mozilla-central/rev/cd3e1f0bdeeb

Comment 7

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

The patch landed in nightly and beta is affected.
:whimboo, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks for the reminder. We should indeed uplift this test-patch to beta.

Comment 9

[:Gijs (he/him)]

Could we safely uplift this to 75?

Comment 10

[:Gijs (he/him)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #8)

Thanks for the reminder. We should indeed uplift this test-patch to beta.

Ah, commented before I saw this - I think you normally need to request beta approval if you want uplift?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Yes you are right. Just clarified with RyanVM on Matrix.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I just went through all the options for enabling Marionette, and I can verify with a Nightly build that only with those set Marionette will run.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 9133671 [details]
Bug 1622012 - [marionette] Only initialize Marionette during startup if explicitely enabled.

Beta/Release Uplift Approval Request

• User impact if declined: Marionette registers some observer notifications during startup even when it hasn't been enabled. As result it will force close Firefox in case of XML parsing errors.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Disabling the registration of observer notifications won't cause Marionette to get partially initialized.
• String changes made/needed: None

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 9133671 [details]
Bug 1622012 - [marionette] Only initialize Marionette during startup if explicitely enabled.

approved for 75.0b10

Comment 15

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4dbdf745dd8c
https://hg.mozilla.org/releases/mozilla-beta/rev/847777a9208c
https://hg.mozilla.org/releases/mozilla-beta/rev/acafefc48432