Bug 76121 - WebKit should expose the DOM 4 Event.isTrusted property
Summary: WebKit should expose the DOM 4 Event.isTrusted property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on: 149147 153903
Blocks: 16735 76124 76198 154133
  Show dependency treegraph
 
Reported: 2012-01-11 16:34 PST by Eric Seidel (no email)
Modified: 2017-06-16 16:52 PDT (History)
24 users (show)

See Also:


Attachments
Patch (4.52 KB, patch)
2012-01-11 16:36 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for landing (4.48 KB, patch)
2012-01-11 16:40 PST, Eric Seidel (no email)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
A tentative patch for ews (24.00 KB, patch)
2016-01-25 10:32 PST, Jiewen Tan
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (531.02 KB, application/zip)
2016-01-25 11:22 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (389.83 KB, application/zip)
2016-01-25 11:24 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (514.84 KB, application/zip)
2016-01-25 11:24 PST, Build Bot
no flags Details
A tentative patch for ews (28.68 KB, patch)
2016-01-25 16:51 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
A tentative patch for ews (30.32 KB, patch)
2016-01-25 17:22 PST, Jiewen Tan
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-yosemite (386.81 KB, application/zip)
2016-01-25 18:18 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (533.41 KB, application/zip)
2016-01-25 18:22 PST, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-yosemite (763.89 KB, application/zip)
2016-01-25 18:37 PST, Build Bot
no flags Details
a tentative patch for ews (31.25 KB, patch)
2016-01-27 11:48 PST, Jiewen Tan
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (1.45 MB, application/zip)
2016-01-27 12:43 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (823.17 KB, application/zip)
2016-01-27 12:52 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.13 MB, application/zip)
2016-01-27 12:54 PST, Build Bot
no flags Details
A tentative patch for ews (57.67 KB, patch)
2016-01-28 12:01 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (66.76 KB, patch)
2016-01-29 11:11 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-yosemite (807.22 KB, application/zip)
2016-01-29 12:18 PST, Build Bot
no flags Details
Patch (60.62 KB, patch)
2016-02-11 11:58 PST, Jiewen Tan
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (898.94 KB, application/zip)
2016-02-11 12:58 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (964.96 KB, application/zip)
2016-02-11 13:02 PST, Build Bot
no flags Details
Patch for landing (41.62 KB, patch)
2016-02-12 12:57 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-01-11 16:34:01 PST
WebKit should expose the DOM 4 Event.isTrusted property
Comment 1 Eric Seidel (no email) 2012-01-11 16:36:10 PST
Created attachment 122127 [details]
Patch
Comment 3 Adam Barth 2012-01-11 16:38:45 PST
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
Comment 4 Eric Seidel (no email) 2012-01-11 16:40:42 PST
Created attachment 122129 [details]
Patch for landing
Comment 5 WebKit Review Bot 2012-01-11 17:56:28 PST
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 6 WebKit Review Bot 2012-01-11 19:05:06 PST
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
Comment 7 Alexey Proskuryakov 2012-01-12 10:20:11 PST
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 8 Ryosuke Niwa 2012-01-12 10:29:51 PST
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 :(
Comment 9 Geoffrey Garen 2012-01-12 10:44:35 PST
> This is going to break feature detection :(

That seems like a showstopper to me.
Comment 10 Alexey Proskuryakov 2012-01-12 13:00:23 PST
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.
Comment 11 Adam Barth 2012-01-12 13:30:56 PST
That's actually why I liked the idea of always returning false.  None of these events can really be trusted.  :)
Comment 12 Chris Dumez 2015-09-14 19:02:37 PDT
rdar://problem/22558494
Comment 13 Jiewen Tan 2016-01-25 10:32:43 PST
Created attachment 269758 [details]
A tentative patch for ews

This is a tentative patch for ews only to see if it breaks anything.
Comment 14 Chris Dumez 2016-01-25 10:36:46 PST
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 15 Build Bot 2016-01-25 11:22:37 PST
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.
Comment 16 Build Bot 2016-01-25 11:22:43 PST
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 17 Build Bot 2016-01-25 11:24:15 PST
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.
Comment 18 Build Bot 2016-01-25 11:24:21 PST
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 19 Build Bot 2016-01-25 11:24:29 PST
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.
Comment 20 Build Bot 2016-01-25 11:24:36 PST
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
Comment 21 Chris Dumez 2016-01-25 16:13:15 PST
Hmm, the GObject bindings generator hardcodes "dispatchEvent" unfortunately :(
Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:    push(@cBodyProperties, "    gboolean result = coreTarget->dispatchEvent(coreEvent, ec);\n");
Comment 22 Chris Dumez 2016-01-25 16:14:50 PST
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 23 Chris Dumez 2016-01-25 16:31:32 PST
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.
Comment 24 Jiewen Tan 2016-01-25 16:51:49 PST
Created attachment 269816 [details]
A tentative patch for ews
Comment 25 Jiewen Tan 2016-01-25 17:22:20 PST
Created attachment 269824 [details]
A tentative patch for ews
Comment 26 Michael Catanzaro 2016-01-25 17:32:34 PST
(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!)
Comment 27 Michael Catanzaro 2016-01-25 17:33:51 PST
(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 28 Build Bot 2016-01-25 18:18:46 PST
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.
Comment 29 Build Bot 2016-01-25 18:18:52 PST
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 30 Build Bot 2016-01-25 18:22:19 PST
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.
Comment 31 Build Bot 2016-01-25 18:22:24 PST
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 32 Build Bot 2016-01-25 18:37:01 PST
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.
Comment 33 Build Bot 2016-01-25 18:37:06 PST
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
Comment 34 Jiewen Tan 2016-01-27 11:48:07 PST
Created attachment 270017 [details]
a tentative patch for ews
Comment 35 Build Bot 2016-01-27 12:42:58 PST
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
Comment 36 Build Bot 2016-01-27 12:43:04 PST
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 37 Build Bot 2016-01-27 12:52:17 PST
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
Comment 38 Build Bot 2016-01-27 12:52:23 PST
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 39 Build Bot 2016-01-27 12:54:19 PST
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
Comment 40 Build Bot 2016-01-27 12:54:24 PST
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
Comment 41 Jiewen Tan 2016-01-28 12:01:37 PST
Created attachment 270138 [details]
A tentative patch for ews
Comment 42 Chris Dumez 2016-01-28 12:04:23 PST
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?
Comment 43 Jiewen Tan 2016-01-28 12:14:15 PST
(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.
Comment 44 Chris Dumez 2016-01-28 12:21:00 PST
(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.
Comment 45 Jiewen Tan 2016-01-28 13:10:37 PST
(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?
Comment 46 Jiewen Tan 2016-01-29 11:11:15 PST
Created attachment 270226 [details]
Patch
Comment 47 Build Bot 2016-01-29 12:17:54 PST
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
Comment 48 Build Bot 2016-01-29 12:18:02 PST
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 49 Darin Adler 2016-01-30 11:46:13 PST
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 50 Darin Adler 2016-01-30 11:46:39 PST
Comment on attachment 270226 [details]
Patch

Looks like a test failed.
Comment 51 Chris Dumez 2016-01-30 11:50:10 PST
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 52 Chris Dumez 2016-01-30 13:17:01 PST
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 53 Darin Adler 2016-01-30 13:22:42 PST
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 54 Jiewen Tan 2016-02-01 14:15:26 PST
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.
Comment 55 Darin Adler 2016-02-01 17:49:00 PST
(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.
Comment 56 Darin Adler 2016-02-01 17:53:33 PST
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.
Comment 57 Chris Dumez 2016-02-01 18:43:04 PST
(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.
Comment 58 Darin Adler 2016-02-02 08:31:25 PST
(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.
Comment 59 Chris Dumez 2016-02-02 09:32:13 PST
(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.
Comment 60 Jiewen Tan 2016-02-02 11:20:57 PST
(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!
Comment 61 Jiewen Tan 2016-02-03 10:32:47 PST
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.
Comment 62 Jiewen Tan 2016-02-11 11:58:30 PST
Created attachment 271070 [details]
Patch
Comment 63 Darin Adler 2016-02-11 12:11:24 PST
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 64 Build Bot 2016-02-11 12:58:08 PST
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
Comment 65 Build Bot 2016-02-11 12:58:13 PST
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 66 Jiewen Tan 2016-02-11 13:01:10 PST
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 67 Build Bot 2016-02-11 13:02:16 PST
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
Comment 68 Build Bot 2016-02-11 13:02:21 PST
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 69 Jiewen Tan 2016-02-11 13:35:46 PST
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.
Comment 70 Jiewen Tan 2016-02-12 12:57:45 PST
Created attachment 271206 [details]
Patch for landing
Comment 71 Jiewen Tan 2016-02-12 15:25:05 PST
Committed r196520: <http://trac.webkit.org/changeset/196520>
Comment 72 Daniel Bates 2016-02-12 17:19:28 PST
(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>.
Comment 73 Daniel Bates 2016-02-12 17:20:57 PST
(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>
Comment 74 Ryan Haddad 2016-02-12 17:27:33 PST
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>