572322 - [ACR] Compatibility Prefs require restart after changing (ACR installation)

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Compatibility Tools, defect, P2)

Description

[Brian King [:kinger]]

Steps:

I installed the Add-ons Compatibility Reporter in TB 3.1. Version to install is not public yet, you can get it at:

https://bugzilla.mozilla.org/show_bug.cgi?id=564870#c7

On restart, the extensions changes the following prefs.
extensions.checkCompatibility.3.0
extensions.checkCompatibility.3.1p
extensions.checkCompatibility.3.1pre
extensions.checkCompatibility.3.1a
extensions.checkCompatibility.3.1b
extensions.checkCompatibility.3.1

The add-ons manager shows a notification to restart Thunderbird. Incompatible add-ons are not enabled until another restart.

Expected:
Incompatible add-ons are enabled immediately when the prefs are changed after install restart.

Comment 1

[David McNamara [:mackers]]

A second restart is required in both Firefox and Thunderbird to enable incompatible add-ons.

Here is what typically happens during the acr install process:

1) Open/download acr.xpi
2) User restarts Firefox
3) During start-up, Firefox installs the acr add-on
4) During acr initialization the checkCompatibility prefs are set
5) Opening the Extension Manager now shows a) "Compatibility checking is disabled" and b) "Restart to enable this add-on" on any incompatible add-ons.
6) User restarts Firefox
7) Firefox enables incompatible add-ons.
8) All incompatible add-ons appear enabled in Extension Manager.

I don't see any way around this because AFAIK during FF startup the Extension Manager stuff (enable/disable) is done before add-ons get to act on anything.

Or put another way: I don't think there is any way for an add-on to programmatically set the checkCompatibility prefs before the Extension Manager next checks should it re-enable incompatible add-ons (during the next start-up).

Comment 2

[Brian King [:kinger]]

(In reply to comment #1)
> I don't see any way around this because AFAIK during FF startup the Extension
> Manager stuff (enable/disable) is done before add-ons get to act on anything.

Dave, we may have had this discussion elsewhere, but can you comment? Right now we are changing the prefs at 'final-ui-startup'.

Comment 3

[Jesse Ruderman]

See also bug 576568, "Update Compatibility Reporter for XPCOM component registration changes".

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #2)
> (In reply to comment #1)
> > I don't see any way around this because AFAIK during FF startup the Extension
> > Manager stuff (enable/disable) is done before add-ons get to act on anything.
> 
> Dave, we may have had this discussion elsewhere, but can you comment? Right now
> we are changing the prefs at 'final-ui-startup'.

I actually can't think of how to manage this now. I presume this used to work because the EM restart made your pref changes took effect, but now that is no longer there, and extensions cannot physically do anything before the extension manager does its stuff. The only think I can think of is that ACR could check for any extensions that are pending-enable at startup and if so immediately offer to restart for the user. Hardly nice and clean but it is all I have right now.

Comment 5

[Brian King [:kinger]]

(In reply to comment #4)
> ... The only think I can think of is that ACR could check
> for any extensions that are pending-enable at startup and if so immediately
> offer to restart for the user. Hardly nice and clean but it is all I have right
> now.

Seems acceptable. Justin, thoughts?

Comment 6

[Justin Scott [:fligtar]]

A second restart has always been required to use incompatible add-ons. The first-run page even says to do so in the first step: https://preview.addons.mozilla.org/en-US/firefox/pages/compatibility_firstrun

If we can improve this in any way, that sounds great, but I assumed it wasn't possible since we've been living with it.

Comment 7

[Brian King [:kinger]]

(In reply to comment #6)
> A second restart has always been required to use incompatible add-ons. The
> first-run page even says to do so in the first step:
> https://preview.addons.mozilla.org/en-US/firefox/pages/compatibility_firstrun
> 
> If we can improve this in any way, that sounds great, but I assumed it wasn't
> possible since we've been living with it.

I think it's just something we'll have to live with, as we can't hook into the EM earlier. If anyone knows a way, feel free to reopen.

As for alerting the user about the 2nd restart, this is already happening in the listing of the add-ons that will be enabled (see step 5 in comment 1). Keeping with the spirit of not annoying the users too much, I think this is good enough.

Comment 8

[Dave Townsend [:mossop]]

I had an idea of at least a way to make this look more seamless to the user. If you have a "final-ui-startup" observer and trigger an app restart from there then the user will see no UI or anything the app will just look like it took longer to startup. You'd have to be v.careful to not restart-loop but it seems workable.

Comment 9

[Brian King [:kinger]]

I thought about that before but ruled it out because I thought it would offend the Firefox startup gods, i.e. it would perceive longer startup time which we have been working to improve all the time.

However, once in a while (per install of ACR) is probably acceptable. Reopening to give it a go.

Comment 10

[David McNamara [:mackers]]

A fix for this lands in r92450

Here is the ACR install process:

1) User opens acr.xpi
2) User restarts Firefox
3) During start-up, Firefox installs the ACR add-on (no UI appears yet)
4) ACR performs initialization - we set the checkCompatibility prefs. to false (user-defined checkCompatibility prefs. are also backed-up at this point)
5) ACR now restarts the application (this is the 2nd Firefox restart)
6) Firefox now enables any add-ons that were previously incompatible
7) Firefox UI appears (with all incompatible add-ons installed and running)

During this process, the user is unaware that their browser has restarted twice and there is no noticable increase in start-up time.

Comment 11

[Brian King [:kinger]]

Will be in upcoming 0.8.6.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Works fine with ACR 0.8.6 for the installation of the add-on. But we still fail if you only enable the add-on. I will file a follow-up bug.

Comment 13

[Tony Mechelynck [:tonymec]]

About ACR 0.8.6: see also bug 659772 comment #35.

Comment 14

[Dave Townsend [:mossop]]

From bug 671894 and its dupes I think this is actually breaking the add-ons manager immediately after installation. I think we probably should back it out and get an updated ACR out until we can figure out what is going on.

Comment 15

[Justin Scott [:fligtar]]

I've disabled 0.8.6 on AMO. I too noticed the symptoms of bug 659772 this morning and emailed Brian and Dave wondering if it was related to the ACR update.

Comment 16

[Tony Mechelynck [:tonymec]]

(In reply to comment #15)
> I've disabled 0.8.6 on AMO. I too noticed the symptoms of bug 659772 this
> morning and emailed Brian and Dave wondering if it was related to the ACR
> update.

What is the recommended course of action for users who updated ACR to 0.8.6 several days ago and restarted the app several times (maybe for nightly updates) since then?
(a) Don't make waves, the critical restart is the first one after installing 0.8.6, if you leave it enabled nothing more should happen,
or (b) Go back to 0.8.5, and faster than that!

Comment 17

[Dave Townsend [:mossop]]

(In reply to comment #16)
> (In reply to comment #15)
> > I've disabled 0.8.6 on AMO. I too noticed the symptoms of bug 659772 this
> > morning and emailed Brian and Dave wondering if it was related to the ACR
> > update.
> 
> What is the recommended course of action for users who updated ACR to 0.8.6
> several days ago and restarted the app several times (maybe for nightly
> updates) since then?
> (a) Don't make waves, the critical restart is the first one after installing
> 0.8.6, if you leave it enabled nothing more should happen,
> or (b) Go back to 0.8.5, and faster than that!

With the way the change is the first install is the one that does the work that causes the problem, subsequently I'd expect there to be no problem.

Comment 18

[Brian King [:kinger]]

Reopening to see if we can do it right.

Causes bug 659772 (and possibly others), and 0.8.6 is disabled on AMO.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We could consider a Mozmill test for at least this situation once identified.

Comment 20

[Brian King [:kinger]]

I've uploaded version 0.8.7 with this feature turned off for now, r92665.

Comment 21

[David McNamara [:mackers]]

Maybe the best solution to this would be to simply display a doorhanger notification to the user: 

"You must restart Firefox to enable incompatible addons. [Restart]"

By the time they read the notification and click, it will be safe to restart.

Comment 22

[Brian King [:kinger]]

(In reply to David McNamara [:mackers] from comment #21)
> Maybe the best solution to this would be to simply display a doorhanger
> notification to the user: 
> 
> "You must restart Firefox to enable incompatible addons. [Restart]"
> 
> By the time they read the notification and click, it will be safe to restart.

A notification message is already shown in the the listing of each add-on that will become compatible, but this would certainly be more noticable.

It doesn't mean we shouldn't give fixing this properly another shot though.

Comment 23

[David McNamara [:mackers]]

A possible solution is to use eAttemptQuit instead of eForceQuit in nsIAppStartup.quit

However, I believe this distinction is only concerned with what happens should there be a window open that will not close. 

https://developer.mozilla.org/en/NsIAppStartup#quit%28%29

In our case, there should be no windows open yet, and the app will still quit immediately, corrupting the addons database.

I can't test this as I am unable to reproduce bug 659772.

Comment 24

[David McNamara [:mackers]]

Here are two separate builds for testing:

http://dl.dropbox.com/u/2575442/acr-bug572322-forceRestart.xpi
http://dl.dropbox.com/u/2575442/acr-bug572322-attemptRestart.xpi

** WARNING: above xpis will possibly corrupt your profile **

To test: 

1) uninstall any version of the acr extension
2) reset pref "extensions.acr.postinstall"
3) install addon from link above
4) restart Firefox

ACR will silently restart the browser. For the "forceRestart" XPI, you should hopefully be able to reproduce the profile corruption seen in bug 659772

If the "attemptRestart" XPI also corrupts your profile, then this fix is not viable.

Comment 25

[Brian King [:kinger]]

The better reference bug I think for what corruption happens is bug 671894.

I tried both builds from comment 24 and am not able to reproduce. I never was. I even tried installing some of the add-ons mentioned in bug 671894.

I suspect you are correct in saying that the Add-ons Manager makes no distinction between what flag you use (eAttemptQuit or eForceQuit), because it does not hook into it or rely on windows being available. Mossop?

David, what event are you waiting for before restarting ... "final-ui-startup" as Mossop suggested? Might there be a later one perhaps? Poking around it seems not, at least not without windows appearing.

Also poking around, I wonder if this will help us in any way:
https://developer.mozilla.org/en/Addons/Add-on_Manager/AddonManager#getStartupChanges%28%29

Comment 26

[Dave Townsend [:mossop]]

(In reply to Brian King (Briks) [:kinger] from comment #25)
> The better reference bug I think for what corruption happens is bug 671894.
> 
> I tried both builds from comment 24 and am not able to reproduce. I never
> was. I even tried installing some of the add-ons mentioned in bug 671894.
> 
> I suspect you are correct in saying that the Add-ons Manager makes no
> distinction between what flag you use (eAttemptQuit or eForceQuit), because
> it does not hook into it or rely on windows being available. Mossop?

Yeah, add-ons manager shouldn't care here. That said I also never managed to really reproduce this for myself so I'm not really sure what was going wrong.

Comment 27

[David McNamara [:mackers]]

Assuming the problem here is that the addon repository is not shutdown correctly causing the sqlite database to become corrupt.

r96098 has the following behaviour:

- On first restart after ACR install ("postinstall"), we send a asynchronous shutdown request to the AddonRepository module. This does the necessary storage shutdown stuff.

- ACR is notified of the shutdown by "addon-repository-shutdown". At this point we assume it is safe and restart the application.

Here is a build of the above which hopefully we can test against (notwithstanding the problem of not being able to repro. the original bug).

http://dl.dropbox.com/u/2575442/acr-r96098.xpi

Comment 28

[Brian King [:kinger]]

Updated test build (version # 0.9.3):

http://dl.dropbox.com/u/5940796/acr/acr093.xpi

Comment 29

[Dave Townsend [:mossop]]

(In reply to David McNamara [:mackers] from comment #27)
> Assuming the problem here is that the addon repository is not shutdown
> correctly causing the sqlite database to become corrupt.

Can you explain where this aassumption comes from? The errors we were seeing were coming from XPIProvider, not AddonRepository.

We're seeing a few reports trickling in again of the errors we were seeing before, at least some of which have ACR 1.0 installed. At this point it is too few to draw any real conclusions though.

Comment 30

[Brian King [:kinger]]

(In reply to Dave Townsend (:Mossop) from comment #29)
> We're seeing a few reports trickling in again of the errors we were seeing
> before, at least some of which have ACR 1.0 installed. At this point it is
> too few to draw any real conclusions though.

Are these Nightly only reports? See:
https://bugzilla.mozilla.org/show_bug.cgi?id=698653#c14

Comment 31

[Dave Townsend [:mossop]]

(In reply to Brian King (Briks) [:kinger] from comment #30)
> (In reply to Dave Townsend (:Mossop) from comment #29)
> > We're seeing a few reports trickling in again of the errors we were seeing
> > before, at least some of which have ACR 1.0 installed. At this point it is
> > too few to draw any real conclusions though.
> 
> Are these Nightly only reports? See:
> https://bugzilla.mozilla.org/show_bug.cgi?id=698653#c14

At least one was the 10.0a2 from aurora.

Comment 32

[David McNamara [:mackers]]

(In reply to Dave Townsend (:Mossop) from comment #29)
> Can you explain where this aassumption comes from? The errors we were seeing
> were coming from XPIProvider, not AddonRepository.

It wasn't an assumption that this was causing the bug. Rather, we were creating a test build to see if the problem was still occurring with this fix.

Comment 33

[David McNamara [:mackers]]

Added preference to allow easier control of ACR auto restart functionality.

extensions.acr.autorestart

Default is FALSE until we can satisfactorily test this solution.

Comment 34

[David McNamara [:mackers]]

Added preference to allow easier control of ACR auto restart functionality.

extensions.acr.autorestart

Default is FALSE until we can satisfactorily test this solution.

r98999

Comment 35

[Brian King [:kinger]]

(In reply to David McNamara [:mackers] from comment #34)
> Added preference to allow easier control of ACR auto restart functionality.
> 
> extensions.acr.autorestart
> 
> Default is FALSE until we can satisfactorily test this solution.

I don't think this should be FALSE by default, because there are no confirmed reports of this causing breakage since the new implementation.

Comment 36

[Brian King [:kinger]]

(In reply to Brian King (Briks) [:kinger] from comment #35)
> I don't think this should be FALSE by default, because there are no
> confirmed reports of this causing breakage since the new implementation.

I have set it to TRUE, r99366. So v1.0.2 (being released today) will still have auto restart turned on.

Comment 37

[Dave Townsend [:mossop]]

(In reply to Brian King (Briks) [:kinger] from comment #36)
> (In reply to Brian King (Briks) [:kinger] from comment #35)
> > I don't think this should be FALSE by default, because there are no
> > confirmed reports of this causing breakage since the new implementation.
> 
> I have set it to TRUE, r99366. So v1.0.2 (being released today) will still
> have auto restart turned on.

Confirmed that this code is still causing problems, see bug 728466 which, along with add-on sync being enabled causes us to throw away all add-ons too.