|
Description
Eric Seidel (no email)
2012-01-11 16:34:01 PST
Created attachment 122127 [details]
Patch
This fixes 2 IETC tests: http://samples.msdn.microsoft.com/ietestcenter/domevents/domevents_harness.htm?url=MutationEvent.initMutationEvent.html http://samples.msdn.microsoft.com/ietestcenter/domevents/domevents_harness.htm?url=TextEvent.initTextEvent.html Comment on attachment 122127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122127&action=review > Source/WebCore/dom/Event.idl:90 > + // DOM Level 4 Additions. > + readonly attribute boolean isTrusted; This isn't a DOM4 addition. It's a legacy feature from DOM2: http://www.w3.org/TR/DOM-Level-3-Events/#events-event-type-isTrusted Created attachment 122129 [details]
Patch for landing
Comment on attachment 122129 [details] Patch for landing Rejecting attachment 122129 [details] from commit-queue. New failing tests: fast/events/isTrusted.html fast/xmlhttprequest/xmlhttprequest-get.xhtml http/tests/workers/worker-importScriptsOnError.html Full output: http://queues.webkit.org/results/11110448 Comment on attachment 122129 [details] Patch for landing Rejecting attachment 122129 [details] from commit-queue. New failing tests: fast/events/isTrusted.html fast/xmlhttprequest/xmlhttprequest-get.xhtml http/tests/workers/worker-importScriptsOnError.html Full output: http://queues.webkit.org/results/11212251 I do not think that we should be adding a fake implementation like this. The explanation in ChangeLog doesn't seem to apply to the actual concept, as described in DOM 3 Events. Comment on attachment 122129 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=122129&action=review > Source/WebCore/dom/Event.idl:89 > + readonly attribute boolean isTrusted; This is going to break feature detection :( > This is going to break feature detection :(
That seems like a showstopper to me.
Talking about a full implementation of the property, deciding whether we want it appears somewhat non-trivial. 1. It's not obvious what the use case is. Disclaimer: I didn't attempt to find a discussion that resulted in adding it. 2. HTML5 also specifies when default actions are triggered, and it's unclear if that agrees with DOM (Events) approach. In any case, having this split between two specs is unfortunate and error-prone. 3. While I agree with HTML5 approach, WebKit doesn't fully implement it (see bug 65215), and we can't know if we can practically make the change until we try. Exposing a property that basically tells you whether default handling will take place makes little sense if there are cases where it disagrees with reality in WebKit. "Trusted" in property name is unfortunate, as someone could decide that we found a way to defeat clickjacking. That's actually why I liked the idea of always returning false. None of these events can really be trusted. :) Created attachment 269758 [details]
A tentative patch for ews
This is a tentative patch for ews only to see if it breaks anything.
Comment on attachment 269758 [details] A tentative patch for ews View in context: https://bugs.webkit.org/attachment.cgi?id=269758&action=review > Source/WebCore/dom/EventTarget.cpp:156 > bool EventTarget::dispatchEvent(Event& event) Maybe we rename this to dispatchTrustedEvent() ? Comment on attachment 269758 [details] A tentative patch for ews Attachment 269758 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/737613 Number of test failures exceeded the failure limit. Created attachment 269766 [details]
Archive of layout-test-results from ews101 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 269758 [details] A tentative patch for ews Attachment 269758 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/737592 Number of test failures exceeded the failure limit. Created attachment 269768 [details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 269758 [details] A tentative patch for ews Attachment 269758 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/737619 Number of test failures exceeded the failure limit. Created attachment 269769 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Hmm, the GObject bindings generator hardcodes "dispatchEvent" unfortunately :( Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm: push(@cBodyProperties, " gboolean result = coreTarget->dispatchEvent(coreEvent, ec);\n"); Comment on attachment 269758 [details]
A tentative patch for ews
Looks like you're still missing a few dispatchEvent renames in some idl files (e.g. BatteryManager.idl)
Comment on attachment 269758 [details] A tentative patch for ews View in context: https://bugs.webkit.org/attachment.cgi?id=269758&action=review > Source/WebCore/dom/Event.idl:71 > + [Unforgeable] readonly attribute boolean isTrusted; I would add a blank line before this since this is not a DOM3 addition. Created attachment 269816 [details]
A tentative patch for ews
Created attachment 269824 [details]
A tentative patch for ews
(In reply to comment #25) > Created attachment 269824 [details] > A tentative patch for ews When you post a patch but do not set r? then the EWS does not analyze the patch unless you press the 'Submit patch for EWS analysis' button. (I'm pressing it now!) (In reply to comment #26) > (I'm pressing it now!) Er, just kidding, it's already going. I thought I refreshed the page to check right before leaving that comment. Maybe you got to it right before me. Or magic. Comment on attachment 269824 [details] A tentative patch for ews Attachment 269824 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/739052 Number of test failures exceeded the failure limit. Created attachment 269832 [details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 269824 [details] A tentative patch for ews Attachment 269824 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/739072 Number of test failures exceeded the failure limit. Created attachment 269833 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 269824 [details] A tentative patch for ews Attachment 269824 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/739130 Number of test failures exceeded the failure limit. Created attachment 269836 [details]
Archive of layout-test-results from ews100 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 270017 [details]
a tentative patch for ews
Comment on attachment 270017 [details] a tentative patch for ews Attachment 270017 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/746682 New failing tests: imported/w3c/web-platform-tests/html/semantics/forms/the-button-element/button-events.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/onenter.html imported/w3c/web-platform-tests/html/dom/interfaces.html fast/dom/unforgeable-attributes.html imported/w3c/web-platform-tests/dom/nodes/Document-createEvent.html http/tests/workers/worker-importScriptsOnError.html imported/w3c/web-platform-tests/dom/interfaces.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/checkbox.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/onexit.html fast/xmlhttprequest/xmlhttprequest-get.xhtml imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/radio.html Created attachment 270024 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 270017 [details] a tentative patch for ews Attachment 270017 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/746690 New failing tests: imported/w3c/web-platform-tests/html/semantics/forms/the-button-element/button-events.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/onenter.html fast/dom/unforgeable-attributes.html imported/w3c/web-platform-tests/dom/nodes/Document-createEvent.html http/tests/workers/worker-importScriptsOnError.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/checkbox.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/onexit.html fast/xmlhttprequest/xmlhttprequest-get.xhtml imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/radio.html Created attachment 270025 [details]
Archive of layout-test-results from ews117 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 270017 [details] a tentative patch for ews Attachment 270017 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/746702 New failing tests: imported/w3c/web-platform-tests/html/semantics/forms/the-button-element/button-events.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/onenter.html imported/w3c/web-platform-tests/html/dom/interfaces.html fast/dom/unforgeable-attributes.html imported/w3c/web-platform-tests/dom/nodes/Document-createEvent.html http/tests/workers/worker-importScriptsOnError.html imported/w3c/web-platform-tests/dom/interfaces.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/checkbox.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/onexit.html fast/xmlhttprequest/xmlhttprequest-get.xhtml imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/radio.html Created attachment 270027 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 270138 [details]
A tentative patch for ews
Comment on attachment 270138 [details] A tentative patch for ews View in context: https://bugs.webkit.org/attachment.cgi?id=270138&action=review > Source/WebCore/dom/EventTarget.cpp:156 > bool EventTarget::dispatchEvent(Event& event) Can we call this dispatchTrustedEvent() to avoid mistakes? > Source/WebCore/dom/Node.cpp:2097 > void Node::dispatchScopedEvent(Event& event) Can we call this dispatchTrustedScopedEvent() to avoid mistakes? > Source/WebCore/dom/Node.cpp:2103 > bool Node::dispatchEvent(Event& event) Can we call this dispatchTrustedEvent() to avoid mistakes? (In reply to comment #42) > Comment on attachment 270138 [details] > A tentative patch for ews > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270138&action=review > > > Source/WebCore/dom/EventTarget.cpp:156 > > bool EventTarget::dispatchEvent(Event& event) > > Can we call this dispatchTrustedEvent() to avoid mistakes? > > > Source/WebCore/dom/Node.cpp:2097 > > void Node::dispatchScopedEvent(Event& event) > > Can we call this dispatchTrustedScopedEvent() to avoid mistakes? > > > Source/WebCore/dom/Node.cpp:2103 > > bool Node::dispatchEvent(Event& event) > > Can we call this dispatchTrustedEvent() to avoid mistakes? Do we really need to do this renaming? I don't like this new name as isTrusted is just one of the many properties of an event. If we renamed as dispatchTrustedEvent, it might indicate that TrustedEvent is a special kind of events, and there is another way to dispatch regular events. (In reply to comment #43) > (In reply to comment #42) > > Comment on attachment 270138 [details] > > A tentative patch for ews > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=270138&action=review > > > > > Source/WebCore/dom/EventTarget.cpp:156 > > > bool EventTarget::dispatchEvent(Event& event) > > > > Can we call this dispatchTrustedEvent() to avoid mistakes? > > > > > Source/WebCore/dom/Node.cpp:2097 > > > void Node::dispatchScopedEvent(Event& event) > > > > Can we call this dispatchTrustedScopedEvent() to avoid mistakes? > > > > > Source/WebCore/dom/Node.cpp:2103 > > > bool Node::dispatchEvent(Event& event) > > > > Can we call this dispatchTrustedEvent() to avoid mistakes? > > Do we really need to do this renaming? I don't like this new name as > isTrusted is just one of the many properties of an event. If we renamed as > dispatchTrustedEvent, it might indicate that TrustedEvent is a special kind > of events, and there is another way to dispatch regular events. If we don't do it, then it is very easy for someone to add a new dispatchEvent() call site which is done on behalf of the JS. (In reply to comment #44) > (In reply to comment #43) > > (In reply to comment #42) > > > Comment on attachment 270138 [details] > > > A tentative patch for ews > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=270138&action=review > > > > > > > Source/WebCore/dom/EventTarget.cpp:156 > > > > bool EventTarget::dispatchEvent(Event& event) > > > > > > Can we call this dispatchTrustedEvent() to avoid mistakes? > > > > > > > Source/WebCore/dom/Node.cpp:2097 > > > > void Node::dispatchScopedEvent(Event& event) > > > > > > Can we call this dispatchTrustedScopedEvent() to avoid mistakes? > > > > > > > Source/WebCore/dom/Node.cpp:2103 > > > > bool Node::dispatchEvent(Event& event) > > > > > > Can we call this dispatchTrustedEvent() to avoid mistakes? > > > > Do we really need to do this renaming? I don't like this new name as > > isTrusted is just one of the many properties of an event. If we renamed as > > dispatchTrustedEvent, it might indicate that TrustedEvent is a special kind > > of events, and there is another way to dispatch regular events. > > If we don't do it, then it is very easy for someone to add a new > dispatchEvent() call site which is done on behalf of the JS. What do you mean by that? Created attachment 270226 [details]
Patch
Comment on attachment 270226 [details] Patch Attachment 270226 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/756309 New failing tests: imported/blink/fast/events/event-trusted.html Created attachment 270239 [details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 270226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270226&action=review Looks like a pretty thorough job, but I’m not sure the approach is good. > Source/WebCore/ChangeLog:9 > + Implements Event.isTrusted. The idea is to completely seperate the API of EventTarget::dispatchEvent into Spelling error, it’s separate, not seperate. > Source/WebCore/ChangeLog:10 > + EventTarget::dispatchEventForBindings and EventTarget::dispatchEvent, and set the Event.isTrusted attribute It makes sense to have two separate functions, but I don’t think the two names make it clear what the difference is. As others have suggested, dispatchUntrustedEvent and dispatchTrustedEvent would be a lot easier to understand and less oblique, and we’d have less chance of making an error later if we used those names. I also think that to get this right we need to look at every call site of dispatchEvent, not just the bindings; renaming both versions would help us be sure we did that. The trick here is that when handed an event by script, we need to make sure we never call something that turns it into a trusted event. That requires auditing all the code. I wish we could come up with a design where we couldn’t so easily make the mistake of taking an event created by script and accidentally dispatching it as "trusted". Of course, the flip side is not true. An event created by C++ code would need to become "untrusted" if JavaScript code turned around and dispatched it a second time or to a different object. (If that is possible, not sure it is.) I’m not so sure the basic design is strong enough. A better design might be that all events created by JavaScript code would be marked untrusted. All events created by C++ code would be marked untrusted. And all events passed in from JavaScript to the DOM to functions that might then dispatch the event would become untrusted as part of being passed into C++. There would be no "marking trusted" at all. We certainly would not have dispatchEvent mark an event as trusted as a side effect of calling it! Just because Chromium did it like this does not mean it’s a clear enough design for us to use as-is in WebKit. > Source/WebCore/dom/EventTarget.cpp:162 > +bool EventTarget::dispatchEventImpl(Event& event) I don’t think this is a good name. Perhaps finishDispatchingEvent would be better? > Source/WebCore/dom/Node.cpp:2101 > void Node::dispatchScopedEvent(Event& event) > { > + event.setIsTrusted(true); > EventDispatcher::dispatchScopedEvent(*this, event); > } Completely unclear to me from this name that this is a trusted event and it didn’t come indirectly from script code. The function name doesn’t make that side effect clear at all. I think this shows our design is weak. > Source/WebCore/dom/Node.cpp:2103 > +bool Node::dispatchEventImpl(Event& event) This is a subtle change; it’s not just about “is trusted”. Node’s check for TouchEvent now happens after setting the trusted flag on the touch event, but also after checking isInitialized and isBeingDispatched. Perhaps we now have redundant code in dispatchTouchEvent that can be removed? If not, then maybe this fixes a bug and before some needed checks were skipped? > Source/WebCore/page/EventHandler.cpp:2109 > - dragTarget.dispatchEvent(me.get(), IGNORE_EXCEPTION); > + dragTarget.dispatchEvent(me); Is it OK to assert there is no exception here? What guarantees there won’t be one? Comment on attachment 270226 [details]
Patch
Looks like a test failed.
Comment on attachment 270226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270226&action=review >> Source/WebCore/page/EventHandler.cpp:2109 >> + dragTarget.dispatchEvent(me); > > Is it OK to assert there is no exception here? What guarantees there won’t be one? Yes, this one is fine. dispatchEvent(ev, ec) only throws if: 1. event is null (Not possible here) 2. event has not been initialized (We have initialized it here) 3. event is already being dispatched (not possible here as we have just created it). Comment on attachment 270226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270226&action=review >> Source/WebCore/dom/EventTarget.cpp:162 >> +bool EventTarget::dispatchEventImpl(Event& event) > > I don’t think this is a good name. Perhaps finishDispatchingEvent would be better? I don't know if this is a better name but I have often seen the *Internal() naming used for such methods inside WebKit. finishDispatchingEvent() seems a little bit odd to me because we really haven't started dispatching before calling this function. We've merely set the trusted flag on the event. How about we add a enum class IsTrustedEvent { No, Yes } parameter to dispatchEvent() and get rid of this new method entirely? dispatchEventForBindings() could then call dispatchEvent(ev, IsTrustedEvent::No). >> Source/WebCore/dom/Node.cpp:2101 >> } > > Completely unclear to me from this name that this is a trusted event and it didn’t come indirectly from script code. The function name doesn’t make that side effect clear at all. I think this shows our design is weak. Yes, to address this, I earlier suggested we rename these methods to dispatchTrusted*Event() or that we add a non optional enum class IsTrustedEvent { No, Yes } parameter. Comment on attachment 270226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270226&action=review >>> Source/WebCore/dom/EventTarget.cpp:162 >>> +bool EventTarget::dispatchEventImpl(Event& event) >> >> I don’t think this is a good name. Perhaps finishDispatchingEvent would be better? > > I don't know if this is a better name but I have often seen the *Internal() naming used for such methods inside WebKit. finishDispatchingEvent() seems a little bit odd to me because we really haven't started dispatching before calling this function. We've merely set the trusted flag on the event. How about we add a enum class IsTrustedEvent { No, Yes } parameter to dispatchEvent() and get rid of this new method entirely? dispatchEventForBindings() could then call dispatchEvent(ev, IsTrustedEvent::No). Yes, dispatchEventInternal would be better than dispatchEventImpl. Comment on attachment 270226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270226&action=review >> Source/WebCore/ChangeLog:10 >> + EventTarget::dispatchEventForBindings and EventTarget::dispatchEvent, and set the Event.isTrusted attribute > > It makes sense to have two separate functions, but I don’t think the two names make it clear what the difference is. As others have suggested, dispatchUntrustedEvent and dispatchTrustedEvent would be a lot easier to understand and less oblique, and we’d have less chance of making an error later if we used those names. > > I also think that to get this right we need to look at every call site of dispatchEvent, not just the bindings; renaming both versions would help us be sure we did that. > > The trick here is that when handed an event by script, we need to make sure we never call something that turns it into a trusted event. That requires auditing all the code. I wish we could come up with a design where we couldn’t so easily make the mistake of taking an event created by script and accidentally dispatching it as "trusted". > > Of course, the flip side is not true. An event created by C++ code would need to become "untrusted" if JavaScript code turned around and dispatched it a second time or to a different object. (If that is possible, not sure it is.) > > I’m not so sure the basic design is strong enough. A better design might be that all events created by JavaScript code would be marked untrusted. All events created by C++ code would be marked untrusted. And all events passed in from JavaScript to the DOM to functions that might then dispatch the event would become untrusted as part of being passed into C++. There would be no "marking trusted" at all. We certainly would not have dispatchEvent mark an event as trusted as a side effect of calling it! > > Just because Chromium did it like this does not mean it’s a clear enough design for us to use as-is in WebKit. I agree with you that the design does have some side effects (too easy to set it wrong), and it is very hard to ensure that we have already done it right and we won't do it wrong later. However, the design is specified by the DOM Spec: https://dom.spec.whatwg.org/#dom-event-istrusted "event.isTrusted Returns true if event was dispatched by the user agent, and false otherwise." "The isTrusted attribute must return the value it was initialized to. When an event is created the attribute must be initialized to false." "To fire an event named e means that a new event using the Event interface, with its type attribute initialized to e, and its isTrusted attribute initialized to true, is to be dispatched to the given object." I am not sure whether we should go with the DOM spec or a better design. >> Source/WebCore/dom/Node.cpp:2103 >> +bool Node::dispatchEventImpl(Event& event) > > This is a subtle change; it’s not just about “is trusted”. Node’s check for TouchEvent now happens after setting the trusted flag on the touch event, but also after checking isInitialized and isBeingDispatched. Perhaps we now have redundant code in dispatchTouchEvent that can be removed? If not, then maybe this fixes a bug and before some needed checks were skipped? bool Node::dispatchTouchEvent(TouchEvent& event) { return EventDispatcher::dispatchEvent(this, event); } It will call the EventDispatcher::dispatchEvent just as the ordinary case. So, I guess there is no special treatment for touch event. (In reply to comment #54) > However, the design is specified by the DOM Spec: > https://dom.spec.whatwg.org/#dom-event-istrusted > "event.isTrusted Returns true if event was dispatched by the user agent, and > false otherwise." > "The isTrusted attribute must return the value it was initialized to. When > an event is created the attribute must be initialized to false." > "To fire an event named e means that a new event using the Event interface, > with its type attribute initialized to e, and its isTrusted attribute > initialized to true, is to be dispatched to the given object." I don’t agree that this language dictates the specific implementation we chose in this patch. Despite the fact that the DOM specification uses the phrases "event was dispatched by the user agent", I think that isTrusted should be set to true in our code to *create* events inside WebKit that code intends to dispatch to webpages, and it should be left false on events created by other means such as the JavaScript "new Event" and such. There should be no change to event dispatching functions at all. And we should not even need a setter for isTrusted because we are going to know whether it’s a "created so the user agent can dispatch it" event or not right when we create the event object. That’s the design I would prefer. (In reply to comment #56) > Despite the fact that the DOM specification uses the phrases "event was > dispatched by the user agent", I think that isTrusted should be set to true > in our code to *create* events inside WebKit that code intends to dispatch > to webpages, and it should be left false on events created by other means > such as the JavaScript "new Event" and such. > > There should be no change to event dispatching functions at all. And we > should not even need a setter for isTrusted because we are going to know > whether it’s a "created so the user agent can dispatch it" event or not > right when we create the event object. > > That’s the design I would prefer. One thing to take into consideration is that the JS can take an Event fired by the user agent (and therefore trusted) and then dispatch it again by calling dispatchEvent(eventFromTheUserAgent), in which case it should no longer be trusted. I think this is why dispatchEvent() set the isTrusted flag to false when called: - https://dom.spec.whatwg.org/#dom-eventtarget-dispatchevent (step 2) Therefore, even if we move the isTrusted flag initialization to the constructor, we will likely still need a method to reset the isTrusted flag to false at a later point. (In reply to comment #57) > One thing to take into consideration is that the JS can take an Event fired > by the user agent (and therefore trusted) and then dispatch it again by > calling dispatchEvent(eventFromTheUserAgent), in which case it should no > longer be trusted. Yes, that makes sense. I think I was aware of that originally and even mentioned it. It seems sloppy that this retroactively changes the isTrusted flag on the original event to "false" even in the original context it was being handled in, but I suppose that’s something intrinsic to the design in the DOM specification. I think the main thing I object to is setting trusted to *true* at dispatch time. So easy to use that wrong. I suggest that: 1) New event objects have trusted set to *false* if they are created in the code paths available to JavaScript. 2) New event objects have trusted set to *true* if they are created by C++ code for dispatch by WebKit itself. (Note that this is practical in part because there are are separate constructors to do these jobs in the Event classes.) 3) Calling dispatchEvent from JavaScript sets trusted to false. That’s covered by the patch already attached to this bug. So I think we should start with the attached patch but get rid of the setIsTrusted(bool) function on Event. Instead we should have a Event::setUntrusted() function and we should also change around Event class constructors to initialize m_isTrusted to true in the code paths that are used by C++ code to make events that the C++ code dispatched. This might require some minor reorganization of constructors for Event and the classes derived from it so that there’s not one constructor shared both by code that makes events for JavaScript and for C++. This design is pretty close to what’s in this patch, but removes the possibility of error where C++ code could accidentally re-dispatch an event and mark it trusted as a side effect. It still leaves the possibility of error where we could accidentally allow JavaScript code to create a trusted event instead of an untrusted one. (In reply to comment #58) > (In reply to comment #57) > > One thing to take into consideration is that the JS can take an Event fired > > by the user agent (and therefore trusted) and then dispatch it again by > > calling dispatchEvent(eventFromTheUserAgent), in which case it should no > > longer be trusted. > > Yes, that makes sense. I think I was aware of that originally and even > mentioned it. > > It seems sloppy that this retroactively changes the isTrusted flag on the > original event to "false" even in the original context it was being handled > in, but I suppose that’s something intrinsic to the design in the DOM > specification. > > I think the main thing I object to is setting trusted to *true* at dispatch > time. So easy to use that wrong. I suggest that: > > 1) New event objects have trusted set to *false* if they are created in the > code paths available to JavaScript. > > 2) New event objects have trusted set to *true* if they are created by C++ > code for dispatch by WebKit itself. > > (Note that this is practical in part because there are are separate > constructors to do these jobs in the Event classes.) > > 3) Calling dispatchEvent from JavaScript sets trusted to false. That’s > covered by the patch already attached to this bug. > > So I think we should start with the attached patch but get rid of the > setIsTrusted(bool) function on Event. Instead we should have a > Event::setUntrusted() function and we should also change around Event class > constructors to initialize m_isTrusted to true in the code paths that are > used by C++ code to make events that the C++ code dispatched. This might > require some minor reorganization of constructors for Event and the classes > derived from it so that there’s not one constructor shared both by code that > makes events for JavaScript and for C++. > > This design is pretty close to what’s in this patch, but removes the > possibility of error where C++ code could accidentally re-dispatch an event > and mark it trusted as a side effect. It still leaves the possibility of > error where we could accidentally allow JavaScript code to create a trusted > event instead of an untrusted one. I like this proposal as well. (In reply to comment #58) > (In reply to comment #57) > > One thing to take into consideration is that the JS can take an Event fired > > by the user agent (and therefore trusted) and then dispatch it again by > > calling dispatchEvent(eventFromTheUserAgent), in which case it should no > > longer be trusted. > > Yes, that makes sense. I think I was aware of that originally and even > mentioned it. > > It seems sloppy that this retroactively changes the isTrusted flag on the > original event to "false" even in the original context it was being handled > in, but I suppose that’s something intrinsic to the design in the DOM > specification. > > I think the main thing I object to is setting trusted to *true* at dispatch > time. So easy to use that wrong. I suggest that: > > 1) New event objects have trusted set to *false* if they are created in the > code paths available to JavaScript. > > 2) New event objects have trusted set to *true* if they are created by C++ > code for dispatch by WebKit itself. > > (Note that this is practical in part because there are are separate > constructors to do these jobs in the Event classes.) > > 3) Calling dispatchEvent from JavaScript sets trusted to false. That’s > covered by the patch already attached to this bug. > > So I think we should start with the attached patch but get rid of the > setIsTrusted(bool) function on Event. Instead we should have a > Event::setUntrusted() function and we should also change around Event class > constructors to initialize m_isTrusted to true in the code paths that are > used by C++ code to make events that the C++ code dispatched. This might > require some minor reorganization of constructors for Event and the classes > derived from it so that there’s not one constructor shared both by code that > makes events for JavaScript and for C++. > > This design is pretty close to what’s in this patch, but removes the > possibility of error where C++ code could accidentally re-dispatch an event > and mark it trusted as a side effect. It still leaves the possibility of > error where we could accidentally allow JavaScript code to create a trusted > event instead of an untrusted one. I like this proposal as well. Let me implement it. Thanks! After working on it for a while, I suggest to split the patch into two: 1. rename Event::create(const AtomicString& type, const EventInit& initializer) to Event::createForBindings(const AtomicString& type, const EventInit& initializer) and for all the subclasses as well. Since this one is the DOM API, the isTrusted flag will be unset here using the Event(const AtomicString& type, const EventInit&) constructor in the second patch. Correspondingly, Event::create(const AtomicString& type, bool canBubble, bool cancelable) will set the isTrusted flag. From my inspection, some of the subclasses such as MediaKeyEvent* and UIEventWithKeyState* will use DOM API for C++ code path as well. Therefore, besides renaming, this patch will also cleanup the path. What's more, there is another Event::create() method. This one needs to be rename to Event::createForBindings() as well and a path cleanup. For legacy content, it is combined with Event::initEvent to create an event for bindings. 2. Support Event.isTrusted API. a) Use Event(const AtomicString& type, bool canBubble, bool cancelable)* to set the flag during construction. b) Unset the flag in both Event::initEvent and EventTarget::dispatchEventForBindings. Since the flag is initialized to false, there should be no changes for other constructors which are used by bindings. Created attachment 271070 [details]
Patch
Comment on attachment 271070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271070&action=review Do we have enough coverage for the various cases? I am not sure I see one for all the uses of SimulatedMouseEvent. > Source/WebCore/dom/EventTarget.cpp:152 > + event->setUntrusted(); Maybe we should do this earlier in this function. What’s the downside of doing this just after the null check? I think it‘s actually web-platform-observable if we do that or not for the !isInitialized and !isBeingDispatched cases so we could possibly even test it. > Source/WebCore/dom/MouseEvent.cpp:287 > + // Set SimulatedMouseEvent to untrusted in order to match Firefox's and Chrome's behavior. Do we have test coverage for each of the different ways this class is used, which checks that it’s untrusted? > Source/WebCore/dom/MouseEvent.cpp:288 > + // https://codereview.chromium.org/1285793004 I don’t think the Chromium patch review URL is good to include here. > LayoutTests/imported/blink/fast/events/event-trusted.html:1 > +<input id="clitty-click" type="checkbox"/> I’d suggest a different ID, maybe "test-checkbox" or if you want to have it be fun, "clickety-click". > LayoutTests/imported/w3c/web-platform-tests/dom/interfaces-expected.txt:46 > +FAIL Event interface: document.createEvent("Event") must have own property "isTrusted" assert_equals: getter must be Function expected "function" but got "undefined" What’s the fix for this? Why aren’t see seeing this failure for properties like cancelable? Comment on attachment 271070 [details] Patch Attachment 271070 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/815967 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html imported/w3c/web-platform-tests/dom/interfaces.html Created attachment 271074 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 271070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271070&action=review >> Source/WebCore/dom/MouseEvent.cpp:287 >> + // Set SimulatedMouseEvent to untrusted in order to match Firefox's and Chrome's behavior. > > Do we have test coverage for each of the different ways this class is used, which checks that it’s untrusted? The imported Blink test case cover the basic use case. I will try to figure out more. Comment on attachment 271070 [details] Patch Attachment 271070 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/815973 New failing tests: imported/w3c/web-platform-tests/html/dom/interfaces.html imported/w3c/web-platform-tests/dom/interfaces.html Created attachment 271075 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 271070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271070&action=review >>> Source/WebCore/dom/MouseEvent.cpp:287 >>> + // Set SimulatedMouseEvent to untrusted in order to match Firefox's and Chrome's behavior. >> >> Do we have test coverage for each of the different ways this class is used, which checks that it’s untrusted? > > The imported Blink test case cover the basic use case. I will try to figure out more. After digging into use cases of the SimulatedMouseEvent, it is more complicated than I thought. I think what I was doing here is not correct as the SimulatedMouseEvent could be dispatched by either user agent and bindings. Therefore, we need to separate the way how simulated mouse event is dispatched/created before landing this patch. Created attachment 271206 [details]
Patch for landing
Committed r196520: <http://trac.webkit.org/changeset/196520> (In reply to comment #71) > Committed r196520: <http://trac.webkit.org/changeset/196520> This caused some bindings tests to fail. Ryan Haddad landed updated expected result in <http://trac.webkit.org/changeset/196532>. (In reply to comment #72) > (In reply to comment #71) > > Committed r196520: <http://trac.webkit.org/changeset/196520> > > This caused some bindings tests to fail. Ryan Haddad landed updated expected > result in <http://trac.webkit.org/changeset/196532>. For completeness, one such test failure can be seen in <https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK1%20%28Tests%29/builds/3398/steps/bindings-generation-tests/logs/stdio> These two tests are failing after the change and may also need a rebaseline: inspector/model/remote-object-get-properties.html inspector/model/remote-object.html <https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r196532%20(3314)/results.html> |