Closed Bug 1513541 Opened 2 years ago Closed 2 years ago

Report Site Issue button enabled for some release population users?

Categories

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

defect

Tracking

(firefox64+ wontfix, firefox65+ wontfix, firefox66+ fixed)

RESOLVED FIXED
Tracking Status
firefox64 + wontfix
firefox65 + wontfix
firefox66 + fixed

People

(Reporter: miketaylr, Assigned: twisniewski)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

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)
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
Flags: needinfo?(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?
Flags: needinfo?(twisniewski) → needinfo?(aswan)
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.
Also sync system addon enabled/disabled state with its pref during updateAddonMetadata
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.
Flags: needinfo?(aswan)
See Also: → 1513617
I've updated the patch after a fresh discussion on IRC, but I'm still writing tests so it's not quite ready yet.
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.
Flags: needinfo?(aswan)
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).
I sent Thomas some patches offline
Flags: needinfo?(aswan)
Assignee: nobody → twisniewski
Attachment #9030851 - Attachment description: Bug 1513541 - Also sync system addon enabled/disabled state with its pref during updateAddonMetadata; r?aswan → Bug 1513541 - Ensure system addons are not accidentally started on start-up even if their about:config preferences are off; r?kmag

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

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

Pushed by twisniewski@mozilla.com:
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

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.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Depends on: 1521083

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.

You need to log in before you can comment on or make changes to this bug.