310745 - Extension update compatibility wizard tries to install updates when xpinstall is disabled

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Robert Strong (they/them - no direct email)]

This is similar to bug 307928 except this is for the update compatibility wizard
that is used to update incompatible extensions when the app is upgraded and the
app extension version changes.

Patch coming up.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch

Comment 2

[Mike Connor [:mconnor]]

Comment on attachment 198184 [details] [diff] [review]
patch

so the problem is that this fails? or that this works?	  Note that we don't
have UI for this pref anymore, the pref now corresponds to whitelisting, not
xpinstall in general.  And is disabling updates really the expected result, or
is it _new_ software that people don't want?  But 307928 in conjunction with
this would force users to toggle pref, do updates, toggle pref, which really
isn't what I think we want to do here...

Comment 3

[Robert Strong (they/them - no direct email)]

Attempting to install an update with this wizard when xpinstall.enabled is false
will fail and it will appear as a hang to the user. This wizard is shown before
the main event loop starts so there is no ui except for the wizard at this stage
and hence no ability for the user to toggle the pref. Bug 307928 disables
checking for updates similarly since it is not possible to install updates when
xpinstall.enabled is false. As for expected results I agree that we should
provide other options but the xpinstall.enabled pref's purpose is to disable
installs so we shouldn't be showing / enabling ui for installing updates.

Comment 4

[Robert Strong (they/them - no direct email)]

To clarify just in case... I do think we need the ability for a "just this time"
option but I think we also need to keep the ability to disable ALL installs as
xpinstall.enabled provides so sysadmins can continue to lock down a system.

Comment 5

[Robert Strong (they/them - no direct email)]

The options available as I see it:
a) leave it how it is now where if xpinstall.enabled is false we fail during the
install and the ui appears to hang.
b) add the ability to toggle the pref in the wizard. There is a patch in bug
288054 that does this though this option was shot down for reasons I happen to
agree with but IMO it is better than option a.
c) go with this patch.
d) ...?

I think it would be a good thing if app update also warns the user during an
upgrade that will initiate a compatibility check when xpinstall.enabled is set
to false and provide the ability to change this pref value if it is not locked.
This would of course require new strings.

The same pretty much applies to bug 307928 and if new strings were allowed more
could be done... I think it is better to prevent than to fail halfway through
the operation which is where we are now and that is the direction that bug is
headed at this time. Though it is not clear whether the patch for bug 288054
will be backed out even if it is it will not fix this bug or bug 307928.

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: screenshot

It has been a while since I last verified this bug still exists so I went ahead
and did so today and took a screenshot of the wizard page where it dies. If an
update is found the user has the option to install it. If they try to install
it the wizard page for downloading/installing is displayed with all buttons
disabled and it never goes any further.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

So, if I'm reading this patch right, what you're saying is that if
xpinstall.enabled is set to false, we just totally skip the part of the
compatability wizard where we check for updates to the extensions?

That seems like the right behaviour to me, although we might *possibly* want to
inform users that the step was skipped and they should manually update
extensions if they want to find compatible versions.

Comment 8

[Robert Strong (they/them - no direct email)]

That is what the patch does. Also, part of an earlier patch in bug 288054 does
inform the user and it would be relatively easy to add though it would require
new strings.

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 198184 [details] [diff] [review]
patch

Clearing review - a more comprehensive fix which would include new strings is more appropriate for the trunk

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

K, lemme know if there's anything I can do to help here, in terms of strings, Rob.

Comment 11

[Robert Strong (they/them - no direct email)]

(In reply to comment #10)
> K, lemme know if there's anything I can do to help here, in terms of strings,
Thanks Mike... I'd like to provide the option to set xpinstall.enabled to true if the wizard. I wrote a patch that did this previously but it was shot down due to not wanting prefs set via ui outside of preferences. Since we are doing this now I hope that this won't be shot down again due to that. I have an old patch that accomplishes 90% of this and I'll update it when I have the time. I'd also like to take care of bug 333860 since it is essentially the same problem except it is due to the app being offline.

Comment 12

[Robert Strong (they/them - no direct email)]

I have a patch that is about 80% complete for fixing this bug.

Comment 13

[Robert Strong (they/them - no direct email)]

Attachment: screenshot of wizard with patch applied

Mike, this shows the update wizard when xpinstall.enabled is false and the user can change the pref and when xpinstall.enabled is false and the user can't change the pref due to it being locked.

patch coming up.

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: patch

Comment 15

[Robert Strong (they/them - no direct email)]

mconnor, perhaps one of the interns could take on bug 333860?

Comment 16

[Robert Strong (they/them - no direct email)]

beltzner: I should mention that the strings used are from an old patch and obviously need revision.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 225403 [details]
screenshot of wizard with patch applied

The string seems a little odd to me. Perhaps:

"This update can not be installed because software installation is currently disabled. You can change this setting below."

Or something that better indicates that this is changing a global setting?

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: screenshot

beltzner, screenshot with suggested string change. I also went through the update wiz strings and updated as appropriate / removed no longer used strings so you should probably review the strings in the patch instead of the screenshot.

Comment 19

[Robert Strong (they/them - no direct email)]

Attachment: patch

Mike and Mike, besides displaying the appropriate text, etc. regarding this bug this also removes the no longer used strings from the update wizard (e.g. the ones that should have been removed when app update was removed from this wizard) and updates the strings to use add-ons instead of extensions and themes, etc.

Comment 20

[Robert Strong (they/them - no direct email)]

Comment on attachment 225916 [details] [diff] [review]
patch

Benjamin, mconnor is a bit swamped with review requests... could you review this? Thanks

Comment 21

[Benjamin Smedberg]

Comment on attachment 225916 [details] [diff] [review]
patch

>Index: toolkit/mozapps/extensions/content/update.js

>+    this.xpinstallEnabled = pref.getBoolPref(PREF_XPINSTALL_ENABLED);
>+    this.xpinstallLocked = pref.prefIsLocked(PREF_XPINSTALL_ENABLED);

please add a try/catch around these, for XR apps that may not have set the default pref.

Comment 22

[Robert Strong (they/them - no direct email)]

Checked in to trunk

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 225916 [details] [diff] [review]
patch

I'll post a followup bug for suggested string changes, sorry to be late.

Comment 24

[Robert Strong (they/them - no direct email)]

I am going to hold off on landing this on the branch until bug 342350 can be landed on the branch in order to avoid changing the strings multiple times.

Comment 25

[Robert Strong (they/them - no direct email)]

Checked in to MOZILLA_1_8_BRANCH for Firefox 2.0