304357 - InstallTrigger status callbacks always report failure (-210: user cancelled)

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Daniel Veditz [:dveditz]]

The checkin for bug 296566 changed the extension manager to first cancel an
existing nsXPInstallManager and then create a new one. Web pages that have a
trigger callback function to help direct users in case of error are now broken,
as they will now always get the USER_CANCELLED (-210) return code.

I know sites use this because I got a raft of complaints about the error code
returned when we added install site whitelisting.

Killing the install manager and starting a new one has also complicated my
attempt to add install hashes (bug 302284), though since we may also want them
for updates (which call InitManagerFromChrome) that's not why this bug should be
fixed.

Comment 1

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

Attachment: patch

I tested updates and InstallTrigger installs from a web page successfully but
this needs more testing before review. Daniel, can you try it out?

Comment 2

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

For clarity, updates initiated by the EM are still handled using
InitManagerFromChrome and installs from a web page are handled in the manner
before the patch for bug 296566 landed. In regards to update I don't think this
is a problem for bug 302284 since there is no method to add a hash - at least
not yet.

Comment 3

[Daniel Veditz [:dveditz]]

heh, that's very nearly identical to the patch I just finished testing. I forgot
to update the uuid though, so I'll go with yours.

Comment 4

[Daniel Veditz [:dveditz]]

Here's steps to repro:

1: go to https://addons.mozilla.org (or other whitelisted site)
2: in the location bar enter the following
javascript:InstallTrigger.install({'foo':'http://www.mozilla.org/foo.xpi'},function(u,s){alert(u+'\n'+s);});void(0)
3: click "InstallNow" button

Expected result: status -207 (download error, bogus .xpi)
Actual result: status -210 (install cancelled)

Or you could use a valid package url in which case the expected status would be 0.

Comment 5

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

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

Thanks Daniel

Comment 6

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

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

Forgot to update the comment in the interface.

Comment 7

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

Attachment: patch w/ commented interface

Comment 8

[Daniel Veditz [:dveditz]]

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

sr=dveditz
Seeking r= from ben

Should copy the @param comment in the .idl file

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 192432 [details] [diff] [review]
patch w/ commented interface

sr=dveditz

Comment 10

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

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

>+    if (fromChrome) {
>+      // Cancel the existing xpimgr...
>+      gOS.notifyObservers(txn, "xpinstall-progress", "cancel");

Is this necessary anymore now that we're not trying to cancel the web page
xpimgr?

r=[e-mail]

Comment 11

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

Attachment: removed xpinstall cancel

Ben is correct... the xpinstall cancel is no longer needed.

Comment 12

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

Benjamin - this has ben gave this an r+ and dveditz gave this an sr+. Can I get
an  a+ from you when you review it?

Comment 13

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

Carried over Ben's r+ and dveditz's sr+

Fixed for 1.8b4