29.51 KB, image/png
123.81 KB, image/png
Bug 1513541 - Ensure system addons are not accidentally started on start-up even if their about:config preferences are off; r?kmag
47 bytes, text/x-phabricator-request
|Details | Review|
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  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... :) : https://phabricator.services.mozilla.com/D8678#change-tgz96YwIRtPN
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.
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.
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
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
Pushed by firstname.lastname@example.org: 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
You need to log in before you can comment on or make changes to this bug.