Report Site Issue button enabled for some release population users?



5 months ago
11 days ago


(Reporter: miketaylr, Assigned: twisniewski)


(Depends on 1 bug)

Dependency tree / graph

Firefox Tracking Flags

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



(3 attachments)



5 months ago
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... :)

Flags: needinfo?(twisniewski)

Comment 3

5 months ago
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)

Comment 4

5 months ago
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: Working on it now.

Comment 5

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

Comment 6

5 months ago
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

Comment 7

5 months ago
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

5 months ago
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)

Comment 9

5 months ago
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):

Comment 15

4 months ago
Pushed by
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.

Comment 17

4 months ago
Last Resolved: 4 months 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.