252 bytes, text/html
10.79 KB, patch
Mike Schroepfer: approval184.108.40.206-
Mike Schroepfer: approval1.8.1+
|Details | Diff | Splinter Review|
7.98 KB, patch
|Details | Diff | Splinter Review|
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
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.
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
Yes... The test case seems to cause the problem I reported.
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 220.127.116.11... :(
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.
> 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. :(
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.
Created attachment 206362 [details] [diff] [review] Trunk patch This is the trunk patch that doesn't add that nasty additional interface.
Comment on attachment 206360 [details] [diff] [review] Branch-safe patch r=jst
Comment on attachment 206362 [details] [diff] [review] Trunk patch r=jst
Comment on attachment 206360 [details] [diff] [review] Branch-safe patch sr=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
No trunk-baked patch (why didn't it land?), need to push into 18.104.22.168
Trunk patch checked into the proper place. Sorry for the delay, I lost track of this when I went home for the holidays.
Comment on attachment 206360 [details] [diff] [review] Branch-safe patch I really do think we should take this for 22.214.171.124. This is a pretty annoying usability regression from a security fix we took on the branch... The patch is very safe, in my opinion.
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
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.
(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 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 126.96.36.199 (in March) in which case it should be ready to go.
I checked this into the 1.8 branch.
Comment on attachment 206360 [details] [diff] [review] Branch-safe patch approved for 1.8.0 branch, a=dveditz for drivers
Fix checked into the 1.8.0 branch.
Marking [rft-dl] (ready for testing in Firefox 188.8.131.52 release candidates)
Typo: - [noscropt] nsIDOMNode trustedGetTooltipNode(); + [noscript] nsIDOMNode trustedGetTooltipNode();