Bug 112195 – [Chromium] Restrict WebNode::addEventListener and friends to a whitelist of event types

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

[Chromium] Restrict WebNode::addEventListener and friends to a whitelist of event types

[Adam Barth]

Created attachment 192818 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 192818 [details]
Patch

commit-queue- pending discussion of how to handle the CEF dependency.

[Elliott Sprehn]

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.

[Elliott Sprehn]

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

[Adam Barth]

(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.

[Stephen Chenney]

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?

[Adam Barth]

(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.

[Adam Barth]

Created attachment 192956 [details]
Patch

[Adam Barth]

(In reply to comment #5)
> btw if you do want it to crash in release builds I think you want RELEASE_ASSERT.

Doner.

[Elliott Sprehn]

Comment on attachment 192956 [details]
Patch

LGTM

[Adam Barth]

Chromium try jobs running in https://codereview.chromium.org/12463035

[Adam Barth]

Comment on attachment 192956 [details]
Patch

Try job results look reasonable.  (Unfortunately they're not linked from the CL I pasted above.)

[WebKit Review Bot]

Comment on attachment 192956 [details]
Patch

Rejecting attachment 192956 [details] from review queue.

[e-mail] 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.

[Elliott Sprehn]

(In reply to comment #14)
> (From update of attachment 192956 [details])
> Rejecting attachment 192956 [details] from review queue.
> 
> [e-mail] 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.

[Eric Seidel (no email)]

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

[Elliott Sprehn]

(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

[WebKit Review Bot]

Comment on attachment 192956 [details]
Patch

Clearing flags on attachment: 192956

Committed r145770: <http://trac.webkit.org/changeset/145770>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.