Bug 17164 - REGRESSION: JavaScript pop-up menu appears at wrong location when hovering image at http://news.chinatimes.com/
Summary: REGRESSION: JavaScript pop-up menu appears at wrong location when hovering im...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: Nobody
URL: http://news.chinatimes.com/
Keywords: HasReduction, InRadar, Regression
: 16889 17015 (view as bug list)
Depends on:
Blocks: 16889
  Show dependency treegraph
 
Reported: 2008-02-03 00:09 PST by Oliver Hunt
Modified: 2008-02-05 00:01 PST (History)
7 users (show)

See Also:


Attachments
Partial reduction (28.04 KB, text/html)
2008-02-03 06:06 PST, Mark Rowe (bdash)
no flags Details
Further reduction (5.68 KB, text/html)
2008-02-03 17:16 PST, Cameron Zwarich (cpst)
no flags Details
Further reduction (32.40 KB, text/html)
2008-02-03 18:32 PST, Cameron Zwarich (cpst)
no flags Details
Further reduction (23.84 KB, text/html)
2008-02-03 19:57 PST, Cameron Zwarich (cpst)
no flags Details
Further reduction (146 bytes, text/html)
2008-02-03 21:21 PST, Cameron Zwarich (cpst)
no flags Details
Fix (2.10 KB, patch)
2008-02-04 02:08 PST, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Proposed patch (7.77 KB, patch)
2008-02-04 21:50 PST, Cameron Zwarich (cpst)
oliver: review+
Details | Formatted Diff | Diff
Revised proposed patch (7.74 KB, patch)
2008-02-04 21:56 PST, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised proposed patch (8.37 KB, patch)
2008-02-04 23:15 PST, Cameron Zwarich (cpst)
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2008-02-03 00:09:19 PST
The JS popup menu that appeared was being drawn at the wrong location on the page. When hovering over the mouse over the menu bar (3rd horizontal bar, background color is white, popups initially take a while to load as they are loaded from server) the popup does not appear under the current menu item, but instead hovers on the far left.

<rdar://problem/5720947>
Comment 1 Oliver Hunt 2008-02-03 00:10:08 PST
Whoops, forgot to say, this regressed in r29425 -- eg. the activationimp patch
Comment 2 Mark Rowe (bdash) 2008-02-03 05:23:12 PST
I'm working on a reduction.
Comment 3 Mark Rowe (bdash) 2008-02-03 06:06:29 PST
Created attachment 18883 [details]
Partial reduction

The HTML on this site is insane.  I've spent nearly an hour working on a reduction so far but still have a way to go.  I'm going to put this aside for now and hope that someone else can finish it off.
Comment 4 Cameron Zwarich (cpst) 2008-02-03 17:16:32 PST
Created attachment 18891 [details]
Further reduction

Here is a further reduction of this bug. The next thing to do is to merge in all of the dynamically included JS files. I'll do that soon, but first I need to shovel some snow.
Comment 5 Cameron Zwarich (cpst) 2008-02-03 18:32:01 PST
Created attachment 18895 [details]
Further reduction

This is a further reduction of the bug. It now doesn't load any JS files dynamically. I think I can still clean it up a lot and make the reduction even smaller.
Comment 6 mitz 2008-02-03 18:48:18 PST
Probably a duplicate of bug 17015.
Comment 7 Cameron Zwarich (cpst) 2008-02-03 19:57:08 PST
Created attachment 18896 [details]
Further reduction

Here is a further reduction of the bug. I found out what's going wrong. In the function calculateSumOffset(), the statement

totalOffset += eval('item.' + offsetName);

is not adding the value to totalOffset the first time through the loop. If you move the eval to a different line then it works fine.
Comment 8 Cameron Zwarich (cpst) 2008-02-03 20:18:13 PST
I should also add that simply putting an

eval('item.' + offsetName);

statement before the offending line makes it work right.
Comment 9 Brad 2008-02-03 20:34:51 PST
I just ran into this same thing (as comment #7) in Version 3.0.4 (523.12.2).

Given a form named "pops" and the name of an input of that form assigned to the variable "which", the following does not work (but should):

	var popUpText = "";
	popUpText += eval("document.pops." + which + ".value");
	alert(popUpText)

However, the following modification does work:

	var popUpText = "";
	var thisthing = eval("document.pops." + which + ".value")
	popUpText += thisthing;
	alert(popUpText)

This even simpler modification also works (but limits its use):

	var popUpText = eval("document.pops." + which + ".value")
	alert(popUpText)

So, it seems thatthis version of Webkit does not like combining "+=" with "eval".

The Safari 3.04 does not have this problem, and neither did version 2.
Comment 10 Cameron Zwarich (cpst) 2008-02-03 21:21:14 PST
Created attachment 18898 [details]
Further reduction

This is about as simple as it gets. Some notes:

1) It doesn't even matter if there is a lookup being done in the eval(). I just used a constant here and it still exhibits the problem.

2) The bug is exhibited in function scope but not global scope.
Comment 11 Cameron Zwarich (cpst) 2008-02-03 21:34:50 PST
I found the likely cause of the bug. In the body of ReadModifyLocalVarNode::evaluate(), the slot is retrieved from the localStorage before calling valueForReadModifyAssignment(), which then would cause a tear-off by evaluating the call to eval. Similar problems (albeit more obscure ones) probably exist in the other callers of valueForReadModifyAssignment().

The most straightforward fix is to modify all of the callers of valueForReadModifyAssignment() to simply get the slot in which they will be storing the value one more time. A similar issue came up in the development of the tear-off patch, where using f.arguments could cause a tear-off that would require a slot to be retrieved again, and that's how I fixed it.
Comment 12 Cameron Zwarich (cpst) 2008-02-04 02:08:23 PST
Created attachment 18900 [details]
Fix

Here's a patch that fixes this bug, as well as bug 16889 and bug 17015. I will add a nice big set of layout tests tomorrow, but I need to get some sleep first.
Comment 13 Darin Adler 2008-02-04 09:55:49 PST
Comment on attachment 18900 [details]
Fix

I don't see the point of putting these re-gotten locations into the local variables. I would write this as:

    exec->localStorage()[m_index].value = v;

and

    (*iter)->put(exec, m_ident, v);

Also, I would suggest adding a comment at each of these call sites explaining why we can't use slot and base.
Comment 14 Cameron Zwarich (cpst) 2008-02-04 21:50:14 PST
Created attachment 18925 [details]
Proposed patch

Here's the patch, along with tests. Darin, I agree with your comment so I changed the code. I also added a comment that should have been added to the original tear-off patch (I removed all of the copies of it with references to it, including the one referenced).
Comment 15 Oliver Hunt 2008-02-04 21:52:44 PST
Comment on attachment 18925 [details]
Proposed patch

Add a newline to the end of the test and run sun-spider :D

r=me
Comment 16 Cameron Zwarich (cpst) 2008-02-04 21:56:31 PST
Created attachment 18926 [details]
Revised proposed patch

Oops. I always look at the diff in a text editor, but I didn't scroll to the last line. Here it is without the space

I will try to run SunSpider later tonight, but I need to do some work first.
Comment 17 Oliver Hunt 2008-02-04 22:29:51 PST
*** Bug 16889 has been marked as a duplicate of this bug. ***
Comment 18 Cameron Zwarich (cpst) 2008-02-04 23:15:07 PST
Created attachment 18930 [details]
Revised proposed patch

I added more tests at Oliver's request.
Comment 19 Oliver Hunt 2008-02-04 23:18:51 PST
Comment on attachment 18930 [details]
Revised proposed patch

r=me!
Comment 20 Oliver Hunt 2008-02-05 00:01:17 PST
Landed r29997
Comment 21 Oliver Hunt 2008-02-05 00:01:57 PST
*** Bug 17015 has been marked as a duplicate of this bug. ***