Last Comment Bug 319434 - Context menu appears at the wrong position when Flash content is loaded in other tab
: Context menu appears at the wrong position when Flash content is loaded in ot...
Status: RESOLVED FIXED
[rft-dl]
: fixed1.8.0.2, fixed1.8.1, regression, testcase
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Linux
: P2 normal (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap) (please use needinfo!)
:
Mentors:
Depends on:
Blocks: 313566
  Show dependency treegraph
 
Reported: 2005-12-07 08:01 PST by John
Modified: 2008-07-31 03:19 PDT (History)
11 users (show)
dveditz: blocking1.8.0.1-
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (252 bytes, text/html)
2005-12-08 00:06 PST, Andrew Schultz
no flags Details
Branch-safe patch (10.79 KB, patch)
2005-12-19 21:11 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: review+
bzbarsky: superreview+
mtschrep: approval1.8.0.1-
dveditz: approval1.8.0.2+
mtschrep: approval1.8.1+
Details | Diff | Review
Trunk patch (7.98 KB, patch)
2005-12-19 21:30 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: review+
bzbarsky: superreview+
Details | Diff | Review

Description John 2005-12-07 08:01:20 PST
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 2005-12-08 00:06:38 PST
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 Andrew Schultz 2005-12-08 00:26:51 PST
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 2005-12-08 06:05:17 PST
Yes... The test case seems to cause the problem I reported.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-12-08 11:40:50 PST
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) (please use needinfo!) 2005-12-08 23:53:46 PST
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 [:bz] (Out June 25-July 6) 2005-12-09 09:11:37 PST
> 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) (please use needinfo!) 2005-12-19 21:11:22 PST
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.
Comment 8 Blake Kaplan (:mrbkap) (please use needinfo!) 2005-12-19 21:30:21 PST
Created attachment 206362 [details] [diff] [review]
Trunk patch

This is the trunk patch that doesn't add that nasty additional interface.
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2005-12-28 12:13:22 PST
Comment on attachment 206360 [details] [diff] [review]
Branch-safe patch

r=jst
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2005-12-28 12:14:03 PST
Comment on attachment 206362 [details] [diff] [review]
Trunk patch

r=jst
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-12-29 16:09:08 PST
Comment on attachment 206360 [details] [diff] [review]
Branch-safe patch

sr=bzbarsky
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-12-29 16:11:21 PST
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] 2006-01-06 12:24:18 PST
No trunk-baked patch (why didn't it land?), need to push into 1.8.0.2
Comment 14 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-01-06 12:50:39 PST
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 [:bz] (Out June 25-July 6) 2006-01-06 15:24:56 PST
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 [:bz] (Out June 25-July 6) 2006-01-06 15:27:37 PST
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] 2006-01-06 17:40:41 PST
mrbkap, how about attaching the what-landed-on-the-trunk-updated branch patch, with r+ and sr+, nominated?

/be
Comment 18 neil@parkwaycc.co.uk 2006-01-08 06:15:38 PST
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 [:bz] (Out June 25-July 6) 2006-01-08 09:23:08 PST
> 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) (please use needinfo!) 2006-01-08 10:54:13 PST
(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 2006-01-10 13:45:18 PST
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) (please use needinfo!) 2006-02-06 12:28:57 PST
I checked this into the 1.8 branch.
Comment 23 Daniel Veditz [:dveditz] 2006-02-14 15:56:16 PST
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) (please use needinfo!) 2006-02-22 17:28:11 PST
Fix checked into the 1.8.0 branch.
Comment 25 Dave Liebreich [:davel] 2006-03-01 16:20:51 PST
Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)
Comment 26 HJ 2006-03-12 10:42:40 PST
Typo:

-  [noscropt] nsIDOMNode     trustedGetTooltipNode();
+  [noscript] nsIDOMNode     trustedGetTooltipNode();
Comment 27 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-03-12 10:56:49 PST
Fixed that.

Note You need to log in before you can comment on or make changes to this bug.