Bug 27719

Summary:

[Chromium] Regression in r42671 - js event object being hidden.

Product:

WebKit

Reporter:

Nate Chapin <japhet>

Component:

WebCore Misc.

Assignee:

Nate Chapin <japhet>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dglazkov, noel.gordon

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
patch	dglazkov: review+
patch2 - missed a window.event accessor	dglazkov: review+
revert the previous patches	dglazkov: review+, dglazkov: commit-queue+
patch - Add a DOMWindowEvent CustomSetter to V8DOMWindowCustom.cpp	dglazkov: review-
patch2 - Fixed DOMWindow.idl and cleaned up logic in CodeGeneratorV8.pm	abarth: review-
patch3 - Add second layout test	abarth: review-
patch4	abarth: review-
patch5	abarth: review+, japhet: commit-queue-

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+	Details Formatted Diff Diff
patch2 - missed a window.event accessor (1.30 KB, patch) 2009-08-04 10:18 PDT, Nate Chapin	dglazkov: review+	Details Formatted Diff Diff
revert the previous patches (2.87 KB, patch) 2009-08-04 16:13 PDT, Nate Chapin	dglazkov: review+ dglazkov: commit-queue+	Details Formatted Diff Diff
patch - Add a DOMWindowEvent CustomSetter to V8DOMWindowCustom.cpp (5.09 KB, patch) 2009-08-10 12:44 PDT, Nate Chapin	dglazkov: review-	Details Formatted Diff Diff
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-	Details Formatted Diff Diff
patch3 - Add second layout test (11.64 KB, patch) 2009-08-11 13:34 PDT, Nate Chapin	abarth: review-	Details Formatted Diff Diff
patch4 (11.68 KB, patch) 2009-08-11 14:37 PDT, Nate Chapin	abarth: review-	Details Formatted Diff Diff
patch5 (11.71 KB, patch) 2009-08-12 10:17 PDT, Nate Chapin	abarth: review+ japhet: commit-queue-	Details Formatted Diff Diff
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.