Bug 27719 - [Chromium] Regression in r42671 - js event object being hidden.
Summary: [Chromium] Regression in r42671 - js event object being hidden.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-27 11:06 PDT by Nate Chapin
Modified: 2009-08-12 11:44 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 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)
Comment 1 Nate Chapin 2009-07-27 11:09:12 PDT
Created attachment 33557 [details]
patch
Comment 2 Dimitri Glazkov (Google) 2009-07-27 11:12:10 PDT
Comment on attachment 33557 [details]
patch

ok.
Comment 3 Nate Chapin 2009-07-27 11:19:12 PDT
Landed as r46421.
Comment 4 noel gordon 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);
Comment 5 Nate Chapin 2009-08-04 10:18:56 PDT
Created attachment 34074 [details]
patch2 - missed a window.event accessor
Comment 6 Dimitri Glazkov (Google) 2009-08-04 10:20:33 PDT
Comment on attachment 34074 [details]
patch2 - missed a window.event accessor

ok.
Comment 7 Nate Chapin 2009-08-04 10:28:42 PDT
Landed 2nd patch as r46769.
Comment 8 Nate Chapin 2009-08-04 16:11:48 PDT
My solution appears to have broken things, so this is getting reopened.
Comment 9 Nate Chapin 2009-08-04 16:13:52 PDT
Created attachment 34097 [details]
revert the previous patches
Comment 10 Dimitri Glazkov (Google) 2009-08-04 16:15:32 PDT
Comment on attachment 34097 [details]
revert the previous patches

ok. Sorry about that.
Comment 11 Nate Chapin 2009-08-04 16:20:58 PDT
Revert previous changes at r46784.
Comment 12 Adam Barth 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.
Comment 13 Adam Barth 2009-08-04 17:41:20 PDT
Actually, it just looks like  http://trac.webkit.org/changeset/46784 won the race.  :)
Comment 14 Nate Chapin 2009-08-10 12:44:01 PDT
Reopening again for attempt #2 at actually fixing this issue.
Comment 15 Nate Chapin 2009-08-10 12:44:54 PDT
Created attachment 34499 [details]
patch - Add a DOMWindowEvent CustomSetter to V8DOMWindowCustom.cpp
Comment 16 Dimitri Glazkov (Google) 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>
Comment 17 Nate Chapin 2009-08-10 15:35:37 PDT
Created attachment 34522 [details]
patch2 - Fixed DOMWindow.idl and cleaned up logic in CodeGeneratorV8.pm
Comment 18 Adam Barth 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?
Comment 19 Nate Chapin 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.
Comment 20 Adam Barth 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.
Comment 21 Nate Chapin 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?
Comment 22 Adam Barth 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.
Comment 23 Nate Chapin 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.
Comment 24 Adam Barth 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.
Comment 25 Nate Chapin 2009-08-11 14:37:51 PDT
Created attachment 34595 [details]
patch4
Comment 26 Adam Barth 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
Comment 27 noel gordon 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?
Comment 28 Nate Chapin 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.
Comment 29 Nate Chapin 2009-08-12 10:17:57 PDT
Created attachment 34669 [details]
patch5
Comment 30 Adam Barth 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.)
Comment 31 Nate Chapin 2009-08-12 11:31:55 PDT
Comment on attachment 34669 [details]
patch5

I'll commit this myself.
Comment 32 Nate Chapin 2009-08-12 11:44:27 PDT
Landed as r47130.