Reconsider InstallTrigger.enabled() return value



Core Graveyard
Installer: XPInstall Engine
14 years ago
2 years ago


(Reporter: dveditz, Assigned: dveditz)


({fixed-aviary1.0, fixed1.7.5})

fixed-aviary1.0, fixed1.7.5
Bug Flags:
blocking-aviary1.0PR +

Firefox Tracking Flags

(Not tracked)




(1 attachment, 1 obsolete attachment)

When we added whitelisting to xpinstall launches it seemed very few sites
bothered to check InstallTrigger.enabled(), and that those that did would prefer
knowing the install was likely to fail so they could give a message to their users.

It may be better to return enabled() to its previous value (the global install
state, generally "true") and let the site attempt to install and then let the
whitelisting block lead to the user-visible info-banner.

Comment 1

14 years ago
> ... would prefer knowing the install was likely to fail so they could give a 
> message to their users.

I believe that this is still somewhat true.  But, sites would only expect
InstallTrigger.enabled() to return false if the user took some action to prevent
software update.  Hence, the message a site might show the user would be
designed for a user who would understand what they did to disable software
update in the first place.

Moreover, a webapp that pre-dates XPI whitelisting in Mozilla would be unable to
explain XPI whitelisting to the user!  Hence, the burden is on the browser to
explain XPI whitelisting to the user.  We cannot expect all webapps to be
revised overnight, so we must rely on the browser to provide some useful
feedback to the user when XPI whitelisting is the reason that an install failed.

If reverting InstallTrigger.enabled() to the older behavior is the best way to
do this, then I'm all for it.

Comment 2

14 years ago
Adding testcase.  It turns out was likewise broken.
Flags: blocking1.8a4?
Flags: blocking1.7.x?
Flags: blocking-aviary1.0?

Comment 3

14 years ago
*** Bug 258641 has been marked as a duplicate of this bug. ***

Comment 4

14 years ago
after some discussion sounds like we want to release note this and/or review
futher before PR.
Flags: blocking-aviary1.0PR?

Comment 5

14 years ago
saving the install page to a local file, then clicking on the install link works
as a very ugly and difficult to discover work around...
Comment on attachment 158795 [details] [diff] [review]

Attachment #158795 - Flags: review+
Comment on attachment 158795 [details] [diff] [review]

no good, this one disabled whitelisting entirely -- all the nsJSInstallTrigger
entrypoints call through here.

We'll have to put an explicit pref check into nsJSInstallTrigger instead, or
pass some sort of boolean into this one (which might be useful to allow
installChrome to bypass whitelisting as well).
Attachment #158795 - Flags: superreview-
Chofmann made me r+ it, really! :-)

Comment 10

14 years ago
decided to take this for PR...  dan is testing an update to the patch.
Flags: blocking-aviary1.0PR? → blocking-aviary1.0PR+
Created attachment 158817 [details] [diff] [review]
aviary patch

Same basic idea, but pass a flag from the callsites whether they need global or
whitelist checking.

The reason for the flag, rather than just putting pref code directly into
InstallTriggerGlobalUpdateEnabled() is that the flag can be used to solve bug
246375 at the same time.  I've slipped that into this patch -- if you don't
like it I can change the installChrome() call to simply pass XPI_WHITELIST
without checking the chromeType and fix only this bug.
Attachment #158795 - Attachment is obsolete: true
Comment on attachment 158817 [details] [diff] [review]
aviary patch

Note this is a -w diff to make reviews easier. I'll apply to all branches. If
someone else wants to apply it watch for whitespace issues.
Attachment #158817 - Flags: superreview?(jst)
Attachment #158817 - Flags: review?(bugs)
Attachment #158817 - Flags: approval1.7.x?
Attachment #158817 - Flags: approval-aviary?
Comment on attachment 158817 [details] [diff] [review]
aviary patch

Attachment #158817 - Flags: superreview?(jst) → superreview+
+            prefBranch->GetBoolPref( XPINSTALL_ENABLE_PREF, aReturn);

doesn't look like the space after the '(' is the style of the file

Comment 16

14 years ago
Comment on attachment 158817 [details] [diff] [review]
aviary patch

a=chofmann for PR
Attachment #158817 - Flags: approval-aviary? → approval-aviary+
Fixed on trunk plus aviary and 1.7 branches
Last Resolved: 14 years ago
Keywords: fixed-aviary1.0, fixed1.7.x
Resolution: --- → FIXED

Comment 18

14 years ago
This doesn't seem fixed to me in the latest round of respins available at
Resolution: FIXED → ---
It is and it isn't... the bug described here is fixed, but several sites are
suffering from two bugs.

Any site doing:
   if (InstallTrigger.enabled())
      InstallTrigger.install(....);   //or installChrome()

is now fixed by this patch.  Any site doing

   if (InstallTrigger.enabled())

is /closer/ to fixed, but still doesn't work. The startSoftwareUpdate function
does not send the notifications that the UI uses to put up the infobar.
Depends on: 259211
Just to keep problems straight--the bug as narrowly defined here IS fixed--I
filed bug 259211 on the startSoftwareUpdate problem. The link
above suffers from both problems, but other sites are fixed by just this bug.
Last Resolved: 14 years ago14 years ago
No longer depends on: 259211
Resolution: --- → FIXED


14 years ago
Flags: blocking1.8a4?
Flags: blocking1.7.x?
Flags: blocking-aviary1.0?
*** Bug 252015 has been marked as a duplicate of this bug. ***


13 years ago
Attachment #158817 - Flags: approval1.7.x? → approval1.7.x+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.