Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1950403002:
Add the ability to return descedant event listeners. (Closed)
|
Created:
4 years, 7 months ago by dtapuska Modified:
3 years, 9 months ago CC:
chromium-reviews, caseq+blink_chromium.org, dtapuska+blinkwatch_chromium.org, blink-reviews-events_chromium.org, eae+blinkwatch, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, sergeyv+blink_chromium.org, kinuko+watch, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd the ability to return descedant event listeners.
Modify the command line query to return the descedant event listeners
for a given object.
It also accepts a filter string to filter out passive vs blocking listeners.
eg. getEventListeners(document, {descendants: true, passive: false});
BUG=609553
Patch Set 1 #
Total comments: 7
Patch Set 2 : Add v8 context check back #Patch Set 3 : Rebase #
Total comments: 12
Patch Set 4 : Rebase and rewrite to match new flow #
Total comments: 2
Patch Set 5 : Address nits #
Total comments: 18
Patch Set 6 : Address pfeldman's comments #
Total comments: 6
Messages
Total messages: 40 (5 generated)
Description was changed from
==========
Add the ability to return descedant event listeners.
Modify the command line query to return the descedant event listeners
for a given object.
This code traverses inside iframes to return all the event listeners. It
also accepts a filter string to filter out passive vs blocking listeners.
eg. getEventListeners(document, {descendants: true, passive: false});
BUG=609553
==========
to
==========
Add the ability to return descedant event listeners.
Modify the command line query to return the descedant event listeners
for a given object.
This code traverses inside iframes to return all the event listeners. It
also accepts a filter string to filter out passive vs blocking listeners.
eg. getEventListeners(document, {descendants: true, passive: false});
BUG=609553
==========
dtapuska@chromium.org changed reviewers: + caseq@chromium.org, pfeldman@chromium.org
https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp (left): https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:99: v8::Local<v8::Context> context = toV8Context(executionContext, v8Listener->world()); Please look at these lines hard. The checking of the same context was removed for this to work across iframes. I wasn't sure if it should be kept in some scenario; but I couldn't find how to get the v8::Context from the iframe's document. caseq@ was the one that added this check so would like some feedback.
https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp (left): https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:99: v8::Local<v8::Context> context = toV8Context(executionContext, v8Listener->world()); On 2016/05/05 19:51:42, dtapuska wrote: > Please look at these lines hard. The checking of the same context was removed > for this to work across iframes. I wasn't sure if it should be kept in some > scenario; but I couldn't find how to get the v8::Context from the iframe's > document. > > caseq@ was the one that added this check so would like some feedback. The primary use case for getEventListeners() console API call was in fact to expose it to DevTools extensions, so we wanted to ensure that the extensions only see the handlers that the page has installed and not those of other extensions. In general, considering we return actual listener functions, allowing to do this cross execution context sounds scary to me from the security point of view. Perhaps what we could do here is to return actual functions only in case the security origin and the DOM world match?
dtapuska@chromium.org changed reviewers: + rbyers@chromium.org
On 2016/05/05 23:00:04, caseq wrote: > https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp > (left): > > https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:99: > v8::Local<v8::Context> context = toV8Context(executionContext, > v8Listener->world()); > On 2016/05/05 19:51:42, dtapuska wrote: > > Please look at these lines hard. The checking of the same context was removed > > for this to work across iframes. I wasn't sure if it should be kept in some > > scenario; but I couldn't find how to get the v8::Context from the iframe's > > document. > > > > caseq@ was the one that added this check so would like some feedback. > > The primary use case for getEventListeners() console API call was in fact to > expose it to DevTools extensions, so we wanted to ensure that the extensions > only see the handlers that the page has installed and not those of other > extensions. In general, considering we return actual listener functions, > allowing to do this cross execution context sounds scary to me from the security > point of view. Perhaps what we could do here is to return actual functions only > in case the security origin and the DOM world match? This is what I wondered. This feature is a huge benefit for those of us who try to find out why a page is blocking scroll. A simple query like getEventListeners(window, {descendants: true, passive: false}) is vastly more efficient than trying to find all the leaf node paths and showing the ancestors on those paths in the UI. I'd really appreciate if we could think about this before we dismiss the iframe because of security concerns; perhaps there is a way to make this secure with this feature. None the less if I do add back the v8 context origin check the rest of the code essentially remains the same it just means the user would have to execute the getEventListeners in each frame's console context; which is less efficient but still miles ahead of where we are now.
On 2016/05/06 13:02:57, dtapuska wrote: > I'd really appreciate if we could think about this before we dismiss the iframe > because of security concerns; perhaps there is a way to make this secure with this feature. I'm not saying we shouldn't do this at all, I rather think we can find a secure compromise as well. In a nutshell, our threat model for the command line API is that a malicious page will eventually be able to invoke a command line API call, so the privileges those calls expose should be not greater than those of the conventional web API. In your particular implementation, a caller running within one V8 context would be able to get hold of the objects (event listeners) within another context, which would mean a security check induced crash at best, or uncontrolled access to another security origin in the worst case. Some possible ways to mitigate the security risks would be either to not return any function objects in case of the context (or just a security origin) mismatch or only exposing this functionality to the front-end, not the command-line API -- i.e. via the protocol call (getEventListeners() in the DOMDebugger domain) and in the Event Listeners sidebar of the Elements panel. The latter also may be adapted to work with OOPIF, while the command like API would have a problem with OOPIF due to the fact that we expect the call to return synchronously.
https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector/console/resources/eventlistener-in-iframe.html (right): https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector/console/resources/eventlistener-in-iframe.html:2: window.addEventListener("keydown", function(e) {}); I think we'll need to test this with cross-origin iframes as well -- so perhaps move this under http/tests/inspector and make sure we include iframes of both the same and a different origin. https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:1230: if (!options) options = {}; style: move options to the next line, or just options = options || {}
On 2016/05/09 21:07:13, caseq wrote: > On 2016/05/06 13:02:57, dtapuska wrote: > > I'd really appreciate if we could think about this before we dismiss the > iframe > > because of security concerns; perhaps there is a way to make this secure with > this feature. > > I'm not saying we shouldn't do this at all, I rather think we can find a secure > compromise as well. > In a nutshell, our threat model for the command line API is that a malicious > page will eventually > be able to invoke a command line API call, so the privileges those calls expose > should be not > greater than those of the conventional web API. In your particular > implementation, a caller > running within one V8 context would be able to get hold of the objects (event > listeners) within > another context, which would mean a security check induced crash at best, or > uncontrolled access > to another security origin in the worst case. I didn't realize the command line API had a higher security bar than the UI - interesting. Should searching for event listeners across iframes just be a UI feature instead then? I know there was performance concern, but if an explicit button click was involved it would be OK, right? > Some possible ways to mitigate the security risks would be either to not return > any function > objects in case of the context (or just a security origin) mismatch or only > exposing this > functionality to the front-end, not the command-line API -- i.e. via the > protocol call > (getEventListeners() in the DOMDebugger domain) and in the Event Listeners > sidebar of the Elements > panel. > > The latter also may be adapted to work with OOPIF, while the command like API > would have a problem with OOPIF due to the fact that we expect the call to > return synchronously.
On 2016/05/09 21:17:19, Rick Byers wrote: > I didn't realize the command line API had a higher security bar than the UI - > interesting. Should searching for event listeners across iframes just be a UI > feature instead then? I know there was performance concern, but if an explicit > button click was involved it would be OK, right? > Yep, I'm all for that! Pavel, WDYT?
>> I didn't realize the command line API had a higher security bar than the UI - >> interesting. Right, we don't want to enable XSS here, while we can expose everything over the protocol. Showing all the event listeners upon click "Show all" that would reset as the relection changes sounds fine to me. Having said that, we should weight it against the augmenting the DOM with the event listener markers. What is it going to be used for?
On 2016/05/10 20:07:40, pfeldman wrote: > >> I didn't realize the command line API had a higher security bar than the UI - > >> interesting. > > Right, we don't want to enable XSS here, while we can expose everything over the > protocol. Showing all the event listeners upon click "Show all" that would reset > as the relection changes sounds fine to me. Having said that, we should weight > it against the augmenting the DOM with the event listener markers. What is it > going to be used for? So I think it was Dmitry that wanted me to do it this way; via expanding the API. Do you think it is reasonable to limit the call for recursive event listeners to be limited to a frame only. And then the UI could search each frame and call that on each frame that exists in the document. The problem with doing it all in the UI (as a previous approach I took had to load all the nodes in the DOM into devtools). I presume we can just quickly query what frames are inside a document (including recursive frames); can we? As for what is this going to be used for. Is the devtools right now are lacking in their ability to show answer the following question. What touchstart event handlers do I have on the page? Or what wheel event handlers do I have on the page? Because the devtools only show ancestors; you have to walk down to the leave nodes to determine if a said listener is in a page.
On 2016/05/11 00:28:55, dtapuska wrote: > Do you think it is reasonable to limit the call for recursive event listeners > to be limited to a frame only. > > And then the UI could search each frame and call that on each frame that exists > in the document. The problem with doing it all in the UI (as a previous > approach I took had to load all the nodes in the DOM into devtools). I presume > we can just quickly query what frames are inside a document (including recursive > frames); > can we? The important thing here is that the getEventListeners() command line API call that this CL modifies is not related to the UI -- the front-end queries event listeners through the protocol method (see https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit..., implemented by https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). Unlike the command-line API, we presume the front-end to to be well isolated from the page, so there are no security restrictions on what can be exposed there. We do have frame tree structure on the front-end, but I think it would be more practical to keep the frame traversal logic on the back-end (i.e. in renderer) to avoid races with frame creation/deletion. > As for what is this going to be used for. Is the devtools right now are lacking > in their ability to show answer the following question. What touchstart event > handlers do I have on the page? Or what wheel event handlers do I have on the page? > > Because the devtools only show ancestors; you have to walk down to the leave > nodes to determine if a said listener is in a page. This sounds like a reasonable first step to me, but Pavel brought up another possibility in a brief off-line discussion that we had -- since the impact of a listener on a wheel/touch event is determined by the visible area of the event target, it might be more practical to present the listeners graphically. For example, these may be shown on our page overlay (a transparent page that is composited on top of the inspected page and used to draw element outlines when "inspecting" elements, device emulation gutter and controls and other fancy stuff).
On 2016/05/11 00:54:01, caseq wrote: > On 2016/05/11 00:28:55, dtapuska wrote: > > Do you think it is reasonable to limit the call for recursive event listeners > > to be limited to a frame only. > > > > And then the UI could search each frame and call that on each frame that > exists > > in the document. The problem with doing it all in the UI (as a previous > > approach I took had to load all the nodes in the DOM into devtools). I presume > > we can just quickly query what frames are inside a document (including > recursive > > frames); > > can we? > > The important thing here is that the getEventListeners() command line API call > that this CL modifies is not related to the UI -- the front-end queries event > listeners through the protocol method (see > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit..., > implemented by > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > Unlike the command-line API, we presume the front-end to to be well isolated > from the page, so there are no security restrictions on what can be exposed > there. We do have frame tree structure on the front-end, but I think it would be > more practical to keep the frame traversal logic on the back-end (i.e. in > renderer) to avoid races with frame creation/deletion. > > > As for what is this going to be used for. Is the devtools right now are > lacking > > in their ability to show answer the following question. What touchstart event > > handlers do I have on the page? Or what wheel event handlers do I have on the > page? > > > > Because the devtools only show ancestors; you have to walk down to the leave > > nodes to determine if a said listener is in a page. > > This sounds like a reasonable first step to me, but Pavel brought up another > possibility in a brief off-line discussion that we had -- since the impact of a > listener on a wheel/touch event is determined by the visible area of the event > target, it might be more practical to present the listeners graphically. For > example, these may be shown on our page overlay (a transparent page that is > composited on top of the inspected page and used to draw element outlines when > "inspecting" elements, device emulation gutter and controls and other fancy > stuff).
> The important thing here is that the getEventListeners() command line API call > that this CL modifies is not related to the UI -- the front-end queries event > listeners through the protocol method (see > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit..., > implemented by > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > Unlike the command-line API, we presume the front-end to to be well isolated > from the page, so there are no security restrictions on what can be exposed > there. We do have frame tree structure on the front-end, but I think it would be > more practical to keep the frame traversal logic on the back-end (i.e. in > renderer) to avoid races with frame creation/deletion. > Yes I realize this the front end vs back end ultimately it ends up at the same code. I'll work on adjusting this CL so it maintains the context check when the command line call is used; but not when it is called via the protocol method. > This sounds like a reasonable first step to me, but Pavel brought up another > possibility in a brief off-line discussion that we had -- since the impact of a > listener on a wheel/touch event is determined by the visible area of the event > target, it might be more practical to present the listeners graphically. For > example, these may be shown on our page overlay (a transparent page that is > composited on top of the inspected page and used to draw element outlines when > "inspecting" elements, device emulation gutter and controls and other fancy > stuff). Isn't that what perf scrolling issues overlay is showing?
On 2016/05/11 01:06:33, dtapuska wrote: > > > > This sounds like a reasonable first step to me, but Pavel brought up another > > possibility in a brief off-line discussion that we had -- since the impact of > a > > listener on a wheel/touch event is determined by the visible area of the event > > target, it might be more practical to present the listeners graphically. For > > example, these may be shown on our page overlay (a transparent page that is > > composited on top of the inspected page and used to draw element outlines when > > "inspecting" elements, device emulation gutter and controls and other fancy > > stuff). > > Isn't that what perf scrolling issues overlay is showing? Yup, but that we get on another level (from compositor) and we lack the traceability to actual listeners and an ability to deal with those (remove or even force passive), so I guess it currently does not address the use cases that you're trying to solve. But it looks like it possibly could.
On 2016/05/11 01:10:37, caseq wrote: > On 2016/05/11 01:06:33, dtapuska wrote: > > > > > > > > This sounds like a reasonable first step to me, but Pavel brought up another > > > possibility in a brief off-line discussion that we had -- since the impact > of > > a > > > listener on a wheel/touch event is determined by the visible area of the > event > > > target, it might be more practical to present the listeners graphically. For > > > example, these may be shown on our page overlay (a transparent page that is > > > composited on top of the inspected page and used to draw element outlines > when > > > "inspecting" elements, device emulation gutter and controls and other fancy > > > stuff). > > > > Isn't that what perf scrolling issues overlay is showing? > > Yup, but that we get on another level (from compositor) and we lack the > traceability to actual listeners and an ability to deal with those (remove or > even force passive), so I guess it currently does not address the use cases that > you're trying to solve. But it looks like it possibly could. Yeah "show scrolling perf issues" is a total hack with a bunch of limitations. If we could rework it to somehow be able to connect back to the responsible listeners that would be great!
Description was changed from
==========
Add the ability to return descedant event listeners.
Modify the command line query to return the descedant event listeners
for a given object.
This code traverses inside iframes to return all the event listeners. It
also accepts a filter string to filter out passive vs blocking listeners.
eg. getEventListeners(document, {descendants: true, passive: false});
BUG=609553
==========
to
==========
Add the ability to return descedant event listeners.
Modify the command line query to return the descedant event listeners
for a given object.
It also accepts a filter string to filter out passive vs blocking listeners.
eg. getEventListeners(document, {descendants: true, passive: false});
BUG=609553
==========
PTAL; I've added back the v8 context check and removed the iframe traversal. As per comments we can try to do the traversal in the UI; although I'm not sure how that would look. But at least this gives us something powerful on the command line. https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector/console/resources/eventlistener-in-iframe.html (right): https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector/console/resources/eventlistener-in-iframe.html:2: window.addEventListener("keydown", function(e) {}); On 2016/05/09 21:08:39, caseq wrote: > I think we'll need to test this with cross-origin iframes as well -- so perhaps > move this under http/tests/inspector and make sure we include iframes of both > the same and a different origin. Since I have removed traversing iframes completely I thought this might still be ok here. https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp (left): https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:99: v8::Local<v8::Context> context = toV8Context(executionContext, v8Listener->world()); On 2016/05/05 23:00:04, caseq wrote: > On 2016/05/05 19:51:42, dtapuska wrote: > > Please look at these lines hard. The checking of the same context was removed > > for this to work across iframes. I wasn't sure if it should be kept in some > > scenario; but I couldn't find how to get the v8::Context from the iframe's > > document. > > > > caseq@ was the one that added this check so would like some feedback. > > The primary use case for getEventListeners() console API call was in fact to > expose it to DevTools extensions, so we wanted to ensure that the extensions > only see the handlers that the page has installed and not those of other > extensions. In general, considering we return actual listener functions, > allowing to do this cross execution context sounds scary to me from the security > point of view. Perhaps what we could do here is to return actual functions only > in case the security origin and the DOM world match? I've just added back the check for the v8 context matching. Make sense? https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1950403002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:1230: if (!options) options = {}; On 2016/05/09 21:08:39, caseq wrote: > style: move options to the next line, or just options = options || {} Done.
https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp (right): https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:98: if (LocalDOMWindow* window = target->toLocalDOMWindow()) { Can we move this logic up to eventListenersInfoForTarget()? The fact htat we handle tree traversal for descendents of window here and regular DOM traversal there is a bit confusing. https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:3922: { "name": "descendants", "type": "boolean", "optional": true, "description": "True if should return all the event listeners in descendants including this node." } nit: "If set, also include event listeners on all descendant nodes" https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:1263: if (typeof options['passive'] != "undefined") { here and below, double quotes please! Also, use !== instead of != https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:1269: options['descendants'], Put all params on either one or two lines, or use indent of 4. https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/v8_inspector/public/V8EventListenerInfo.h (right): https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/public/V8EventListenerInfo.h:17: static const unsigned EventListenerFilterShowDescendants = 1 << 0; use enum perhaps?
https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/protocol.json (left): https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:3918: "name": "getEventListeners", I guess you also want to update LayoutTests/inspector-protocol/debugger/domdebugger-getEventListeners.html when you touch this. Also, should we expose this in UI?
https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp (right): https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:98: if (LocalDOMWindow* window = target->toLocalDOMWindow()) { On 2016/05/20 00:47:24, caseq wrote: > Can we move this logic up to eventListenersInfoForTarget()? The fact htat we > handle tree traversal for descendents of window here and regular DOM traversal > there is a bit confusing. Ya; sorry this was less confusing when the iframe traversal was here. I've cleaned it up. https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/protocol.json (left): https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:3918: "name": "getEventListeners", On 2016/05/20 01:00:23, caseq wrote: > I guess you also want to update > LayoutTests/inspector-protocol/debugger/domdebugger-getEventListeners.html when > you touch this. > Also, should we expose this in UI? That was the goal to expose it. But it is kind of unclear what the UX would look like. It is kind of weird the event listeners get associated with the object you queried; because they could be low in the tree. I did think perhaps we want to return the objectId or something like that with the response of the event listeners indicating the target. But for what it is worth it does work well with just asking for the listeners on a node and they are associated with that element. https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:3922: { "name": "descendants", "type": "boolean", "optional": true, "description": "True if should return all the event listeners in descendants including this node." } On 2016/05/20 00:47:24, caseq wrote: > nit: "If set, also include event listeners on all descendant nodes" Done. https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:1263: if (typeof options['passive'] != "undefined") { On 2016/05/20 00:47:24, caseq wrote: > here and below, double quotes please! > Also, use !== instead of != removed after rebase https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:1269: options['descendants'], On 2016/05/20 00:47:24, caseq wrote: > Put all params on either one or two lines, or use indent of 4. removed after rebase https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/v8_inspector/public/V8EventListenerInfo.h (right): https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/public/V8EventListenerInfo.h:17: static const unsigned EventListenerFilterShowDescendants = 1 << 0; On 2016/05/20 00:47:24, caseq wrote: > use enum perhaps? Enums that are bitsets aren't permitted in chromium; but are kind of allowed in blink. What do you want me to do?
lgtm % comments On 2016/05/26 20:37:44, dtapuska wrote: > https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... > File > third_party/WebKit/Source/platform/v8_inspector/public/V8EventListenerInfo.h > (right): > > https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/v8_inspector/public/V8EventListenerInfo.h:17: > static const unsigned EventListenerFilterShowDescendants = 1 << 0; > On 2016/05/20 00:47:24, caseq wrote: > > use enum perhaps? > > Enums that are bitsets aren't permitted in chromium; but are kind of allowed in > blink. What do you want me to do? Let's go for enums, at least that would get us a more descriptive type than "unsigned" in function signatures. Also, these seem to be pretty common in blink: https://code.google.com/p/chromium/codesearch#search/&q=f:webkit/source.*h$%2...
https://codereview.chromium.org/1950403002/diff/50001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector-protocol/debugger/domdebugger-getEventListeners.html (right): https://codereview.chromium.org/1950403002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/debugger/domdebugger-getEventListeners.html:90: var expression = "(function(){\n\ why not just expression = "window"? https://codereview.chromium.org/1950403002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/debugger/domdebugger-getEventListeners.html:137: var div = document.getElementById(\"listeners1\");\n\ ditto, just "document.getElemenyById(...)", or even "$('#listeners1'), as command-line API should work -- the only reason we wrap things into a (function() {})() elsewhere is to prevent global namespace pollution when vars are needed.
On 2016/05/26 22:36:25, caseq wrote: > https://codereview.chromium.org/1950403002/diff/50001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/inspector-protocol/debugger/domdebugger-getEventListeners.html > (right): > > https://codereview.chromium.org/1950403002/diff/50001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/inspector-protocol/debugger/domdebugger-getEventListeners.html:90: > var expression = "(function(){\n\ > why not just expression = "window"? > > https://codereview.chromium.org/1950403002/diff/50001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/inspector-protocol/debugger/domdebugger-getEventListeners.html:137: > var div = document.getElementById(\"listeners1\");\n\ > ditto, just "document.getElemenyById(...)", or even "$('#listeners1'), as > command-line API should work -- the only reason we wrap things into a > (function() {})() elsewhere is to prevent global namespace pollution when vars > are needed. Done
On 2016/05/26 22:33:23, caseq wrote: > lgtm % comments > > On 2016/05/26 20:37:44, dtapuska wrote: > > > https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... > > File > > third_party/WebKit/Source/platform/v8_inspector/public/V8EventListenerInfo.h > > (right): > > > > > https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/v8_inspector/public/V8EventListenerInfo.h:17: > > static const unsigned EventListenerFilterShowDescendants = 1 << 0; > > On 2016/05/20 00:47:24, caseq wrote: > > > use enum perhaps? > > > > Enums that are bitsets aren't permitted in chromium; but are kind of allowed > in > > blink. What do you want me to do? > > Let's go for enums, at least that would get us a more descriptive type than > "unsigned" in function signatures. Also, these seem to be pretty common in > blink: > > https://code.google.com/p/chromium/codesearch#search/&q=f:webkit/source.*h$%2... Done; I've moved this definition into the InspectorDOMDebuggerAgent.h
On 2016/05/27 15:26:14, dtapuska wrote: > On 2016/05/26 22:33:23, caseq wrote: > > lgtm % comments > > > > On 2016/05/26 20:37:44, dtapuska wrote: > > > > > > https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... > > > File > > > third_party/WebKit/Source/platform/v8_inspector/public/V8EventListenerInfo.h > > > (right): > > > > > > > > > https://codereview.chromium.org/1950403002/diff/30015/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/platform/v8_inspector/public/V8EventListenerInfo.h:17: > > > static const unsigned EventListenerFilterShowDescendants = 1 << 0; > > > On 2016/05/20 00:47:24, caseq wrote: > > > > use enum perhaps? > > > > > > Enums that are bitsets aren't permitted in chromium; but are kind of allowed > > in > > > blink. What do you want me to do? > > > > Let's go for enums, at least that would get us a more descriptive type than > > "unsigned" in function signatures. Also, these seem to be pretty common in > > blink: > > > > > https://code.google.com/p/chromium/codesearch#search/&q=f:webkit/source.*h$%2... > > Done; I've moved this definition into the InspectorDOMDebuggerAgent.h pfeldman@ can you take a look at this now?
https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h (right): https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h:56: ShowNothing = 0, We don't really need nothing as an option. https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h:57: ShowDescendants = 1 << 0, These seem to be orthogonal to passive/blocking, we should not use the same set for them. If that is that is a mast, you should not have 2 entries for blocking/passive. https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp (right): https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:320: InspectorDOMDebuggerAgent::eventListenersInfoForTarget(isolate, info[0], filterMask, listenerInfo); We should not mix data from different worlds in the same object, binding should only list listeners that are local to the execution context.
https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h (right): https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h:56: ShowNothing = 0, On 2016/05/27 21:54:01, pfeldman wrote: > We don't really need nothing as an option. It is mainly for comparison. Because this is an enum class you can't compare it against 0. You need to test something of the same type. https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h:57: ShowDescendants = 1 << 0, On 2016/05/27 21:54:01, pfeldman wrote: > These seem to be orthogonal to passive/blocking, we should not use the same set > for them. If that is that is a mast, you should not have 2 entries for > blocking/passive. How do you indicate the default. Show me all passive and blocking listeners.
https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp (right): https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:320: InspectorDOMDebuggerAgent::eventListenersInfoForTarget(isolate, info[0], filterMask, listenerInfo); On 2016/05/27 21:54:01, pfeldman wrote: > We should not mix data from different worlds in the same object, binding should > only list listeners that are local to the execution context. Yes there is a check inside the query. Is there something wrong with this code?
On 2016/05/27 21:54:01, pfeldman wrote: > https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:320: > InspectorDOMDebuggerAgent::eventListenersInfoForTarget(isolate, info[0], > filterMask, listenerInfo); > We should not mix data from different worlds in the same object, binding should > only list listeners that are local to the execution context. I think this actually works this way due to filtering by the execution context having been moved to InspectorDOMDebuggerAgent::eventListenersInfoForTarget() previously () We should eventually bring it back here, but this could as well be done by a separate patch.
On 2016/05/27 22:10:53, dtapuska wrote: > https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h > (right): > > https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h:56: > ShowNothing = 0, > On 2016/05/27 21:54:01, pfeldman wrote: > > We don't really need nothing as an option. > > It is mainly for comparison. Because this is an enum class you can't compare it > against 0. You need to test something of the same type. Why are you making it a class btw? Can we just go with a plain old int-based enum and save on re-implementing bit-wise ops?
https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp (right): https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:84: if (context != mainContext) We should use access checks instead of context comparison. https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:117: if (!target) Why would we fall back to the window? https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:125: if ((filterMask & EventListenerFilter::ShowDescendants) != EventListenerFilter::ShowNothing) { Get rid of the ShowNothing, simply if w/ mask test. https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:127: if (LocalDOMWindow* window = target->toLocalDOMWindow()) { What if target is not window? https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:138: addEventListenersForEventTarget(&element, filterMask, mainContext, eventInformation); Do you intend to cross the frame boundary? https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp (right): https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:320: InspectorDOMDebuggerAgent::eventListenersInfoForTarget(isolate, info[0], filterMask, listenerInfo); We want InspectorDOMDebuggerAgent::eventListenersInfoForTarget to report all the listeners, cross the iframe boundary and not do the security checks. Security checks should be placed here instead.
On 2016/05/27 22:19:15, caseq wrote: > On 2016/05/27 22:10:53, dtapuska wrote: > > > https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h > > (right): > > > > > https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h:56: > > ShowNothing = 0, > > On 2016/05/27 21:54:01, pfeldman wrote: > > > We don't really need nothing as an option. > > > > It is mainly for comparison. Because this is an enum class you can't compare > it > > against 0. You need to test something of the same type. > > Why are you making it a class btw? Can we just go with a plain old int-based > enum and save on re-implementing bit-wise ops? This is the newer pattern in chromium so it is strongly typed. Ie. You'll get a compilation error this way. But if you don't want it as an enum class I can change it. But there are a couple of spots that will need a static cast because you or the enums together.
On 2016/05/27 22:32:30, dtapuska wrote: > On 2016/05/27 22:19:15, caseq wrote: > > On 2016/05/27 22:10:53, dtapuska wrote: > > > > > > https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h > > > (right): > > > > > > > > > https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h:56: > > > ShowNothing = 0, > > > On 2016/05/27 21:54:01, pfeldman wrote: > > > > We don't really need nothing as an option. > > > > > > It is mainly for comparison. Because this is an enum class you can't compare > > it > > > against 0. You need to test something of the same type. > > > > Why are you making it a class btw? Can we just go with a plain old int-based > > enum and save on re-implementing bit-wise ops? > > This is the newer pattern in chromium so it is strongly typed. Ie. You'll get a > compilation error this way. But if you don't want it as an enum class I can > change it. But there are a couple of spots that will need a static cast because > you or the enums together. PTAL; I think I've addressed your comments.
https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp (right): https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:84: if (context != mainContext) On 2016/05/27 22:32:12, pfeldman wrote: > We should use access checks instead of context comparison. I removed the context check and moved it up to the ThreadDebugger. Is that what you wanted? https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:117: if (!target) On 2016/05/27 22:32:12, pfeldman wrote: > Why would we fall back to the window? This code was copied from previous impl (see removed code) https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:125: if ((filterMask & EventListenerFilter::ShowDescendants) != EventListenerFilter::ShowNothing) { On 2016/05/27 22:32:12, pfeldman wrote: > Get rid of the ShowNothing, simply if w/ mask test. Done. https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:127: if (LocalDOMWindow* window = target->toLocalDOMWindow()) { On 2016/05/27 22:32:12, pfeldman wrote: > What if target is not window? See else if path. https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:138: addEventListenersForEventTarget(&element, filterMask, mainContext, eventInformation); On 2016/05/27 22:32:12, pfeldman wrote: > Do you intend to cross the frame boundary? I wanted to; I've added this back. It seemed to me that the initial comments on patch set 1 were to remove that functionality. https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp (right): https://codereview.chromium.org/1950403002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:320: InspectorDOMDebuggerAgent::eventListenersInfoForTarget(isolate, info[0], filterMask, listenerInfo); On 2016/05/27 22:32:12, pfeldman wrote: > We want InspectorDOMDebuggerAgent::eventListenersInfoForTarget to report all the > listeners, cross the iframe boundary and not do the security checks. Security > checks should be placed here instead. Can you be more explicit of which security checks? I've added based on creation context; but I expect that is not what you mean.
https://codereview.chromium.org/1950403002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp (right): https://codereview.chromium.org/1950403002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:335: if (info.handler->CreationContext() != context) I think this is the right places for this check to be, but my understanding is that Pavel's suggestion was to use some security-origin based check (e.g. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). Unlike simple checking for context, this should allow access to same-origin subframes, though we also need to assure these will work well for extension-based listeners (which we don't want to expose).
On 2016/05/31 21:36:49, caseq wrote: > https://codereview.chromium.org/1950403002/diff/90001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp (right): > > https://codereview.chromium.org/1950403002/diff/90001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp:335: if > (info.handler->CreationContext() != context) > I think this is the right places for this check to be, but my understanding is > that Pavel's suggestion was to use some security-origin based check (e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > Unlike simple checking for context, this should allow access to same-origin > subframes, though we also need to assure these will work well for > extension-based listeners (which we don't want to expose). Can we review this as is? I've tried to add the BindingSecurity check but it requires populating the EventTarget on the V8EventListenerInfo object which I'm not sure if I am doing correctly. I'd prefer someone with more knowledge of the v8 bindings to do this than I.
>> I'd prefer someone with more knowledge of the v8 bindings to do this than I Not sure I follow - is there someone picking up this change? https://codereview.chromium.org/1950403002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp (right): https://codereview.chromium.org/1950403002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:96: addEventListenersForEventTarget(target, filterMask, eventInformation); I don't think this code works for the case of target->toNode().isFrameOwner(); https://codereview.chromium.org/1950403002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:120: static const char webglErrorFiredEventName[] = "webglErrorFired"; Lets keep constants all together (above methods) https://codereview.chromium.org/1950403002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:126: namespace DOMDebuggerAgentState { This could also be a part of anonymous namespace. https://codereview.chromium.org/1950403002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp:146: addEventListenersForEventTargetAndDescendants(target, filterMask, eventInformation); We already pass filterMask in, why would we branch here? https://codereview.chromium.org/1950403002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right): https://codereview.chromium.org/1950403002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:575: this.target().domdebuggerAgent().getEventListeners(this._objectId, false, mycallback.bind(this)); UI should pass the option here.
pfeldman@chromium.org changed reviewers: - pfeldman@chromium.org |
