If you have 2 tabs open and tab 2 contains a site with a blocked pop-up, closing tab 2 results in the blocked pop-up status bar indicator being present on tab 1. This indicator persists through visits to other sites not containing any pop-ups. Steps to reproduce: 1. With 2 browser tabs open. 2. load http://www.mozilla.org/ in tab 1. 3. load http://www.pgatour.com/ in Tab 2. 4. Verify that the blocked pop-up statusbar icon is present in Tab 2. 5. Verify that the blocked pop-up statusbar icon is not present in Tab 1. 6. Close Tab 2. 7. Note that the blocked pop-up statusbar icon is now present in Tab 1. There are really 2 issues here. 1. Why is the blocked pop-up statusbar icon not removed when the tab is closed. 2. Why is the blocked pop-up statusbar icon ever displayed. Isn't it now redundant?

It is a recent regression: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2007-10-19+16%3A00&maxdate=2007-10-19+22%3A00 I think it could be Bug 398135 (by the lack of other possibilities). Do you think it should not display the info icon in the statusbar? I think it is useful information if you have switched off the moving notification bar. And in most cases there is plenty of room there.

I was suggesting removing it because it is redundant if you use the notification bar. I kind of figured people who cared would have the notification bar enabled. But I suppose there are those who just hate the notification bar, yet whould still like to have a blocked popup notification.

bz, any hints for what to look for that we're doing wrong, if it's Fx's fault and not the fault of bug 398135 that it no longer works?

Hmm. So what bug 398135 did is that removing a node from the DOM now fully tears down its XBL binding (which didn't use to happen before). So that's the first thing to look at: are any relevant XBL-bound nodes being removed from the DOM? If so, what happens then? It could well be a core XBL problem, but in that case I'd still love some guidance as to where the relevant UI code lives.

What appears to be broken, is the optimization here: http://lxr.mozilla.org/seamonkey/source/browser/base/content/tabbrowser.xml#711 that tries to avoid calling updatePageReport if the blocked pop-up status is the same on the old and new active page. It appears that this.mCurrentBrowser.pageReport is no longer valid at this point and is now always false.

(In reply to comment #5) > What appears to be broken, is the optimization here: > > http://lxr.mozilla.org/seamonkey/source/browser/base/content/tabbrowser.xml#711 > > that tries to avoid calling updatePageReport if the blocked pop-up status is > the same on the old and new active page. > > It appears that this.mCurrentBrowser.pageReport is no longer valid at this > point and is now always false. > Sorry, always false in the case where you case the active tab, is what I meant to say.

Ah, indeed. <method name="removeTab"> removes the tab node from the DOM (which tears down all the XBL bindings), and only then calls |this.updateCurrentBrowser()|. That means that in updateCurrentBrowser the binding on this.mCurrentBrowser is gone, and this.mCurrentBrowser.pageReport is is undefined. Do we feel that tearing down the binding like this is too incompatible a change? If not, we probably want to change tabbrowser to deal (e.g. null out this.mCurrentBrowser in removeTab, and skip the optimization if !this.mCurrentBrowser.

Or remove the node from the DOM after updating the current tab.

Oddly enough dbaron did this for suite tabbrowser in bug 131456...

I went for the simpler approach. I thought at this stage of the game for 1.9 a simple patch was preferred over moving a lot of code around.

Guess I should have looked into comment #9 more closely.

(In reply to comment #9) > Oddly enough dbaron did this for suite tabbrowser in bug 131456... > So this will fix a leak as well? Great!

Comment on attachment 288091 [details] [diff] [review] Better patch SF isn't needed for /browser

Comment on attachment 288091 [details] [diff] [review] Better patch Sorry. Didn't notice you removed that. Sometimes I have trouble getting bugzilla to fill in review requests.

(In reply to comment #8) > Or remove the node from the DOM after updating the current tab. > In looking at this more closely, I think that even if the moving the code approach is taken, I would still like this patch. Nulling out this.mCurrentBrowser in removeTab is still probably a good idea, and I have no idea exactly how the current code at http://lxr.mozilla.org/seamonkey/source/browser/base/content/tabbrowser.xml#711 works the first time through.

Comment on attachment 288091 [details] [diff] [review] Better patch Not sure why we explicitly call updateCurrentBrowser in the removeTab case, since the onselect handler already takes care of that for us, but that's something for another bug I guess. Sorry for the delay here.

Checking in browser/base/content/tabbrowser.xml; /cvsroot/mozilla/browser/base/content/tabbrowser.xml,v <-- tabbrowser.xml new revision: 1.255; previous revision: 1.254 done

Verified. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008021704 Minefield/3.0b4pre Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3) Gecko/2008020511 Firefox/3.0b3 ID:2008020511