1391187 - Compare build id to perform a full startup directory scan

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Julian Descottes [:jdescottes]]

This came up in a discussion about Bug 1369801 (ship DevTools as a system addon). When we first landed it, the DevTools system addon failed to install on the first startup for Nightly users. 

A full directory scan is performed every time the app version changes, but not when the build id changes. This means that for different Nightly builds of the same Firefox release, the full directory scan will not be performed and new system addons won't be correctly installed and started.

I assume we are talking about the code located at http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/toolkit/mozapps/extensions/AddonManager.jsm#792

The goal would be to rely on the build id rather than the app version here.

Comment 1

[Robert Helmer [:rhelmer]]

We discussed this a while back (in the context of beta builds, but relying on build ID was the same thought) in bug 1273707.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1391187 - compare build id to perform full directory scan in addon manager;

Review commit: https://reviewboard.mozilla.org/r/170062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170062/

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 8898704 [details]
Bug 1391187 - compare build id to perform full directory scan in addon manager;

I need some help with the current patch. I tried adding support for the build id and comparing this instead of the app version, but the issue is that if I do this the "addon compatibility check" dialog will popup every time users update Nightly.

I think we would prefer to avoid that but I'm not sure what's the preferred way to do it. The appChanged variable/argument supports 3 states as explained at [1] :
- true if the application version changed
- false if the application version did not change
- undefined if it's a new profile

Having appChanged set to undefined gives the behavior which I think is best for Nightly (ie scan + no popup) but I'm not sure we want to rely on this?

If yes, then I guess I should update the comment at [1]. If no, I suppose I need to pass another argument to the provider startup method (hasVersionChanged or something similar? and complexify the check found at [2])

[1] http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2000-2006

[2] http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2168-2169

Comment 4

[Julian Descottes [:jdescottes]]

Additional question here: is there any other "provider" that can be used with the AddonManager other than the XPIProvider? It looks like some of the arguments passed at the moment are never used (oldAppVersion, oldPlatformVersion) so I'm wondering if we could get rid of them.

Comment 5

[Andrew Swan [:aswan]]

To answer the last question, there are several other providers (GMP and lightweight themes off the top of my head, probably some others too)

I don't think we should be finding system add-ons with a directory scan at all, perhaps bug 1348981 would be a better solution here?  Regardless, I request that we don't make any changes to startup code until after bug 1353194 lands.

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Andrew Swan [:aswan] from comment #5)
> To answer the last question, there are several other providers (GMP and
> lightweight themes off the top of my head, probably some others too)
> 
> I don't think we should be finding system add-ons with a directory scan at
> all, perhaps bug 1348981 would be a better solution here?  Regardless, I
> request that we don't make any changes to startup code until after bug
> 1353194 lands.

Thanks for the info. My patch here is based on a suggestion from Kris, but maybe I got it wrong, so I'll wait for him to comment.

My root issue is that new system addons are not installed and started on the first startup (except when a full scan is performed). This is blocking Bug 1369801, which attempts to migrate devtools to a system addon. I'll be happy to close this and block Bug 1369801 on something else. If we agree that Bug 1348981 is a better fix for this, I can try to take it.

I'll block this on Bug 1353194 in the meantime.

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8898704 [details]
Bug 1391187 - compare build id to perform full directory scan in addon manager;

I think I'd be OK with this approach, but it might be better to only use the build ID check to decide whether to do the startup scan, rather than tying it to the rest of the appChanged logic too.

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8898704 [details]
Bug 1391187 - compare build id to perform full directory scan in addon manager;

Thanks for the feedback! I'll try to apply your suggestion, as I'd also prefer to avoid messing with appChanged.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8898704 [details]
Bug 1391187 - compare build id to perform full directory scan in addon manager;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170062/diff/1-2/

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8898704 [details]
Bug 1391187 - compare build id to perform full directory scan in addon manager;

https://reviewboard.mozilla.org/r/170062/#review178118

I'd probably be OK with this approach, but I think I'd rather

Comment 11

[Julian Descottes [:jdescottes]]

Thanks for the review, Kris. Looks like your message was cut?

Comment 12

[Kris Maglione [:kmag]]

Huh. It looks like it reposted an incomplete draft of my last review. Just ignore it.

Comment 13

[Julian Descottes [:jdescottes]]

(In reply to Kris Maglione [:kmag] from comment #12)
> Huh. It looks like it reposted an incomplete draft of my last review. Just
> ignore it.

Ok thanks! Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4ba6690d08ace6a75629cc8c422f35525d8a3a0

(In reply to Andrew Swan [:aswan] (PTO through 9/4) from comment #5)
> To answer the last question, there are several other providers (GMP and
> lightweight themes off the top of my head, probably some others too)
> 
> I don't think we should be finding system add-ons with a directory scan at
> all, perhaps bug 1348981 would be a better solution here?  Regardless, I
> request that we don't make any changes to startup code until after bug
> 1353194 lands.

I really would like to land this bug and Bug 1369801 at least 2 weeks before the end of the 57 cycle. 
I checked the patch in Bug 1353194: my patch here can be rebased on it without conflict (and vice-versa). Looking at the current changes in the patch I don't think there should be any issue regarding landing this Bug before Bug 1353194. 

Aswan being on PTO, I'm not sure how to proceed. Andy what do you think about this?

Comment 14

[Kris Maglione [:kmag]]

Please land and request beta uplift. This is a fairly trivial change that gives us more predictable behavior while we work out a cleaner long-term solution.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36071efc126c
compare build id to perform full directory scan in addon manager;r=kmag

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/36071efc126c

Comment 17

[Liz Henry (:lizzard)]

Can you request uplift to m-r?  This was suggested as a) a missed uplift for 56   and b) a possible solution to the release blocking issue in bug 1399936.

Comment 18

[Liz Henry (:lizzard)]

andym, can you help out here if jdescottes is not around? Thanks.

Comment 19

[Julian Descottes [:jdescottes]]

Looking at this and preparing patches for beta/release.

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8898704 [details]
Bug 1391187 - compare build id to perform full directory scan in addon manager;

The current patch should apply correctly on release. I'm currently checking the fix works on release too, but I can't use artifact builds, so it will take ~1hr before I can finish my tests.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: System addons such as shield can fail to be installed on startup. Could fix Bug 1399936 (ask :kmag or :rhelmer for more info if needed)
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: I verified on Nightly. In case we want someone else to do it I can provide STRs.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: When the Firefox version changes, we look for addons with a full directory scan. With this patch we simply also do it if the build id changed. Except some code for checking the build id, there's not much new code here.
[String changes made/needed]: None

Comment 21

[Kris Maglione [:kmag]]

(In reply to Julian Descottes [:jdescottes] from comment #20)
> The current patch should apply correctly on release. I'm currently checking
> the fix works on release too, but I can't use artifact builds, so it will
> take ~1hr before I can finish my tests.

Protip: You can get working artifact builds for release by adding 'relases/mozilla-release' to http://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/python/mozbuild/mozbuild/artifacts.py#116-120

Comment 22

[Kris Maglione [:kmag]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #21)
> (In reply to Julian Descottes [:jdescottes] from comment #20)
> > The current patch should apply correctly on release. I'm currently checking
> > the fix works on release too, but I can't use artifact builds, so it will
> > take ~1hr before I can finish my tests.
> 
> Protip: You can get working artifact builds for release by adding
> 'relases/mozilla-release' to
> http://searchfox.org/mozilla-central/rev/
> 2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/python/mozbuild/mozbuild/artifacts.
> py#116-120

*'releases/mozilla-release'

Comment 23

[Julian Descottes [:jdescottes]]

Verified with the following STRs:
- comment out Pocket in browser/extensions/moz.build
- ./mach build && ./mach package
- run the built FF, create profile P1
=> Firefox starts without Pocket
- restore Pocket in browser/extensions/moz.build
- ./mach build && ./mach package
- run the built FF, reuse profile P1
=> Firefox starts with Pocket


 (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #21)
> (In reply to Julian Descottes [:jdescottes] from comment #20)
> > The current patch should apply correctly on release. I'm currently checking
> > the fix works on release too, but I can't use artifact builds, so it will
> > take ~1hr before I can finish my tests.
> 
> Protip: You can get working artifact builds for release by adding
> 'relases/mozilla-release' to
> http://searchfox.org/mozilla-central/rev/
> 2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/python/mozbuild/mozbuild/artifacts.
> py#116-120

Thanks for the tip! For some reason it retrieved a very old pushhead (FF54) and my build failed, so I switched back to non-artifact. I'm on OSX, not sure if artifacts are published for all OSes?

Comment 24

[Kris Maglione [:kmag]]

(In reply to Julian Descottes [:jdescottes] from comment #23)
> Thanks for the tip! For some reason it retrieved a very old pushhead (FF54)
> and my build failed, so I switched back to non-artifact. I'm on OSX, not
> sure if artifacts are published for all OSes?

They are. Sometimes I find I have to clear the artifact cache in ~/.mozbuild/package-frontend/ and do a clobber build before I get the right artifacts, though. I'm not entirely sure why.

Comment 25

[Liz Henry (:lizzard)]

Comment on attachment 8898704 [details]
Bug 1391187 - compare build id to perform full directory scan in addon manager;

Release blocking issue that makes the addons manager fail to load. 
Please uplift to m-r.
This will need testing.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5b3d123eee24 (FIREFOX_56b13_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/2af6e1c309ba

Comment 27

[Cornel Ionce [:noni]]

Isn't there any other simplier method of verifying this fix?
With the STR provided in comment 23 we need to build Firefox which takes a lot of time in downloading all the prerequisites and our team is heavily loaded at this moment.

Comment 28

[Julian Descottes [:jdescottes]]

(In reply to Cornel Ionce [:cornel_ionce], Desktop Release QA from comment #27)
> Isn't there any other simplier method of verifying this fix?
> With the STR provided in comment 23 we need to build Firefox which takes a
> lot of time in downloading all the prerequisites and our team is heavily
> loaded at this moment.

Not that I know, sorry. My patch fixes the use case where 2 binaries of Firefox have the same version number, but a different set of system addons. I'm not sure how you can reproduce that without building Firefox yourself. I also don't know for sure how this patch unblocks bug 1399936. 

I could do 2 try pushes so that you can fetch Firefox binaries and run the scenario without building yourself. But I don't know if this counts as a valid test?

Comment 29

[Kris Maglione [:kmag]]

You should be able to just use two different beta releases. Betas for a given release always use the same version number (they don't add "b?" to it), so upgrading between them should produce the same issues. Or two different nightly versions, for that matter.

Comment 30

[Julian Descottes [:jdescottes]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #29)
> You should be able to just use two different beta releases. Betas for a
> given release always use the same version number (they don't add "b?" to
> it), so upgrading between them should produce the same issues. Or two
> different nightly versions, for that matter.

Not sure I follow, to reproduce the issue you would still need two betas/nightlies with different sets of system addon. Is there a specific system addon which got enabled only at some point during beta?

Comment 31

[Kris Maglione [:kmag]]

(In reply to Julian Descottes [:jdescottes] from comment #30)

> Not sure I follow, to reproduce the issue you would still need two
> betas/nightlies with different sets of system addon. Is there a specific
> system addon which got enabled only at some point during beta?

Ah, that's a good question. I'd say a nightly before we shipped this is probably your best bet at this point:

https://hg.mozilla.org/mozilla-central/rev/9c581d290dc3

I don't know if we've uplifted any system add-ons to beta this cycle.

Comment 32

[roxleitan]

Reproduced the initial issue on Mac OS X 10.12 using Nightly 56.0a1 (2017-07-20) (create profile P1 - no Pocket) and opened the P1 profile with Nightly 57.0a1 (2017-08-12) -> no Pocket

Verified fixed using latest Nightly 58.0a1(2017-09-24): upgrade from Nightly 57.0a1 (2017-08-12) opened with the same P1 profile.

Reproduced the initial issue on Mac OS X 10.12 using Beta 55.0b10 (2017-07-17)(create profile P2 - no Pocket) and opened the P2 profile with Beta 56.0b3

Verified fixed using the latest Release Candidate 56.0 (2017-09-22).

Cannot reproduce the issue on Windows 10 x64 and Ubuntu 16.04 x64. Was this issue only on Mac?

Comment 33

[Julian Descottes [:jdescottes]]

(In reply to [e-mail] from comment #32)
> Reproduced the initial issue on Mac OS X 10.12 using Nightly 56.0a1
> (2017-07-20) (create profile P1 - no Pocket) and opened the P1 profile with
> Nightly 57.0a1 (2017-08-12) -> no Pocket
> 
> Verified fixed using latest Nightly 58.0a1(2017-09-24): upgrade from Nightly
> 57.0a1 (2017-08-12) opened with the same P1 profile.
> 

The fix was supposed to only be useful for two binaries that have the same version number. So if you go from 56.0a1 to 57.0a1 it should have worked regardless of my fix. I'm glad if my fix helps here, but that was not the intent.

The issue only occurs the first time the system addon is discovered. Reusing the profile P1 for Nightly 58, knowing that it had already been opened with Nightly 57, makes the test invalid. If I understand correctly what you are testing we have:
- FF56 binary, without Pocket
- FF57 binary, with Pocket but without my patch
- FF58 binary, with Pocket and my patch

If that is correct you need to profiles to verify that "something" is fixed. 
P1 where you first open FF56 then FF57 -> Pocket should not show up. 
P2 where you first open FF56 then FF58 -> Pocket should show up. 
Again, the bug fixed here is only occurring the first time a given profile sees a new system addon, so make sure not to open FF57 or FF58 several times before checking if Pocket is correctly started.

This is all assuming that my fix actually solves the scenario you are testing where we are moving from version N to N+1, which is not guaranteed. Sorry this is such a pain to verify :(

> Reproduced the initial issue on Mac OS X 10.12 using Beta 55.0b10
> (2017-07-17)(create profile P2 - no Pocket) and opened the P2 profile with
> Beta 56.0b3
> 
> Verified fixed using the latest Release Candidate 56.0 (2017-09-22).
> 
> Cannot reproduce the issue on Windows 10 x64 and Ubuntu 16.04 x64. Was this
> issue only on Mac?

I believe I only tested on Mac, I'll check if the issue also occurs on Linux and will report back.

Comment 34

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #33)
> If that is correct you need to profiles to verify that "something" is fixed.

s/to profiles/two profiles

Comment 35

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #33)
> I believe I only tested on Mac, I'll check if the issue also occurs on Linux
> and will report back.

I can't reproduce the bug on Linux, so it's possible this was an OSX only issue.

Comment 36

[roxleitan]

(In reply to Julian Descottes [:jdescottes] from comment #33)
 
> If that is correct you need to profiles to verify that "something" is fixed. 
> P1 where you first open FF56 then FF57 -> Pocket should not show up. 
> P2 where you first open FF56 then FF58 -> Pocket should show up. 
> Again, the bug fixed here is only occurring the first time a given profile
> sees a new system addon, so make sure not to open FF57 or FF58 several times
> before checking if Pocket is correctly started.
> 
> This is all assuming that my fix actually solves the scenario you are
> testing where we are moving from version N to N+1, which is not guaranteed.
> Sorry this is such a pain to verify :(

Tested again with 2 profiles:
P1 - opened with Nightly 56.0a1 (2017-07-20), then with Nightly 57.0a1 (2017-08-12)-> no pocket
P2 - opened with Nightly 56.0a1 (2017-07-20), then with latest Nightly 58.0a1 (2017-09-25)-> the pocket appears (fixed)

Tested also with this scenario using Firefox Beta, with the following results:
P3 - opened with Beta 52.0b2, then with Beta 56.0b3 ->no pocket
P4 - opened with Beta 52.0b2, then with the latest Release Candidate 56.0 -> no pocket
Please note that the pocket is missing if I open the latest RC 56.0 with a new profile.(reproducible only on Mac)

Comment 37

[Cornel Ionce [:noni]]

Thanks for covering this, Roxana.
Removing my ni? since manual validation has already been performed.

Comment 38

[roxleitan]

Tested again using Firefox Beta, with the following results:

P5 - opened with Beta 52.0b2, then with Beta 56.0b3 -> no pocket
P6 - opened with Beta 52.0b2, then with Beta 57.0b14 -> the pocket appears (fixed)

Based on the above and based on comment 36 I will mark this bug verified fixed and I will change the flags accordingly.