526733 - Can't install application/x-xpinstall types by clicking on links in content tabs

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Mark Banner (:standard8)]

If you bring up AMO in a content tab and click on a install button, then you'll get prompted with a dialog to save the xpi - that's because amo is forcing the content disposition to be an attachment which causes us to save it to disk.

Even if we got amo to do user-agent sniffing, it still wouldn't quite work - clicking on a link with content-type application/x-xpinstall actually ends up going no-where.

It turns out we're not handling the xpinstall-install-blocked notification. There's already some code we can copy to do that (see link above), but just before TB 3.0 is not the time to squeeze that in and think about security considerations.

Comment 1

[Mark Banner (:standard8)]

Attachment: WIP fix

This patch implements the general code and strings for doing this and an automated test. There's still a few loose ends to tie up, but storing for archival purposes anyway.

Comment 2

[Mark Banner (:standard8)]

Attachment: The fix

This is the proposed fix. This basically adapts some of the Firefox code to Thunderbird and hooks up the necessarily listener.

Strings are copied from Firefox as well, although they are basically the same as Firefox requesting ui-review from Bryan to make sure he's happy.

MozMill test also included, though there's a few XXX parts. One was that I couldn't get the keypress firing reliably, and the second is that our test infrastructure doesn't seem to like two modal dialogs running in one test. As we've got beta freeze on Tuesday, I thought it best to push forward to review as the code is working and has some basic tests, and follow up on test fixes later.

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 427751 [details] [diff] [review]
The fix

Strings look fine, though I feel like the word "this" is awkward and I would recommend replacing with "the".

i.e.

%1$S prevented this site (%2$S) from asking you to install software on your computer.

Could be:

%1$S prevented the site (%2$S) from asking you to install software on your computer.

Does that seem better?

Comment 4

[Blake Winton (:bwinton) (:☕️)]

If you use "the", I think you could remove the parentheses, to get:

%1$S prevented the site %2$S from asking you to install software on your
computer.

Later,
Blake.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Removing the parens as suggested in comment 4 makes it read awkwardly to me..  The string change proposed in comment 3 seems reasonable to me.  For some future version, my personal inclination would be to remove the words "the site" _and_ the parens.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 427751 [details] [diff] [review]
The fix

http://reviews.visophyte.org/r/427751/

on file: mail/base/content/specialTabs.js line 823
>   observe: function (aSubject, aTopic, aData) {
>     if (aTopic != "mail-startup-done")
>       return;

This probably merits a comment since this looks to the untrained eye like an
observer handler used for many events, but it's coded assuming it will only
ever see one.


on file: mail/base/content/specialTabs.js line None

XPCOMUtils.jsm provides a magic defineLazyServiceGetter function.  That's
arguably more self-documenting than the idiom.


Those are nits.  r=asuth conditional on taking my changes to the mozmill test.  Since I'm going to push this for you, that's a given.  Feel free to go back and fix up the nits if you want and push.  I will not lose sleep either way.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

pushed:
http://hg.mozilla.org/comm-central/rev/80e33dcf91ed

Comment 8

[Mark Banner (:standard8)]

The modified unit tests had very-frequent failures across all platforms and both 1.9.2 and trunk:

TEST-UNEXPECTED-FAIL |  test_xpinstall_disabled
  EXCEPTION: timeout exceeded for waitForEval('subject.alerted')
    at: controller.js line 498
       Error("timeout exceeded for waitForEval('subject.alerted')")  0
            controller.js 498
            test-install-xpi.js 71
       install_xpi() test-install-xpi.js 115
       test_xpinstall_disabled() test-install-xpi.js 171
            frame.js 468
            frame.js 520
            frame.js 562
            frame.js 411
            frame.js 568
            server.js 164
            server.js 168

Comment 9

[Mark Banner (:standard8)]

(hence I backed the test out, but kept the code parts in):

http://hg.mozilla.org/comm-central/rev/a372aa4bea07