Closed Bug 1395535 Opened 3 years ago Closed 2 years ago

[dt-addon] Find a workaround for system addon installation issues in local builds

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox57 fix-optional)

RESOLVED INVALID
Tracking Status
firefox57 --- fix-optional

People

(Reporter: jdescottes, Unassigned)

References

Details

Attachments

(4 files)

Before landing Bug 1369801 we need to make sure that DevTools will be installed and enabled properly when a profile moves from a Firefox having regular DevTools built-in to a Firefox that has DevTools as system addon.

Beta and Release channels: In case the application version changes at the same time, system addons are automatically installed, so this is OK.

Nightly Channel: we are currently implementing a check based on the build ID in Bug 1391187. This will address issues with Nightly.

However when performing a local build (at least with artifact builds) the build ID is not updated. So we need another solution to make sure developers don't have to restart Firefox in order to get DevTools.
I see several options here. 

We have two preferences we can play with to force a scan:
- extensions.startupScanScopes: can be set to any of the scopes defined at http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/toolkit/mozapps/extensions/AddonManager.jsm#3420-3431 to force a scan
- extensions.lastAppBuildId: this is the preference introduced by Bug 1391187. It is used to check if the build id is different from last time, and if it is different, will force a scan.

One solution would be to force one of those prefs to a particular value for local builds for the remainder of 57 (and remove the hack as soon as 57->58). 

A second solution is to land both Bug 1369801 and Bug 1391187 at the same time. Since Bug 1391187 introduces the extensions.lastAppBuildId pref, it will also force a scan.

Alex, Brian: opinions on this? I only just thought about the second option and I think that's probably the cleanest thing we can do. (which also means that we can close this Bug as is :) )
Flags: needinfo?(poirot.alex)
Flags: needinfo?(bgrinstead)
Would it still break devtools while going back and forth between FF57 changesets (before/after the landing). Otherwise, it seems to make things at-risj if one or the other get backed out.

Could we programatically force a startup scan?
What about forcing one from devtools-startup if we detect we are on a local/nightly build?
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #2)
> Would it still break devtools while going back and forth between FF57
> changesets (before/after the landing). Otherwise, it seems to make things
> at-risj if one or the other get backed out.
> 

I thought about the backout issue, but my plan would include moving the changeset to the parent bug (things would be backed out together).

Regarding going back and forth ... yes that an issue with most of those preferences based solutions. Works great when going forward, not so much when jumping back and forth. 

> Could we programatically force a startup scan?
> What about forcing one from devtools-startup if we detect we are on a
> local/nightly build?

We could set the pref from devtools-startup. It does mean that we will always perform a scan for local builds. I think people that care the most about those prefs are the maintainers of addonmanager.jsm and xpiprovider.jsm. If for some reason that have to debug startup scan issues and have to realize that DevTools is resetting a pref that impacts their codebase, I think it will be pretty unsettling for them. 

So -> "why not", but I wouldn't do it from devtools-startup and rather do it from the addonmanager's code. That actually fits my solution one, except I didn't have any technical solution in mind.
Discussed this with Alex again, let's try to address this in a similar fashion to Bug 1391187.
This means we would programmatically force a scan in case we are on a local build. This should be done in the XPIProvider.
I'm completely stuck trying to find a good way to detect local "artifact" builds. Everything that relies on preprocessing cannot be used here:
- Services.appinfo.isOfficial is true for a local artifact build
- AppConstants.NIGHTLY_BUILD is true for a local artifact build

It seems like the only reliable way that works for both aritfact and non artifact builds is:

> const IS_UNOFFICIAL_BUILD = Services.prefs.getStringPref("app.update.channel") === "default";
(In reply to Julian Descottes [:jdescottes] from comment #1)
> I see several options here. 
> 
> We have two preferences we can play with to force a scan:
> - extensions.startupScanScopes: can be set to any of the scopes defined at
> http://searchfox.org/mozilla-central/rev/
> 51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/toolkit/mozapps/extensions/
> AddonManager.jsm#3420-3431 to force a scan
> - extensions.lastAppBuildId: this is the preference introduced by Bug
> 1391187. It is used to check if the build id is different from last time,
> and if it is different, will force a scan.
> 
> One solution would be to force one of those prefs to a particular value for
> local builds for the remainder of 57 (and remove the hack as soon as
> 57->58). 
> 
> A second solution is to land both Bug 1369801 and Bug 1391187 at the same
> time. Since Bug 1391187 introduces the extensions.lastAppBuildId pref, it
> will also force a scan.
> 
> Alex, Brian: opinions on this? I only just thought about the second option
> and I think that's probably the cleanest thing we can do. (which also means
> that we can close this Bug as is :) )

Is this patch actually a 'workaround' for the system addon installation issues or does it fix the problem?  I'm not sure how this is related to Bug 1369801 and Bug 1391187 - can this code be removed once those are fixed?
Flags: needinfo?(bgrinstead)
Bug 1369801 is blocked by this bug and by Bug 1391187. Bug 1369801 switches DevTools to a system addon. This doesn't work well in Nightly and in Local builds. Bug 1391187 introduces a fix/workaround for Nightly. This bug introduces a fix/workaround for Local builds.

The code for both this bug and Bug 1391187 can be removed once the sideload bug lands: Bug 1386295.

(another potential solution/duplicate of Bug 1386295 is Bug 1348981, but I don't know enough to say which one makes more sense here)
> I'm not sure how this is related to Bug 1369801 and Bug 1391187 

The reason why landing Bug 1369801 and Bug 1391187 together would have "improved" the situation for Local builds is that Bug 1391187 is about checking the app buildid to force a directory scan. 

Comparing build ids does not work with a local artifact build, since the build ID is always the same. 

But to check this Bug 1391187 introduces a new preference. Which means that the first time it runs, it will be "as if" the build ID changed since the preference is "new".
Priority: -- → P3
Comment on attachment 8903647 [details]
Bug 1395535 - trigger addon reload from devtools-startup if devtools addon failed to start;

https://reviewboard.mozilla.org/r/175420/#review181102

I'm on the fence about this, but I've been considering it for the last few days, and I think I'd prefer local builds to behave as similarly to production builds as possible by default. In the odd case where developers need to add a new system extension, they can pass `--setpref extensions.startupScanScopes=15` for one run. And when rhelmer finishes his work to store the manifest of built-in system extensions in omni.ja, even that should no longer be necessary.
Attachment #8903647 - Flags: review?(kmaglione+bmo) → review-
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #11)
> Comment on attachment 8903647 [details]
> Bug 1395535 - force directory scan on local builds to discover new system
> addons;
> 
> https://reviewboard.mozilla.org/r/175420/#review181102
> 
> I'm on the fence about this, but I've been considering it for the last few
> days, and I think I'd prefer local builds to behave as similarly to
> production builds as possible by default. In the odd case where developers
> need to add a new system extension, they can pass `--setpref
> extensions.startupScanScopes=15` for one run. And when rhelmer finishes his
> work to store the manifest of built-in system extensions in omni.ja, even
> that should no longer be necessary.

Thanks for the feedback, although I'm not sure how to progress from here :(

> In the odd case where developers need to add a new system extension

With DevTools as a system addon, I think it will happen quite often. Say we switch DevTools to a system addon in changeset `C`. Developers moving before and after `C` will repeatedly hit this problem. It's not just about devs working on DevTools, everybody working on Firefox should use `--setpref extensions.startupScanScopes=15` for the duration of the 57 development if they don't want to be plagued with missing DevTools while working.

Kris: we could remove the code here as soon as we go to 58, if that helps? Any other solution that could help us make it for 57?
Flags: needinfo?(kmaglione+bmo)
The only time this should ever need to happen is when a built-in system add-on is added or removed. For developers not working on the feature, it shouldn't be an issue except the first time the add-on is shipped, and we can always add a clobber for that.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #13)
> For developers not working on the feature, it shouldn't be an issue except
> the first time the add-on is shipped, and we can always add a clobber for
> that.

(Or just bump the add-ons DB schema version)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #13)
> The only time this should ever need to happen is when a built-in system
> add-on is added or removed. For developers not working on the feature, it
> shouldn't be an issue except the first time the add-on is shipped, and we
> can always add a clobber for that.

Forcing a clobber is not enough. With all patches from Bug 1369801 + a CLOBBER file modification as `tip`
- hg up central && ./mach build && ./mach run -P testprofile => DevTools start
- hg up tip && ./mach build && ./mach run -P testprofile => DevTools start
- hg up central && ./mach build && ./mach run -P testprofile => DevTools start
- hg up tip && ./mach build && ./mach run -P testprofile => No DevTools (addon is installed, not started, see Bug 1386295)
(and after a restart DevTools work because the addon's startup method is called)

Basically for a given profile, only the first time we attempt to install+start the addon is successful, after that going back and forth around the changeset that moves DevTools as a system addon triggers the problem.

The patch attached here fixes this issue. 

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #14)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #13)
> > For developers not working on the feature, it shouldn't be an issue except
> > the first time the add-on is shipped, and we can always add a clobber for
> > that.
> 
> (Or just bump the add-ons DB schema version)

Are you talking about extensions.databaseSchema ? Can I update it programmatically before the AddonManager/XPIProvider needs to check it?
Kris: by email you mentioned that a solution to enable the addon could be "The easiest way would probably be executing something in the browser console." So I'm trying to do more or less that. In our command line handler component, I'm checking if devtools' resource:// path has been registered and if not I'm trying to "enable" the addon. Here is the code I currently have for this:

>     const { AddonManager } = Components.utils.import("resource://gre/modules/AddonManager.jsm", {});
>     AddonManager.getAddonByID("devtools@mozilla.org", (addon) => {
>       addon.enabled = true;
>       addon.active = true;
>       addon.userDisabled = false;
>     });

But the addon startup is still not called. Tips on how to enable an addon programmatically? 
Note that this would not be a great solution, as it introduces async code in a path which is purely synchronous right now. I would really prefer us to fix this so that the addon correctly starts.
Flags: needinfo?(kmaglione+bmo)
Some context about the 4 patches I just uploaded here. First of all they are just workarounds to improve a bit the situation if we can't do anything better on the addon side. I would wait until we hear from Kris before spending too much time on the review but feel free to give me overall feedback in the meantime.

The first patch is pretty straightforward and accpetable. We are just trying to dynamically reload() the addon if it looks like it still hasn't started when we reach "initDevTools". This is done async, so it won't actually fix anything but: if a user pressed a keyboard shortcut they will see an error message in the console and will be prompted to try again. If they just try the keyboard shortcut a second time it will work. (same for inspect node etc...)

The next 3 patches are about introducing async to initDevTools. Depending on the code paths, we can't always handle async properly. The situation with my current patches is that: 
- keyshortcuts, inspect node, hamburger menu will work async
- system menu will only contain "Page Source" if that is the first way users are interacting with DevTools
- command line arguments won't work (but there will be an error message logged, explaining that a pref can be used to force a scan etc...)

Also let's keep in mind this in only relevant with STRs similar to comment 15 . And that you might want to clean the startup cache between two tests (see Bug 1389502, not sure about the impact of not doing so but the STRs are complicated enough, I don't want to add another set of bugs in the mix to investigate).

Maybe that's too much given how edge-case-y this is, we will discuss that later today during the gofaster checkpoint.
Comment on attachment 8903647 [details]
Bug 1395535 - trigger addon reload from devtools-startup if devtools addon failed to start;

https://reviewboard.mozilla.org/r/175420/#review212784

Resetting r? as I'm not sure we want to proceed with this?
Please r? again if you want to move forward with this.
Attachment #8903647 - Flags: review?(poirot.alex)
Comment on attachment 8904556 [details]
Bug 1395535 - initDevTools is async for scenarios where devtools addon did not start;

https://reviewboard.mozilla.org/r/176402/#review212786
Attachment #8904556 - Flags: review?(poirot.alex)
Comment on attachment 8904557 [details]
Bug 1395535 - update DevToolsShim.isInstalled from addon install() method;

https://reviewboard.mozilla.org/r/176404/#review212788
Attachment #8904557 - Flags: review?(poirot.alex)
Comment on attachment 8904558 [details]
Bug 1395535 - wait for async initDevTools in devtools shim inspectNode method;

https://reviewboard.mozilla.org/r/176406/#review212790
Attachment #8904558 - Flags: review?(poirot.alex)
Closing as invalid for now.
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(kmaglione+bmo)
Resolution: --- → INVALID
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.