319434 - Context menu appears at the wrong position when Flash content is loaded in other tab

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[John]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

This seems to be an interaction between a flash presentation on one tab, and the normal use on other tabs.

I've included the link to test site below, because that's where the problem-causing flash is available most of the time... It took me a long time to discover the interaction between the tabs!

This could actually be an indication of underlying problems, so even if this is not a frequently encountered problem, it might be important.




Reproducible: Always

Steps to Reproduce:
1.Open google.com (test rightclick - all's normal)
2.In another tab, open www.mininova.org, (wait till the emoticon add appears)
3.Go back to the google.com tab, rightclick, and the menu will appear slightly
  differently.
4.Entering the menu box where a submenu is located, it'll jump.

Actual Results:  
Right-click menu changes location

Expected Results:  
it should stay put

Standard theme, only extension installed is Webdevelopper. Using XFCE window
manager, Linux 2.6.14. Gtk-2 and Xft.
Also tried with a self-compiled version of Firefox 1.5

Comment 1

[Andrew Schultz]

Attachment: testcase

1. Load a page in one tab
2. Load this in a second tab
3. Right-click in the second tab
4. Rick-click in the first tab
==> context menu in the wrong spot.

Comment 2

[Andrew Schultz]

this regressed between linux seamonkey trunk builds 2005-10-24-05 and 2005-10-25-05, and also between linux seamonkey branch builds 2005-10-22-19 and 2005-10-24-19, so it looks like bug 313566 is to blame

Comment 3

[John]

Yes... The test case seems to cause the problem I reported.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, this is a regression from bug 313566.  The problem is that the user script flushes reflow, and we try to put up the context menu from reflow (see nsPopupSetFrame::RepositionPopup, leading to nsMenuPopupFrame::AdjustClientXYForNestedDocuments).  This calls nsXULDocument::GetPopupNode to find out _where_ to place the popup...  Problem is, there's just user script and C++ on the stack, so CanCallerAccess returns false.  Hence we mis-position the menu.

The same problem could happen with any C++ consumer of these XUL document APIs, of course -- any time C++ runs while JS is on the stack (say off an event during a sync XMLHttpRequest?) the properties touched by bug 313566 will be unreliable.  Which means that either C++ should use some other API, or that we should push null JSContexts on the stack as needed, or something else.

Blake, let me know if you don't want to deal with this, and I'll try to when I get back in January.  But this is a pretty major regression; I think we want to fix this in 1.8.0.1... :(

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Adding a new api for C++ callers won't do on the branch, though that sounds like the right thing to do on the trunk. The fix will probably be to push a null context (or similar) on both the trunk and branch, and follow up with the cleaner solution later on the trunk.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Adding a new api for C++ callers won't do on the branch

How come?  On branch it'd probably have to go on a new interface instead of nsIXULDocument, but....

I agree that pushing contexts is possibly more straightforward; it just takes a lot of code to do.  :(

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Branch-safe patch

This is a branch-safe patch that fixes this bug. I'm not sure what the names of the two new functions should be. I don't plan on checking this patch in on the trunk; instead, I'm going to cook up a cleaner patch for the trunk that doesn't add the hackey new interface.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Trunk patch

This is the trunk patch that doesn't add that nasty additional interface.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 206360 [details] [diff] [review]
Branch-safe patch

r=jst

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 206362 [details] [diff] [review]
Trunk patch

r=jst

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 206360 [details] [diff] [review]
Branch-safe patch

sr=bzbarsky

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 206362 [details] [diff] [review]
Trunk patch

>Index: dom/public/idl/xul/nsIDOMXULDocument.idl

>+  [noscript] nsIDOMNode     trustedGetPopupNode();
>+  [noscropt] nsIDOMNode     trustedGetTooltipNode();

Please document these.  With decent docs, sr=bzbarsky

Comment 13

[Daniel Veditz [:dveditz]]

No trunk-baked patch (why didn't it land?), need to push into 1.8.0.2

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Trunk patch checked into the proper place. Sorry for the delay, I lost track of this when I went home for the holidays.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 206360 [details] [diff] [review]
Branch-safe patch

I really do think we should take this for 1.8.0.1.  This is a pretty annoying usability regression from a security fix we took on the branch...  The patch is very safe, in my opinion.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Put another way, it sounds like to keep things from missing our security releases we need this triage to happen much earlier before freeze....

Comment 17

[Brendan Eich [:brendan]]

mrbkap, how about attaching the what-landed-on-the-trunk-updated branch patch, with r+ and sr+, nominated?

/be

Comment 18

[[e-mail]]

I don't see why a new API was required here.

In the case of the popup node, the C++ could simply retrieve the node directly from the focus controller, rather than indirectly via the XUL document.

In the case of the tooltip node, the node is always in the same document, except for a chrome document, which can listen for content tooltips.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> I don't see why a new API was required here.

To make the patch as safe as possible...

I think we should seriously rethink having these properties on documents to start with, for what it's worth.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #18)
> In the case of the tooltip node, the node is always in the same document,
> except for a chrome document, which can listen for content tooltips.

If you could prove that, the security check in nsXULDocument::GetTooltipNode could be removed entirely. I considered making the first change, but since I was going to add the new API for the tooltip node, I decided to keep all of the work in the document.

Comment 21

[Mike Schroepfer]

Comment on attachment 206360 [details] [diff] [review]
Branch-safe patch

a-=schrep for drivers.   Would like a little more bake time on the trunk (unfortunate that this got a little lost) given it is not a severe security issue and has a specific set of steps to reproduce.   Nominating for 1.8.0.2 (in March) in which case it should be ready to go.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

I checked this into the 1.8 branch.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 206360 [details] [diff] [review]
Branch-safe patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8.0 branch.

Comment 25

[Dave Liebreich [:davel]]

Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)

Comment 26

[HJ]

Typo:

-  [noscropt] nsIDOMNode     trustedGetTooltipNode();
+  [noscript] nsIDOMNode     trustedGetTooltipNode();

Comment 27

[Boris Zbarsky [:bzbarsky]]

Fixed that.