Bug 17164 – REGRESSION: JavaScript pop-up menu appears at wrong location when hovering image at http://news.chinatimes.com/

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Oliver Hunt]

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>

[Oliver Hunt]

Whoops, forgot to say, this regressed in r29425 -- eg. the activationimp patch

[Mark Rowe (bdash)]

I'm working on a reduction.

[Mark Rowe (bdash)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

Probably a duplicate of bug 17015.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

I should also add that simply putting an

eval('item.' + offsetName);

statement before the offending line makes it work right.

[Brad]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Darin Adler]

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.

[Cameron Zwarich (cpst)]

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).

[Oliver Hunt]

Comment on attachment 18925 [details]
Proposed patch

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

r=me

[Cameron Zwarich (cpst)]

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.

[Oliver Hunt]

*** Bug 16889 has been marked as a duplicate of this bug. ***

[Cameron Zwarich (cpst)]

Created attachment 18930 [details]
Revised proposed patch

I added more tests at Oliver's request.

[Oliver Hunt]

Comment on attachment 18930 [details]
Revised proposed patch

r=me!

[Oliver Hunt]

Landed r29997

[Oliver Hunt]

*** Bug 17015 has been marked as a duplicate of this bug. ***