Bug 112195 - [Chromium] Restrict WebNode::addEventListener and friends to a whitelist of event types
Summary: [Chromium] Restrict WebNode::addEventListener and friends to a whitelist of e...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-12 15:06 PDT by Adam Barth
Modified: 2013-03-13 16:18 PDT (History)
10 users (show)

See Also:


Attachments
Patch (14.77 KB, patch)
2013-03-12 15:08 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (14.79 KB, patch)
2013-03-13 11:49 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2013-03-12 15:06:12 PDT
[Chromium] Restrict WebNode::addEventListener and friends to a whitelist of event types
Comment 1 Adam Barth 2013-03-12 15:08:40 PDT
Created attachment 192818 [details]
Patch
Comment 2 WebKit Review Bot 2013-03-12 15:10:56 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adam Barth 2013-03-12 15:11:39 PDT
Comment on attachment 192818 [details]
Patch

commit-queue- pending discussion of how to handle the CEF dependency.
Comment 4 Elliott Sprehn 2013-03-12 15:39:48 PDT
Comment on attachment 192818 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192818&action=review

> Source/WebKit/chromium/public/WebNode.h:-112
> -    WEBKIT_EXPORT void removeEventListener(const WebString& eventType, WebDOMEventListener* listener, bool useCapture);

Seems bad that the embedder can't do removeEventListener anymore for mousedown.

> Source/WebKit/chromium/src/WebNode.cpp:188
> +    if (eventType != "permissionrequest")

Why do you want this to crash in release builds? Shouldn't this be ASSERT(eventType != "permissionrequest") ?

> Source/WebKit/chromium/src/WebNode.cpp:197
> +        CRASH();

It's seems weird that we're effectively crippling all embedders of Chromium, but I do see that UIWebView doesn't expose any of this either.
Comment 5 Elliott Sprehn 2013-03-12 15:42:19 PDT
btw if you do want it to crash in release builds I think you want RELEASE_ASSERT.
Comment 6 Adam Barth 2013-03-12 15:47:49 PDT
(In reply to comment #4)
> (From update of attachment 192818 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192818&action=review
> 
> > Source/WebKit/chromium/public/WebNode.h:-112
> > -    WEBKIT_EXPORT void removeEventListener(const WebString& eventType, WebDOMEventListener* listener, bool useCapture);
> 
> Seems bad that the embedder can't do removeEventListener anymore for mousedown.

The one existing caller doesn't do that today.  They just wait for the document to be destroyed.

> > Source/WebKit/chromium/src/WebNode.cpp:188
> > +    if (eventType != "permissionrequest")
> 
> Why do you want this to crash in release builds? Shouldn't this be ASSERT(eventType != "permissionrequest") ?

I do want it to crash in release builds.

> > Source/WebKit/chromium/src/WebNode.cpp:197
> > +        CRASH();
> 
> It's seems weird that we're effectively crippling all embedders of Chromium, but I do see that UIWebView doesn't expose any of this either.

The Chromium WebKit API is not a general-purpose API.  It's an API for the specific purposes of the Chromium project.

I suspect the one caller of addEventListener is wrong.  After this patch lands, I'd like to study the one caller to see if they're doing something sensible or not.

> btw if you do want it to crash in release builds I think you want RELEASE_ASSERT.

Will do.
Comment 7 Stephen Chenney 2013-03-12 16:03:52 PDT
Comment on attachment 192818 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192818&action=review

> Source/WebKit/chromium/ChangeLog:21
> +            - These tests of DOM mutation events are no loner needed because we

Typo : loner -> longer

> Source/WebKit/chromium/src/WebNode.cpp:195
> +    // Please do not add more eventTypes to this list without an API review.

Is it worth commenting on which code uses this in the vain hope that, should the need go away, this code can reflect the change?
Comment 8 Adam Barth 2013-03-13 11:47:11 PDT
(In reply to comment #7)
> (From update of attachment 192818 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192818&action=review
> 
> > Source/WebKit/chromium/src/WebNode.cpp:195
> > +    // Please do not add more eventTypes to this list without an API review.
> 
> Is it worth commenting on which code uses this in the vain hope that, should the need go away, this code can reflect the change?

Hopefully it's not a vain hope.  :)

http://code.google.com/p/chromium/issues/detail?id=189561 is the bug for removing the one caller of hasEventListener.  The one caller of addEventListener will need more study.
Comment 9 Adam Barth 2013-03-13 11:49:04 PDT
Created attachment 192956 [details]
Patch
Comment 10 Adam Barth 2013-03-13 11:49:22 PDT
(In reply to comment #5)
> btw if you do want it to crash in release builds I think you want RELEASE_ASSERT.

Doner.
Comment 11 Elliott Sprehn 2013-03-13 12:18:07 PDT
Comment on attachment 192956 [details]
Patch

LGTM
Comment 12 Adam Barth 2013-03-13 12:28:21 PDT
Chromium try jobs running in https://codereview.chromium.org/12463035
Comment 13 Adam Barth 2013-03-13 15:45:13 PDT
Comment on attachment 192956 [details]
Patch

Try job results look reasonable.  (Unfortunately they're not linked from the CL I pasted above.)
Comment 14 WebKit Review Bot 2013-03-13 15:45:52 PDT
Comment on attachment 192956 [details]
Patch

Rejecting attachment 192956 [details] from review queue.

esprehn@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.
Comment 15 Elliott Sprehn 2013-03-13 15:46:43 PDT
(In reply to comment #14)
> (From update of attachment 192956 [details])
> Rejecting attachment 192956 [details] from review queue.
> 
> esprehn@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.
> 
> - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.
> 
> - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

Err woops, my bad. Forgot to update the committers.py after I became a reviewer.
Comment 16 Eric Seidel (no email) 2013-03-13 15:47:57 PDT
The feeder bot should restart itself every 2 hours... so you should be a reviewer:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py#L277
Comment 17 Elliott Sprehn 2013-03-13 15:50:43 PDT
(In reply to comment #16)
> The feeder bot should restart itself every 2 hours... so you should be a reviewer:
> http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py#L277

That line says Committer, not Reviewer :P
Comment 18 WebKit Review Bot 2013-03-13 16:18:43 PDT
Comment on attachment 192956 [details]
Patch

Clearing flags on attachment: 192956

Committed r145770: <http://trac.webkit.org/changeset/145770>
Comment 19 WebKit Review Bot 2013-03-13 16:18:48 PDT
All reviewed patches have been landed.  Closing bug.