258798 - Reconsider InstallTrigger.enabled() return value

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Daniel Veditz [:dveditz]]

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

[Darin Fisher]

> ... 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

[Darin Fisher]

Adding testcase.  It turns out xulmine.mozdev.org was likewise broken.

Comment 3

[Darin Fisher]

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

Comment 4

[chris hofmann]

after some discussion sounds like we want to release note this and/or review
futher before PR.

Comment 5

[chris hofmann]

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 6

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

Comment 7

[Johnny Stenback (:jst)]

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

r=jst

Comment 8

[Daniel Veditz [:dveditz]]

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

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).

Comment 9

[Johnny Stenback (:jst)]

Chofmann made me r+ it, really! :-)

Comment 10

[chris hofmann]

decided to take this for PR...  dan is testing an update to the patch.

Comment 11

[Daniel Veditz [:dveditz]]

Attachment: 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.

Comment 12

[Daniel Veditz [:dveditz]]

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.

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 158817 [details] [diff] [review]
aviary patch

r=[e-mail]

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 158817 [details] [diff] [review]
aviary patch

sr=jst

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

+            prefBranch->GetBoolPref( XPINSTALL_ENABLE_PREF, aReturn);

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

Comment 16

[chris hofmann]

Comment on attachment 158817 [details] [diff] [review]
aviary patch

a=chofmann for PR

Comment 17

[Daniel Veditz [:dveditz]]

Fixed on trunk plus aviary and 1.7 branches

Comment 18

[Asa Dotzler [:asa]]

This doesn't seem fixed to me in the latest round of respins available at
http://ftp28f.newaol.com/pub/mozilla.org/firefox/nightly/2004-09-13-19-0.9/

Comment 19

[Daniel Veditz [:dveditz]]

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())
      InstallTrigger.startSoftwareUpdate(...);

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.

Comment 20

[Daniel Veditz [:dveditz]]

Just to keep problems straight--the bug as narrowly defined here IS fixed--I
filed bug 259211 on the startSoftwareUpdate problem. The xulmine.mozdev.org link
above suffers from both problems, but other sites are fixed by just this bug.

Comment 21

[Daniel Veditz [:dveditz]]

*** Bug 252015 has been marked as a duplicate of this bug. ***