Summary: | [Chromium] Restrict WebNode::addEventListener and friends to a whitelist of event types | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||
Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | alancutter, dglazkov, eric, esprehn, fishd, fsamuel, jamesr, lazyboy, tkent+wkapi, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Adam Barth
2013-03-12 15:06:12 PDT
Created attachment 192818 [details]
Patch
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 on attachment 192818 [details]
Patch
commit-queue- pending discussion of how to handle the CEF dependency.
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. btw if you do want it to crash in release builds I think you want RELEASE_ASSERT. (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 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? (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. Created attachment 192956 [details]
Patch
(In reply to comment #5) > btw if you do want it to crash in release builds I think you want RELEASE_ASSERT. Doner. Comment on attachment 192956 [details]
Patch
LGTM
Chromium try jobs running in https://codereview.chromium.org/12463035 Comment on attachment 192956 [details]
Patch
Try job results look reasonable. (Unfortunately they're not linked from the CL I pasted above.)
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. (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. 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 (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 on attachment 192956 [details] Patch Clearing flags on attachment: 192956 Committed r145770: <http://trac.webkit.org/changeset/145770> All reviewed patches have been landed. Closing bug. |