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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XUL
P2
normal
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: John, Assigned: mrbkap)

Tracking

(4 keywords)

Trunk
mozilla1.9alpha1
x86
Linux
fixed1.8.0.2, fixed1.8.1, regression, testcase
Points:
---
Bug Flags:
blocking1.8.0.1 -
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rft-dl])

Attachments

(3 attachments)

(Reporter)

Description

12 years ago
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

Updated

12 years ago
Status: UNCONFIRMED → NEW
Component: General → XP Toolkit/Widgets: Menus
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → xptoolkit.menus
Summary: Right click menu switches position → Context menu appears at the wrong position when Flash content is loaded in other tab
Whiteboard: DUPEME
Version: unspecified → Trunk

Comment 1

12 years ago
Created attachment 205290 [details]
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

12 years ago
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
Keywords: regression, testcase
(Reporter)

Comment 3

12 years ago
Yes... The test case seems to cause the problem I reported.
Blocks: 313566
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... :(
Assignee: nobody → mrbkap
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?

Updated

12 years ago
Whiteboard: DUPEME
(Assignee)

Comment 5

12 years ago
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.
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
> 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.  :(
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 7

12 years ago
Created attachment 206360 [details] [diff] [review]
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.
Attachment #206360 - Flags: review?(jst)
(Assignee)

Comment 8

12 years ago
Created attachment 206362 [details] [diff] [review]
Trunk patch

This is the trunk patch that doesn't add that nasty additional interface.
Attachment #206362 - Flags: review?(jst)
Comment on attachment 206360 [details] [diff] [review]
Branch-safe patch

r=jst
Attachment #206360 - Flags: review?(jst) → review+
Comment on attachment 206362 [details] [diff] [review]
Trunk patch

r=jst
Attachment #206362 - Flags: review?(jst) → review+
(Assignee)

Updated

12 years ago
Attachment #206360 - Flags: superreview?(bzbarsky)
(Assignee)

Updated

12 years ago
Attachment #206362 - Flags: superreview?(bzbarsky)
Comment on attachment 206360 [details] [diff] [review]
Branch-safe patch

sr=bzbarsky
Attachment #206360 - Flags: superreview?(bzbarsky) → superreview+
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
Attachment #206362 - Flags: superreview?(bzbarsky) → superreview+
No trunk-baked patch (why didn't it land?), need to push into 1.8.0.2
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-
(Assignee)

Comment 14

12 years ago
Trunk patch checked into the proper place. Sorry for the delay, I lost track of this when I went home for the holidays.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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.
Attachment #206360 - Flags: approval1.8.1?
Attachment #206360 - Flags: approval1.8.0.1?
Put another way, it sounds like to keep things from missing our security releases we need this triage to happen much earlier before freeze....
mrbkap, how about attaching the what-landed-on-the-trunk-updated branch patch, with r+ and sr+, nominated?

/be

Comment 18

12 years ago
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.
> 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.
(Assignee)

Comment 20

12 years ago
(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

12 years ago
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.
Attachment #206360 - Flags: approval1.8.1?
Attachment #206360 - Flags: approval1.8.1+
Attachment #206360 - Flags: approval1.8.0.2?
Attachment #206360 - Flags: approval1.8.0.1?
Attachment #206360 - Flags: approval1.8.0.1-
(Assignee)

Updated

12 years ago
Keywords: fixed1.8
(Assignee)

Comment 22

12 years ago
I checked this into the 1.8 branch.
Keywords: fixed1.8 → fixed1.8.1
Flags: blocking1.8.0.2? → blocking1.8.0.2+
Comment on attachment 206360 [details] [diff] [review]
Branch-safe patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #206360 - Flags: approval1.8.0.2? → approval1.8.0.2+
(Assignee)

Comment 24

12 years ago
Fix checked into the 1.8.0 branch.
Keywords: fixed1.8.0.2
Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)
Whiteboard: [rft-dl]

Comment 26

12 years ago
Typo:

-  [noscropt] nsIDOMNode     trustedGetTooltipNode();
+  [noscript] nsIDOMNode     trustedGetTooltipNode();
Fixed that.
Flags: blocking1.9a1?
Flags: blocking1.8.1?

Updated

9 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.