235454 - list of blocked popup windows not cleared on page reload

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Dan M]

Using a Seamonkey build with bug 198846 checked in, (I'm using a Gecko/20040223
build)

Visit a page that opens a single popup window in its load handler.
  The "blocked popup" icon becomes visible.
  Its context menu contains one item, as it should.
Reload the page.
  The "blocked popup" context menu now contains two entries.

Repeat ad nauseum.

I suppose this is because the locationchange event isn't firing when we reload
the same page. And as things stand you wouldn't really want it to, since our
handler would make duplicate entries in the browser history. But this build-up
of blocked windows seems inappropriate.

Comment 1

[Dan M]

Attachment: trivial testcase

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

I used the code that makes the icon go away when going to a new domain to flush
the list of blocked popups. 
A site might try to open a popup, and then go to a new page. (I think my banking
site does that). If the list would be flushed on every page load, you couldn't
open those popups.
So maybe we need to scan for duplicate urls?

Comment 3

[Dan M]

That seems like it would work.

That would also help with the psycopathic testcase in bug 234726. Go there, wave
your mouse around for a couple of seconds, and get 2000 blocked (identical)
popups. It makes for an unwieldy list.

I'd also consider limiting the list to some reasonable size, even with unique
URLs. My experience with the list of 2000 makes me worry about freakin out of
memory errors if someone tries hard enough. Of course that would be much more
difficult with identical URLs combined.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v1

This patch removes items with the same url before adding new ones. So it
updates the features array in case something changed there.
Also, it limits the list to 100 entries. This number is ofcourse somewhat
random, but i don't think you need > 100 popups from one site :)

Comment 5

[Dan M]

Comment on attachment 142625 [details] [diff] [review]
patch v1

Splicing and pushing, rather than replacing the original entry's features,
seems inefficient. But I suppose it keeps the list in order.

Note testing on my 2GHz P4 Windows OS suggests that the linear scan of a fully
capped out 100 element array will add on the order of 10 msec to the time
required by a debug build to refuse to open a popup window. I think that's OK,
but possibly worthy of note.

r=danm but when you hit the length limit, be sure to shift both the URL and
feature arrays.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 142625 [details] [diff] [review]
patch v1

Consider the missing |browser.popupFeatures.shift();| to be added.

Comment 7

[jag (Peter Annema)]

Comment on attachment 142625 [details] [diff] [review]
patch v1

Looks good to me. sr=jag

Perhaps we should combine these two arrays into one array:

browser.popupData.push({URI: aEvent.popupWindowURI, features:
aEvent.popupWindowFeatures});

and update the users of popup{Urls,Features}.

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

Checked in. I filed bug 236226 for the unification of the two arrays.