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: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Attachments: |
|
Description
Nate Chapin
2009-07-27 11:06:44 PDT
Created attachment 33557 [details]
patch
Comment on attachment 33557 [details]
patch
ok.
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); Created attachment 34074 [details]
patch2 - missed a window.event accessor
Comment on attachment 34074 [details]
patch2 - missed a window.event accessor
ok.
My solution appears to have broken things, so this is getting reopened. Created attachment 34097 [details]
revert the previous patches
Comment on attachment 34097 [details]
revert the previous patches
ok. Sorry about that.
Revert previous changes at r46784. Comment on attachment 34074 [details]
patch2 - missed a window.event accessor
This flag seems to be confusing the commit queue.
Actually, it just looks like http://trac.webkit.org/changeset/46784 won the race. :) Reopening again for attempt #2 at actually fixing this issue. Created attachment 34499 [details]
patch - Add a DOMWindowEvent CustomSetter to V8DOMWindowCustom.cpp
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> Created attachment 34522 [details]
patch2 - Fixed DOMWindow.idl and cleaned up logic in CodeGeneratorV8.pm
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?
(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. (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. (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? > 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.
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 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. Created attachment 34595 [details]
patch4
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 > ... 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?
(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. Created attachment 34669 [details]
patch5
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 on attachment 34669 [details]
patch5
I'll commit this myself.
Landed as r47130. |