RESOLVED FIXED 27719
[Chromium] Regression in r42671 - js event object being hidden. 
https://bugs.webkit.org/show_bug.cgi?id=27719
Summary [Chromium] Regression in r42671 - js event object being hidden.
Nate Chapin
Reported 2009-07-27 11:06:44 PDT
The event object should be visible to javascript running on a page. Currently, the v8 bindings are setting it as a hidden value, causing certain pages to have unintended behaviors. (Example at www.anselm.edu, the buttons at the top should have menus triggered onmouseover)
Attachments
patch (2.14 KB, patch)
2009-07-27 11:09 PDT, Nate Chapin 		dglazkov: review+
DetailsFormatted DiffDiff
patch2 - missed a window.event accessor (1.30 KB, patch)
2009-08-04 10:18 PDT, Nate Chapin 		dglazkov: review+
DetailsFormatted DiffDiff
revert the previous patches (2.87 KB, patch)
2009-08-04 16:13 PDT, Nate Chapin 		dglazkov: review+
dglazkov: commit-queue+
DetailsFormatted DiffDiff
patch - Add a DOMWindowEvent CustomSetter to V8DOMWindowCustom.cpp (5.09 KB, patch)
2009-08-10 12:44 PDT, Nate Chapin 		dglazkov: review-
DetailsFormatted DiffDiff
patch2 - Fixed DOMWindow.idl and cleaned up logic in CodeGeneratorV8.pm (8.47 KB, patch)
2009-08-10 15:35 PDT, Nate Chapin 		abarth: review-
DetailsFormatted DiffDiff
patch3 - Add second layout test (11.64 KB, patch)
2009-08-11 13:34 PDT, Nate Chapin 		abarth: review-
DetailsFormatted DiffDiff
patch4 (11.68 KB, patch)
2009-08-11 14:37 PDT, Nate Chapin 		abarth: review-
DetailsFormatted DiffDiff
patch5 (11.71 KB, patch)
2009-08-12 10:17 PDT, Nate Chapin 		abarth: review+
japhet: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Nate Chapin
Comment 1 2009-07-27 11:09:12 PDT
Created attachment 33557 [details] patch
Dimitri Glazkov (Google)
Comment 2 2009-07-27 11:12:10 PDT
Comment on attachment 33557 [details] patch ok.
Nate Chapin
Comment 3 2009-07-27 11:19:12 PDT
Landed as r46421.
noel gordon
Comment 4 2009-08-01 03:40:25 PDT
Do you need to update ACCESSOR_GETTER(DOMWindowEvent) in http://trac.webkit.org/browser/trunk/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp?#L146 too? since it tries to extract window.event using: v8::Handle<v8::Value> jsEvent = context->Global()->GetHiddenValue(eventSymbol);
Nate Chapin
Comment 5 2009-08-04 10:18:56 PDT
Created attachment 34074 [details] patch2 - missed a window.event accessor
Dimitri Glazkov (Google)
Comment 6 2009-08-04 10:20:33 PDT
Comment on attachment 34074 [details] patch2 - missed a window.event accessor ok.
Nate Chapin
Comment 7 2009-08-04 10:28:42 PDT
Landed 2nd patch as r46769.
Nate Chapin
Comment 8 2009-08-04 16:11:48 PDT
My solution appears to have broken things, so this is getting reopened.
Nate Chapin
Comment 9 2009-08-04 16:13:52 PDT
Created attachment 34097 [details] revert the previous patches
Dimitri Glazkov (Google)
Comment 10 2009-08-04 16:15:32 PDT
Comment on attachment 34097 [details] revert the previous patches ok. Sorry about that.
Nate Chapin
Comment 11 2009-08-04 16:20:58 PDT
Revert previous changes at r46784.
Adam Barth
Comment 12 2009-08-04 17:36:40 PDT
Comment on attachment 34074 [details] patch2 - missed a window.event accessor This flag seems to be confusing the commit queue.
Adam Barth
Comment 13 2009-08-04 17:41:20 PDT
Actually, it just looks like http://trac.webkit.org/changeset/46784 won the race. :)
Nate Chapin
Comment 14 2009-08-10 12:44:01 PDT
Reopening again for attempt #2 at actually fixing this issue.
Nate Chapin
Comment 15 2009-08-10 12:44:54 PDT
Created attachment 34499 [details] patch - Add a DOMWindowEvent CustomSetter to V8DOMWindowCustom.cpp
Dimitri Glazkov (Google)
Comment 16 2009-08-10 12:56:07 PDT
Comment on attachment 34499 [details] patch - Add a DOMWindowEvent CustomSetter to V8DOMWindowCustom.cpp > > - attribute [Replaceable, CustomGetter] Event event; > + attribute [Custom] Event event; This will break JSC build. Let's just add V8CustomSetter instead. > + * fast/events/manually-nulled-event-expected.txt: Added. > + * fast/events/manually-nulled-event.html: Added. Yucky name. How about setting-event-to-null.html? > +<script language="javascript"> I would rather see this code in <head> element, initiated by body onload="runTest();" > +layoutTestController.dumpAsText(); need to check for layoutTestController existence: if (window.layoutTestController) > + > +</script> > +</body</html>
Nate Chapin
Comment 17 2009-08-10 15:35:37 PDT
Created attachment 34522 [details] patch2 - Fixed DOMWindow.idl and cleaned up logic in CodeGeneratorV8.pm
Adam Barth
Comment 18 2009-08-10 18:54:20 PDT
Comment on attachment 34522 [details] patch2 - Fixed DOMWindow.idl and cleaned up logic in CodeGeneratorV8.pm You should test what happens when you set window.event to null. Currently, you're changing the setter but not actually testing it! Also, I'm skeptical that we should grab CurrentContext in the setter. Shouldn't we grab the window that the setter is being called on?
Nate Chapin
Comment 19 2009-08-11 08:51:01 PDT
(In reply to comment #18) > (From update of attachment 34522 [details]) > You should test what happens when you set window.event to null. Currently, > you're changing the setter but not actually testing it! I'm either misinterpreting this comment or you're misunderstanding the test. Here's what's happening: 1. During page load, window.event gets set to null. In V8, this will call the custom setter (without the custom setter, the custom getter will get swallowed by the nulling of the window.event object). 2. We dispatch a mouse event. V8AbstractEventListener::invokeEvent() sets the event hidden value. 3. We compare event to null. If the custom setter was called, the custom getter will be called here and will access the event hidden value. Otherwise, V8 will return null without using the custom getter and the test will fail. Does that make sense? > > Also, I'm skeptical that we should grab CurrentContext in the setter. > Shouldn't we grab the window that the setter is being called on? You very well may be right here, I copied behavior from the custom getter which may or may not be the correct thing to do.
Adam Barth
Comment 20 2009-08-11 09:03:01 PDT
(In reply to comment #19) > Does that make sense? Ah, I missed that. Can we check the value of window.event before and after setting it to null? Can we also check the value of window.event after the event is done running? Also, why are we checking the current window.event value against undefined? It's going to be in some predictable state at that point. We shouldn't need the branch. > > Also, I'm skeptical that we should grab CurrentContext in the setter. > > Shouldn't we grab the window that the setter is being called on? > > You very well may be right here, I copied behavior from the custom getter which > may or may not be the correct thing to do. You should add a second test to see what the right behavior is here. The way you test this is by having one frame call a function in another frame and then setting the value of event there. The current context will be the window that includes the second function. You might need to use with (parent) { event = null } to get the parent frame's window in scope.
Nate Chapin
Comment 21 2009-08-11 10:12:56 PDT
(In reply to comment #20) > (In reply to comment #19) > > Does that make sense? > > Ah, I missed that. Can we check the value of window.event before and after > setting it to null? Can we also check the value of window.event after the > event is done running? Also, why are we checking the current window.event > value against undefined? It's going to be in some predictable state at that > point. We shouldn't need the branch. Done. The branch was there because I copy/pasted that line of javascript from the website in the original bug report. :-) > > > > Also, I'm skeptical that we should grab CurrentContext in the setter. > > > Shouldn't we grab the window that the setter is being called on? > > > > You very well may be right here, I copied behavior from the custom getter which > > may or may not be the correct thing to do. > > You should add a second test to see what the right behavior is here. The way > you test this is by having one frame call a function in another frame and then > setting the value of event there. The current context will be the window that > includes the second function. You might need to use with (parent) { event = > null } to get the parent frame's window in scope. Will do. Is there a layout test that would serve as a good example for the format of a test like this?
Adam Barth
Comment 22 2009-08-11 10:16:19 PDT
> Will do. Is there a layout test that would serve as a good example for the > format of a test like this? You should look for some in the http/security folder (or one of its subfolders) with "lexical" in its name. There are some test there that do similar things for other APIs. Although don't think there are any that have needed the with() trick yet.
Nate Chapin
Comment 23 2009-08-11 13:34:26 PDT
Created attachment 34587 [details] patch3 - Add second layout test The DOMWindowEvent setter has been modified to use the entered context instead of the current context. Might that be the right answer? :-) Extra scrutiny on the new layout test would also be appreciated, as I'm not confident I'm actually testing the right thing. Chrome, FF, and Safari all give the same results on it though.
Adam Barth
Comment 24 2009-08-11 13:47:00 PDT
Comment on attachment 34587 [details] patch3 - Add second layout test Getting close. A couple points: 1) I don't think you need the with() block in the second test. It's not really doing anything at the moment. You can just say parent.frames[1], etc. 2) You should move the new test to the same folder as the first test. You're not really testing the security of aboutBlank like the other tests in that folder. 3) GetEntered() is also wrong, although you'd need a more complex test to see that. The correct window is the one that the setter is being called on. You can see how to do this by looking at the location setter: http://trac.webkit.org/browser/trunk/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp#L164 The event getter is probably also wrong in the same way at http://trac.webkit.org/browser/trunk/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp#L149 but we can deal with that in another patch.
Nate Chapin
Comment 25 2009-08-11 14:37:51 PDT
Created attachment 34595 [details] patch4
Adam Barth
Comment 26 2009-08-11 14:46:51 PDT
Comment on attachment 34595 [details] patch4 Ok. One more subtle issue, then we should be all set. When you write bindings for the window object, you have to be super careful about security because the window object is exposed across domains. In particular, you need to add an access control check: if (!V8Proxy::canAccessFrame(imp->frame(), true)) return v8::Undefined(); (I'm not sure what that "true" is about, you should investigate before adding this code.) Also, you should be aware that frame() might be null if the window is detached from the frame. In that case, you probably want to return without doing anything. You can look at addEventListener as a model: http://trac.webkit.org/browser/trunk/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp#L197
noel gordon
Comment 27 2009-08-12 06:49:01 PDT
> ... you probably want to return without doing anything. Now there's an idea. Nate could you try that? Define the setter, but don't do anything in the body, just return. Does that fix the www.anselm.edu site?
Nate Chapin
Comment 28 2009-08-12 10:06:18 PDT
(In reply to comment #27) > > ... you probably want to return without doing anything. > > Now there's an idea. Nate could you try that? Define the setter, but don't do > anything in the body, just return. Does that fix the www.anselm.edu site? Yes, an empty custom setter does fix the www.anselm.edu issue. It does, however, create an unnecessary difference between Chromium's behavior and that of FF and Safari browsers (since they both allow window.event to be manually overwritten, and an empty custom setter would prevent that). (In reply to comment #26) > (From update of attachment 34595 [details]) > if (!V8Proxy::canAccessFrame(imp->frame(), true)) > return v8::Undefined(); > > (I'm not sure what that "true" is about, you should investigate before adding > this code.) The "true" indicates that V8Proxy::reportUnsafeAccessTo() should be called if imp->frame() cannot be accessed.
Nate Chapin
Comment 29 2009-08-12 10:17:57 PDT
Created attachment 34669 [details] patch5
Adam Barth
Comment 30 2009-08-12 11:24:11 PDT
Comment on attachment 34669 [details] patch5 Great! Thanks for your patience. (There's some unneeded code in the test, but I don't think we should worry about that too much.)
Nate Chapin
Comment 31 2009-08-12 11:31:55 PDT
Comment on attachment 34669 [details] patch5 I'll commit this myself.
Nate Chapin
Comment 32 2009-08-12 11:44:27 PDT
Landed as r47130.
Note You need to log in before you can comment on or make changes to this bug.