1513541 - Report Site Issue button enabled for some release population users?

Status: RESOLVED FIXED

(Web Compatibility :: Tooling & Investigations, defect, P1)

Description

[Mike Taylor [:miketaylr]]

Attachment: about:telemetry experiements, from an affected build

I cannot reproduce in a clean, new profile, but it appears there are some users out there who have this enabled (judging by the reports coming in).

We need to get it turned off, and figure out why it's on in the first place.

Things we knows: 

* extensions.webcompat-reporter.enabled is set to false, but toggling it to true, then false again actually disables it.
* this does not appear to be a SHIELD recipe gone wrong (cf. Bug 1488931)

Comment 1

[Mike Taylor [:miketaylr]]

Attachment: release w/ relevant prefs shown

Comment 2

[Dennis Schubert [:denschub]]

I can only reproduce with a local optimized release build, and also only when my CPU and my GPU are busy. But then, the "report site issue" button pops up.

To my untrained eye, it almost looks like we are disabling the addon too late. This change [0] looks interesting to me: Tom, why did you decide to not have the function called immediately? Do we have any guarantee that the observer will fire at all/in time? Just brainstorming on what the issue could be here... :)

[0]: https://phabricator.services.mozilla.com/D8678#change-tgz96YwIRtPN

Comment 3

[Thomas Wisniewski [:twisniewski]]

There were failures in checking that early, since the addon wasn't guaranteed to be ready to be enabled/disabled at that point. That's why I moved the startup-check into the other patch-hunk in XPIDatabase.jsm, and kept just the observer where it is.

But if your theory is right and we have a race condition, then perhaps we will need to also start the observer at the same place in XPIDatabase.jsm, or perhaps there is something else going on that I don't really understand. What do you think, aswan?

Comment 4

[Mike Taylor [:miketaylr]]

As a temporary hack, and maybe to buy us time so we can wait for a dot release ride along, I'm going to try to head off any new reports on the server side: https://github.com/webcompat/webcompat.com/issues/2733. Working on it now.

Comment 5

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1513541 - Ensure system addons are not accidentally started on start-up even if their about:config preferences are off; r?kmag

Also sync system addon enabled/disabled state with its pref during updateAddonMetadata

Comment 6

[Thomas Wisniewski [:twisniewski]]

I haven't yet figured out how to test this, but this is the direction that the addons team and I discussed we should take on IRC. Basically, the check for whether to enable the addon or not must be done in a second place in addition to addMetadata.

I also found bug 1513617 with Screenshots' usage of nsBrowserGlue, and if that's fixed it might also benefit from this patch.

Comment 7

[Thomas Wisniewski [:twisniewski]]

I've updated the patch after a fresh discussion on IRC, but I'm still writing tests so it's not quite ready yet.

Comment 8

[Thomas Wisniewski [:twisniewski]]

A quick update: I'm a bit stuck on tests, and have asked aswan for an assist to find out what I'm doing wrong. Assuming I'm not far off, I'm nearly done.

Comment 9

[Thomas Wisniewski [:twisniewski]]

Alright, I've managed to push the patch to a point where aswan feels the ball may be in his court now, due to a concern I'm not familiar with (though he may pass it back if he feels he doesn't have the time to get this across the finish line soon enough).

Comment 10

[Andrew Swan [:aswan]]

I sent Thomas some patches offline

Comment 11

[Thomas Wisniewski [:twisniewski]]

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

Comment 12

[Thomas Wisniewski [:twisniewski]]

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

Comment 13

[Thomas Wisniewski [:twisniewski]]

Alright, a try-run of the latest patch seems fine to me, so I've requested a review from kmag (as aswan suggested).

Comment 14

[Thomas Wisniewski [:twisniewski]]

And one last try-push of the patch, as adjusted for review (just to be extra cautious): https://treeherder.mozilla.org/#/jobs?repo=try&revision=35ff35fa25f1248bdf7843fddfb867c955b2749f

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/054b7887f985
Ensure system addons are not accidentally started on start-up even if their about:config preferences are off; r=kmag

Comment 16

[Thomas Wisniewski [:twisniewski]]

The patch just received r+ conditional on a minor change, which aswan approved on IRC. Given that the previous try-run was fine, I've gone ahead and queued the final patch for landing with lando after a local test.

Comment 17

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/054b7887f985

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Given that this isn't a new issue and the circumstances to hit it seem a bit edge-case, I think it'd be better to let this fix ride the trains. Feel free to nominate the patch for uplift if you feel strongly otherwise, however.