Apparently sometimes installInfo.originatingURI here: http://hg.mozilla.org/mozilla-central/annotate/db7212847c14/browser/base/content/browser-addons.js#l83 is null, which leads to the dialog not being shown. If originatingURI uses the about: or data: protocol, getting its host will also throw. Because of bug 1062631 this exception is currently being swallowed, so the only consequence is that there's no dialog, but that's not very nice. To fix this we probably need a string that indicates "unknown host" (or perhaps we should just create a new string that doesn't include parentheses () with the host at all?). I'm also not sure why originatingURI would be null/undefined here... Dave? Marking backlog+ because bug 1046903 will probably need this to be fixed. (I've picked a points value, but I defer to Dave in potentially adjusting that if this isn't as easy as it seems to me. :-) )

(In reply to :Gijs Kruitbosch from comment #0) > To fix this we probably need a string that indicates "unknown host" (or > perhaps we should just create a new string that doesn't include parentheses > () with the host at all?). I'm also not sure why originatingURI would be > null/undefined here... Dave? Seems like this should actually be a hard error, with an explicit/specific loophole for whatever we do in bug 1046903. If we don't know where an install request is coming from, we shouldn't be making an effort to allow it. (That's also consistent with the move toward only allowing signed addons hosted on AMO.)

(In reply to :Gijs Kruitbosch from comment #0) > Apparently sometimes installInfo.originatingURI here: > > http://hg.mozilla.org/mozilla-central/annotate/db7212847c14/browser/base/ > content/browser-addons.js#l83 > > is null, which leads to the dialog not being shown. That's strange. originatingURI is null in the case where there is no URI triggering the install, so drag and dropping an XPI onto the browser, typing a url to an XPI directly into the url bar are the main cases. In the event that originatingURI is null the install should not be blocked unless the preference xpinstall.whitelist.directRequest is false (Fennec uses this). So you should never get to the "addon-install-blocked" block that you link to with a null originatingURI so there is some strange bug going on here. > (I've picked a points value, but I defer to Dave in potentially adjusting > that if this isn't as easy as it seems to me. :-) ) Looks reasonable

I don't think this needs to be fixed. Bug 1046903 should be solved by allowing the install at an earlier point, we don't want the unknown site install doorhanger for that case.

(In reply to Dave Townsend [:mossop] from comment #2) > So > you should never get to the "addon-install-blocked" block that you link to > with a null originatingURI so there is some strange bug going on here. I'm not usually one to debate WONTFIX much, but considering this was hit in automated testing, should we at least figure out why we're hitting that when you say we shouldn't be? :-)

(In reply to :Gijs Kruitbosch from comment #4) > (In reply to Dave Townsend [:mossop] from comment #2) > > So > > you should never get to the "addon-install-blocked" block that you link to > > with a null originatingURI so there is some strange bug going on here. > > I'm not usually one to debate WONTFIX much, but considering this was hit in > automated testing, should we at least figure out why we're hitting that when > you say we shouldn't be? :-) "maybe" :) I didn't realise we were hitting it in automated testing, any more info here? I would be interested to learn why we're hitting it, right now though it's a case that should only happen because of an unknown bug and the result is to not offer the add-on to the user so we're failing safe. The priority of figuring that out is low to me. If there is a real world case that users hit this and causes problems then sure this is more important. Happy to have a bug on file to recognise it is a problem, but I don't see a need for us to spend time on it right now.

(In reply to Dave Townsend [:mossop] from comment #5) > (In reply to :Gijs Kruitbosch from comment #4) > > (In reply to Dave Townsend [:mossop] from comment #2) > > > So > > > you should never get to the "addon-install-blocked" block that you link to > > > with a null originatingURI so there is some strange bug going on here. > > > > I'm not usually one to debate WONTFIX much, but considering this was hit in > > automated testing, should we at least figure out why we're hitting that when > > you say we shouldn't be? :-) > > "maybe" :) > > I didn't realise we were hitting it in automated testing, any more info here? https://tbpl.mozilla.org/?tree=Try&rev=531613bd40cc 524 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_localfile3.js | uncaught exception - TypeError: installInfo.originatingURI is null at chrome://browser/content/browser.js:269 592 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_whitelist7.js | uncaught exception - TypeError: installInfo.originatingURI is null at chrome://browser/content/browser.js:269 local file sounds like we shouldn't be blocking. Not sure what whitelist7 is about.

(In reply to :Gijs Kruitbosch from comment #6) > (In reply to Dave Townsend [:mossop] from comment #5) > > (In reply to :Gijs Kruitbosch from comment #4) > > > (In reply to Dave Townsend [:mossop] from comment #2) > > > > So > > > > you should never get to the "addon-install-blocked" block that you link to > > > > with a null originatingURI so there is some strange bug going on here. > > > > > > I'm not usually one to debate WONTFIX much, but considering this was hit in > > > automated testing, should we at least figure out why we're hitting that when > > > you say we shouldn't be? :-) > > > > "maybe" :) > > > > I didn't realise we were hitting it in automated testing, any more info here? > > https://tbpl.mozilla.org/?tree=Try&rev=531613bd40cc > > 524 INFO TEST-UNEXPECTED-FAIL | > chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/ > xpinstall/browser_localfile3.js | uncaught exception - TypeError: > installInfo.originatingURI is null at chrome://browser/content/browser.js:269 > 592 INFO TEST-UNEXPECTED-FAIL | > chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/ > xpinstall/browser_whitelist7.js | uncaught exception - TypeError: > installInfo.originatingURI is null at chrome://browser/content/browser.js:269 > > local file sounds like we shouldn't be blocking. Not sure what whitelist7 is > about. Both of these test exactly the scenario I mentioned in comment 2 that Fennec uses with xpinstall.whitelist.directRequest false. It only tests the backend pieces though, so it doesn't spot that the notification bar doesn't appear I guess. Since that pref is really only for Fennec to use and this is an issue in Firefox code I'd suggest just bailing out before attempting to show the blocked notification if originatingURI is null.

(In reply to Dave Townsend [:mossop] from comment #7) > (In reply to :Gijs Kruitbosch from comment #6) > > (In reply to Dave Townsend [:mossop] from comment #5) > > > (In reply to :Gijs Kruitbosch from comment #4) > > > > (In reply to Dave Townsend [:mossop] from comment #2) > > > > > So > > > > > you should never get to the "addon-install-blocked" block that you link to > > > > > with a null originatingURI so there is some strange bug going on here. > > > > > > > > I'm not usually one to debate WONTFIX much, but considering this was hit in > > > > automated testing, should we at least figure out why we're hitting that when > > > > you say we shouldn't be? :-) > > > > > > "maybe" :) > > > > > > I didn't realise we were hitting it in automated testing, any more info here? > > > > https://tbpl.mozilla.org/?tree=Try&rev=531613bd40cc > > > > 524 INFO TEST-UNEXPECTED-FAIL | > > chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/ > > xpinstall/browser_localfile3.js | uncaught exception - TypeError: > > installInfo.originatingURI is null at chrome://browser/content/browser.js:269 > > 592 INFO TEST-UNEXPECTED-FAIL | > > chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/ > > xpinstall/browser_whitelist7.js | uncaught exception - TypeError: > > installInfo.originatingURI is null at chrome://browser/content/browser.js:269 > > > > local file sounds like we shouldn't be blocking. Not sure what whitelist7 is > > about. > > Both of these test exactly the scenario I mentioned in comment 2 that Fennec > uses with xpinstall.whitelist.directRequest false. It only tests the backend > pieces though, so it doesn't spot that the notification bar doesn't appear I > guess. > > Since that pref is really only for Fennec to use and this is an issue in > Firefox code I'd suggest just bailing out before attempting to show the > blocked notification if originatingURI is null. https://hg.mozilla.org/integration/mozilla-inbound/rev/f1169552dd7d does that, so sounds like we're good here! :-)