304727 - Attempting to install an extension no longer triggers yellow info bar (notification) if a site isn't whitelisted or if software installation is disabled

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Ria Klaassen (not reading all bugmail)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050815 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050815 Firefox/1.0+

Online extension installation does not trigger the yellow notifier bar anymore
if the site is not in the whitelist.
But installation works if the domain is listed under the Allowed Sites.

Reproducible: Always

Steps to Reproduce:
1. Create a new profile with default settings.
2. Go to
http://www.extensionsmirror.nl/index.php?s=f25b7e09f1d6079595b79117a6397c52&showtopic=13
and click on the Install button.
3. An hourglass appears for fraction of a second but further nothing.



Expected Results:  
The notifier bar should appear on the top of the screen.

Comment 1

[Jesse Ruderman]

Dup of bug 303058?

Comment 2

[:Gavin Sharp [email: [e-mail]]]

The two are fairly unrelated, and the fix for that bug is specific to the plugin
bar, so I'd say no. Ria, do you have a regression range?

Comment 3

[Ria Klaassen (not reading all bugmail)]

Between 2005081112 (works) and 2005081121 (fails).

Comment 4

[:Gavin Sharp [email: [e-mail]]]

This was broken with Ben's change to findChildShell, due to the checkin for bug
303848.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Oops, I was mistaken in thinking this affected the blocked popup notification as
well.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Fixes this bug, and a couple strict warnings. This is needed trunk&branch. I've
tested that all the findChildShell callers (popup blocked message, extension
install message, feedview) work as they should with this change.

Comment 7

[Sokraates]

Same problem with FF 1.0.6.

Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.10) Gecko/20050717
Firefox/1.0.6

Comment 8

[mv_van_rantwijk]

(In reply to comment #6)
> Created an attachment (id=192766) [edit]
> patch

This patch will fail when you have two identical tabs open, but it works when
you lookup/match the docShell ;)

Comment 9

[mv_van_rantwijk]

(In reply to comment #8)
> (In reply to comment #6)
> > Created an attachment (id=192766) [edit] [edit]
> > patch
> 
> This patch will fail when you have two identical tabs open, but it works when
> you lookup/match the docShell ;)

Great, I looked at the wrong patch, sorry for the bug spam (:

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> This patch will fail when you have two identical tabs open, but it works when
> you lookup/match the docShell ;)

As far as I see, that comment is correct... I tried to take a shortcut without
making it check for a provided document, and that leads to simply checking the
URI, which is wrong.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch I don't really like

This fixes it, but isn't very pretty. Backing out the changes made by the
feedview landing (bug 305134) is probably a better bet.

Comment 12

[Dave Townsend [:mossop]]

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

Comment 13

[Mike Shaver (:shaver emeritus)]

As a datapoint, the backout of feedview didn't seem to repair this bug. Gavin's
on the case, star that he is.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This seems to work for all cases, but it's late (early!), so I'm going to test
this again later once I've gotten some sleep.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch 2

This works in my testing. Rather trying to maintain a super findChildShell that
uses three different ways to test docShell equality, I split it into two, one
that compares either a document or a URI (for Live Bookmarks and Popup
blocking, respectively), and the other that just compares docShells directly
(for extension install). This effectively reverts extension install to
pre-Feedview, and modifies the function previously used for "Popup blocking" to
be more general, for use with Live bookmarks.

Comment 16

[mv_van_rantwijk]

What about this one?

  _getBrowser: function (aDocShell)
  {
    var tabbrowser = getBrowser();
    var browsers = tabbrowser.browsers;

    for (var i = 0; i < browsers.length; ++i) {
      var browser = tabbrowser.getBrowserAtIndex(i);

      if (this.findChildShell(browser.docShell, aDocShell))
        return browser;
    }
    return null;
  },

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #16)
> What about this one?

What about it? If you're suggesting that I not use the temporary "shell"
variable, that's fine by me. Is there anything else? 

Comment 18

[mv_van_rantwijk]

(In reply to comment #17)
> (In reply to comment #16)
> > What about this one?
> 
> What about it? If you're suggesting that I not use the temporary "shell"
> variable, that's fine by me. Is there anything else? 

Yes, because that makes it a little faster. See also:

browsers.length instead of tabbrowser.browsers.length

Also note that _findChildShell is an exact copy of the function we used for
MultiZilla for over I don't know already, so that should be ok and not fail.

Who is the original developer of this line:

var shellInfo = this._getContentShell(targetDoc, feedURI);

Why did he use feedURI (because he thought that having more feeds on a page
would make any difference perhaps?).

p.s. why are these included:

   aDocShell.QueryInterface(Components.interfaces.nsIWebNavigation);
   aDocShell.QueryInterface(Components.interfaces.nsIInterfaceRequestor);

Why is that?

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #18)
> Who is the original developer of this line:
> var shellInfo = this._getContentShell(targetDoc, feedURI);

Ben Goodger

> Why did he use feedURI (because he thought that having more feeds on a page
> would make any difference perhaps?).

Most likely because the clumsily merged findChildShell took a URI parameter, but
it's redundant, since that URI is just built from document.location.href.

> p.s. why are these included:
>    aDocShell.QueryInterface(Components.interfaces.nsIWebNavigation);
>    aDocShell.QueryInterface(Components.interfaces.nsIInterfaceRequestor);

nsIWebNavigation provides currrentURI and nsIInterfaceRequestor provides
getInterface. Better safe than sorry.

Comment 20

[mv_van_rantwijk]

BTW; is there a spec for this? I mean, what about the location bar, history
items and bookmarks? Should that also trigger a BIM (Browser Info Message)?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #20)
> BTW; is there a spec for this? I mean, what about the location bar, history
> items and bookmarks? Should that also trigger a BIM (Browser Info Message)?

A spec for what? Why would those show browser messages? How is it related to
this bug? :)

Comment 22

[Mike Shaver (:shaver emeritus)]

Comment on attachment 193812 [details] [diff] [review]
patch 2

a=shaver for branch landing.

Comment 23

[mv_van_rantwijk]

(In reply to comment #21)
> (In reply to comment #20)
> > BTW; is there a spec for this? I mean, what about the location bar, history
> > items and bookmarks? Should that also trigger a BIM (Browser Info Message)?
> 
> A spec for what? 

A spec that describes how this should work, i.e. when a BIM should be displayed.

> Why would those show browser messages? 

Steps to reproduce:

1) copy the following link into your locationbar:
http://downloads.mozdev.org/multizilla/multiviews-v1810.xpi and press enter
2) cancel the installation (no BIM displayed)
3) add the link as bookmark
4) click on the bookmark (no BIM displayed)
5) visit www.mozilla.org
6) go back in history and select the XPInstall link (no BIM displayed)

How is it related to this bug? :)

That seems pretty obvious to me ;)

Comment 24

[mv_van_rantwijk]

(In reply to comment #23)

Oh great, that is caused by some extension I had installed (:

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Trunk:
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.492; previous revision: 1.491
done

1.8 Branch:
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.479.2.13; previous revision: 1.479.2.12
done

Comment 26

[Asaf Romano (gone)]

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