374569 - (1.8 branch) Don't allow more than one menu popup at a time, at least from content

Status: RESOLVED INCOMPLETE

(Core :: XUL, defect)

Description

[Daniel Veditz [:dveditz]]

This is spun off from bug 326877 comment 72 remaining issue 1). We really only handle tracking for one popup, so if there are multiple calls to showPopup() some of them get left around. We clean up the one we're tracking, and the others remain to bleed through other tabs and windows.

Instead, assuming it's too much work to track an arbitrary number, we should limit user content to a single working call to showPopup, either returning errors if a popup is already showing or having each showPopup() call close any pre-existing one. I don't care much which but I slightly favor the latter as less breakage should any existing content be using this.

Comment 1

[Neil Deakin]

What security issue is this supposed to fix? Just a situation where a user can be annoyed by multiple popups?

Just as a note, there are several cases where multiple popups will be open in chrome.

1. A menu with submenus
2. Dragging a bookmark from one menu to another
3. Opening a bookmark folder menu and then opening a context menu on a bookmark in the folder.
4. A tooltip attached to any element inside a menu

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #1)
> What security issue is this supposed to fix? Just a situation where a user can
> be annoyed by multiple popups?

If I can get a popup to hang around, I can make bug 326877 comment 76 convincing to the point where it could catch even fairly technical people.

> Just as a note, there are several cases where multiple popups will be open in
> chrome.

Ideally this doesn't break submenus in content XUL.

Comment 3

[Neil Deakin]

But that(In reply to comment #2)

> If I can get a popup to hang around, I can make bug 326877 comment 76
> convincing to the point where it could catch even fairly technical people.

But that would work even if only one popup was allowed.

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #3)
> But that(In reply to comment #2)
> 
> > If I can get a popup to hang around, I can make bug 326877 comment 76
> > convincing to the point where it could catch even fairly technical people.
> 
> But that would work even if only one popup was allowed.
> 

Yes, but with only one popup, we track it properly, so it tends to be killed by doing things like pressing keys.

Comment 5

[Daniel Veditz [:dveditz]]

CTho: can you modify your national bank testcase so that it uses multiple-popups and still works even with the fix for bug 326877? Maybe a demonstration of the problem is necessary.

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #5)
> CTho: can you modify your national bank testcase so that it uses
> multiple-popups and still works even with the fix for bug 326877? Maybe a
> demonstration of the problem is necessary.
> 

I put together a multipopup version but was unable to get it to lose track of my popups reliably (it almost never lost them).  I hit bug 378532 4 or 5 times but couldn't come up with steps to reproduce it - it did seem to be related to the popups being forgotten though.  I'll play with this some more later.

Comment 7

[Daniel Veditz [:dveditz]]

No progress on this one, and looks like bug 326877 itself got reopened, so we'll have to wait until next time.

Comment 8

[Neil Deakin]

After bug 279703, which will be checked in soon, we keep track of all popups properly and close them when clicking outside them.

Sorry for not understanding, but I still don't have a clear description of what this bug is about, or what the security issue is. The testcases in bug 326877 are complicated and don't have any description of what security issue or bug I should be looking for. A simplified testcase would be much more useful.
 

Comment 9

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #8)
> After bug 279703, which will be checked in soon, we keep track of all popups
> properly and close them when clicking outside them.

I think that will make this bug a non-issue then.

> Sorry for not understanding, but I still don't have a clear description of what
> this bug is about, or what the security issue is. The testcases in bug 326877
> are complicated and don't have any description of what security issue or bug I
> should be looking for. A simplified testcase would be much more useful.

I never managed to put together a testcase that worked anywhere close to reliably.  The issue here is that sometimes when multiple popups are present, some of them don't get hidden.  Sorry I can't come up with a usable testcase.