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

RESOLVED FIXED in mozilla1.8.1beta1

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

({fixed1.8.1})

1.8 Branch
mozilla1.8.1beta1
fixed1.8.1
Points:
---
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

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 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...
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.
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.
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.
Created attachment 198242 [details]
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.
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.
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 on attachment 198184 [details] [diff] [review]
patch

Clearing review - a more comprehensive fix which would include new strings is more appropriate for the trunk
Attachment #198184 - Flags: review?(mconnor)
Attachment #198184 - Attachment is obsolete: true
Target Milestone: --- → Firefox 2 alpha2

Updated

12 years ago
Flags: blocking-firefox2+
Whiteboard: 1d
Target Milestone: Firefox 2 alpha2 → Firefox 2 beta1
K, lemme know if there's anything I can do to help here, in terms of strings, Rob.
(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.
I have a patch that is about 80% complete for fixing this bug.
Whiteboard: 1d → .5d
Created attachment 225403 [details]
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.
Attachment #225403 - Flags: ui-review?(beltzner)
mconnor, perhaps one of the interns could take on bug 333860?
beltzner: I should mention that the strings used are from an old patch and obviously need revision.
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?
Attachment #225403 - Flags: ui-review?(beltzner) → ui-review-
Attachment #225404 - Flags: review?(mconnor)
Created attachment 225915 [details]
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.
Attachment #225403 - Attachment is obsolete: true
Attachment #225404 - Attachment is obsolete: true
Created attachment 225916 [details] [diff] [review]
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.
Attachment #225916 - Flags: ui-review?(beltzner)
Attachment #225916 - Flags: review?(mconnor)
Version: unspecified → 2.0 Branch
Whiteboard: .5d → [needs review]
Comment on attachment 225916 [details] [diff] [review]
patch

Benjamin, mconnor is a bit swamped with review requests... could you review this? Thanks
Attachment #225916 - Flags: review?(mconnor) → review?(benjamin)

Comment 21

12 years ago
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.
Attachment #225916 - Flags: review?(benjamin) → review+
Checked in to trunk
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
Attachment #225916 - Flags: approval1.8.1?
Comment on attachment 225916 [details] [diff] [review]
patch

I'll post a followup bug for suggested string changes, sorry to be late.
Attachment #225916 - Flags: ui-review?(beltzner) → ui-review+

Updated

12 years ago
Attachment #225916 - Flags: approval1.8.1? → approval1.8.1+
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.
Checked in to MOZILLA_1_8_BRANCH for Firefox 2.0
Keywords: fixed1.8.1
No longer blocks: 333860
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.