Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1051923003:
Add a WebContentsImpl API to snapshot the accessibility tree. (Closed)
|
Created:
5 years, 8 months ago by dmazzoni Modified:
5 years, 8 months ago CC:
aboxhall+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, jam, je_julie(Not used), mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, nektar+watch_chromium.org, ncarter (slow), plundblad+watch_chromium.org, sgurun-gerrit only, site-isolation-reviews_chromium.org, yuzo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a WebContentsImpl API to snapshot the accessibility tree.
Depends on: https://codereview.chromium.org/1053773002/
BUG=472704
Committed: https://crrev.com/83ba5c841ef0bbb4fbf229f6cb342fcc78bea224
Cr-Commit-Position: refs/heads/master@{#325011}
Patch Set 1 #Patch Set 2 : Fixed and finished test #
Total comments: 22
Patch Set 3 : Addressed some of the feedback #
Total comments: 2
Patch Set 4 : Scoped trace, clean up callbacks #
Total comments: 9
Patch Set 5 : Address feedback #
Total comments: 7
Patch Set 6 : Better auto iterator syntax #Patch Set 7 : Rebase #Patch Set 8 : Clean up callbacks before end of OnRenderProcessGone #Messages
Total messages: 31 (10 generated)
dmazzoni@chromium.org changed reviewers: + creis@chromium.org
Charlie, could you look at this? This is another code path that will need OOPIF support, unfortunately - it suggests that the current design isn't quite right. Let's figure out how it could support this use-case too.
[+site-isolation-reviews, nick] Why is a one-time snapshot different from the normal code for getting the AX tree, with respect to OOPIF? In both cases, we'll need to assemble the tree from multiple processes. A few minor nits below (feel free to ignore if this is just a draft CL), but one real question about why we need token tracking on this particular API. I'd like to avoid that if we can. https://codereview.chromium.org/1051923003/diff/20001/content/browser/accessi... File content/browser/accessibility/snapshot_ax_tree_browsertest.cc (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/accessi... content/browser/accessibility/snapshot_ax_tree_browsertest.cc:51: const char url_str[] = "data:text/html,<button>Click</button>"; Just curious, why the char[]? I would normally expect std::string or just inlining this into the next line's GURL constructor, but I'm open to learning reasons to use C style strings. https://codereview.chromium.org/1051923003/diff/20001/content/browser/accessi... content/browser/accessibility/snapshot_ax_tree_browsertest.cc:64: LOG(INFO) << waiter.snapshot().ToString(); LOG(INFO) isn't allowed in new code anymore, is it? Looks like it's very rare now. https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1399: int token, "token" seems like an ambiguous name here. https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1826: static int next_token = 1; This feels like an awkward pattern. We haven't really needed to track callbacks for individual calls to RFH APIs before. Why is it needed here? I don't see any uses of it in this CL, so I don't know alternatives to suggest or why you might need multiple outstanding requests. https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:370: typedef base::Callback<void(const ui::AXTreeUpdate&)> Style nit: typedefs belong at the top. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:689: // RequestAXTreeSnapshot and their corresponding callbacks. Please say what the key is. https://codereview.chromium.org/1051923003/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:727: // frame and everything in the same site instance. How will this work for OOPIFs?
https://codereview.chromium.org/1051923003/diff/20001/content/browser/accessi... File content/browser/accessibility/snapshot_ax_tree_browsertest.cc (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/accessi... content/browser/accessibility/snapshot_ax_tree_browsertest.cc:51: const char url_str[] = "data:text/html,<button>Click</button>"; On 2015/04/02 23:09:06, Charlie Reis wrote: > Just curious, why the char[]? I would normally expect std::string or just > inlining this into the next line's GURL constructor, but I'm open to learning > reasons to use C style strings. No preference - I forked this from another test where it may have made sense to have the url for some reason. I inlined it into the GURL constructor. https://codereview.chromium.org/1051923003/diff/20001/content/browser/accessi... content/browser/accessibility/snapshot_ax_tree_browsertest.cc:64: LOG(INFO) << waiter.snapshot().ToString(); On 2015/04/02 23:09:06, Charlie Reis wrote: > LOG(INFO) isn't allowed in new code anymore, is it? Looks like it's very rare > now. It's banned for browser code, but I don't think it's banned for tests. Here's my reasoning behind leaving it here: if the test passes, you won't see it either way so it doesn't matter. But if the test fails, this debug string will give you a lot of insight into *why* it failed. Making it VLOG(1) could work for debugging a test locally, but that's not helpful if six months from now it starts failing on a builder or try bot. I guess one alternative would be to somehow trigger the log only if the test fails - do you know if there's any way to do that? https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1399: int token, On 2015/04/02 23:09:06, Charlie Reis wrote: > "token" seems like an ambiguous name here. Happy to switch to "id" if you prefer, but deferring fixing this until we resolve the next comment bleow: https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1826: static int next_token = 1; On 2015/04/02 23:09:06, Charlie Reis wrote: > This feels like an awkward pattern. We haven't really needed to track callbacks > for individual calls to RFH APIs before. Why is it needed here? I don't see > any uses of it in this CL, so I don't know alternatives to suggest or why you > might need multiple outstanding requests. FWIW, I copied this code nearly verbatim from ExecuteJavaScript, above. Happy to replace it, but it doesn't seem that awkward to me - and most importantly, I think it's more important that RFHI exposes a simple and pretty foolproof API (make a request, get a callback) so that the slight messiness of waiting for the IPC response and matching it with the callback is localized here rather than leaked. See the bug for more context on how this might be used. The rest of the code that's going to make use of this is still being written and I don't know exactly how it might be used. It seemed the most general possible to implement it this way. The other possibilities would be: 1. Only allow one outstanding request, reject others while one is pending 2. Keep a list of all callbacks while one request is pending With either of those solutions, we'd probably have to implement a timeout too, otherwise if a render process dies then no callbacks after that one would succeed. I'm not sure that's actually any simpler. With the current solution we don't really need a timeout - if a render process dies then one request might never return and we might leak the closure, but subsequent requests would be unaffected. https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:370: typedef base::Callback<void(const ui::AXTreeUpdate&)> On 2015/04/02 23:09:06, Charlie Reis wrote: > Style nit: typedefs belong at the top. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Done. https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:689: // RequestAXTreeSnapshot and their corresponding callbacks. On 2015/04/02 23:09:06, Charlie Reis wrote: > Please say what the key is. Reworded. If this wording looks better, I can modify the same map used for ExecuteJavaScript, above. https://codereview.chromium.org/1051923003/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:727: // frame and everything in the same site instance. On 2015/04/02 23:09:06, Charlie Reis wrote: > How will this work for OOPIFs? Yes - to address this and your question above, here's why it's not so trivial to implement OOPIF support as-is. RIght now the OOPIF accessibility code assumes that you first enable accessibility for the frame, then every time RenderFrameHostImpl::OnAccessibilityEvents is called we update FrameAccessibility with the information about which frame is the child of which host element in its parent frame. In order to do a snapshot, we'd need to send a request to each frame, then take the responses and use that info to combine everything into one frame tree. At a minimum it will mean refactoring out the part of OnAccessibilityEvents that deals with the frame info and putting that somewhere else that we could also call from here. Basically it's making me think that there must be a way to simplify the current code. I've been hung up on the idea that I don't want to put a |process id, routing id| pair directly into an accessibility tree since the data structure is defined in ui/accessibility (outside of content/) and is also used by views/ and chromeos/ - I don't like the idea of sticking a field called "routing id" there, it feels like it's leaking a content/ internal implementation detail out. I still like the idea of a frame accessibility id - I'd have each AX tree on the renderer side make a blocking call to the browser process to get its own frame accessibiltiy id and the id of any child frames it knows about. That way the returned accessibility trees would be trivial to reassemble/compose into a larger tree, it'd make the whole FrameAccessibility class unnecessary. We could use the frame tree node id for this, or we could make up a new id, it doesn't really matter. This is nearly exactly the same issue we have trying to implement the automation API, where an extension process is now the one trying to compose multiple trees into one - but the extension process doesn't have the ability to directly query the frame tree node map. That's why I really lean towards the idea of making each frame's accessibility tree really contain globally-unique ways to identify its child frames, rather than the process_id/routing_id pair we're using now, which requires extra work to interpret.
creis@chromium.org changed reviewers: + phajdan.jr@chromium.org
[+phajdan.jr for test output question; CC jam for sync IPC proposal] https://codereview.chromium.org/1051923003/diff/20001/content/browser/accessi... File content/browser/accessibility/snapshot_ax_tree_browsertest.cc (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/accessi... content/browser/accessibility/snapshot_ax_tree_browsertest.cc:64: LOG(INFO) << waiter.snapshot().ToString(); On 2015/04/03 22:49:41, dmazzoni wrote: > On 2015/04/02 23:09:06, Charlie Reis wrote: > > LOG(INFO) isn't allowed in new code anymore, is it? Looks like it's very rare > > now. > > It's banned for browser code, but I don't think it's banned for tests. Here's my > reasoning behind leaving it here: if the test passes, you won't see it either > way so it doesn't matter. But if the test fails, this debug string will give you > a lot of insight into *why* it failed. Making it VLOG(1) could work for > debugging a test locally, but that's not helpful if six months from now it > starts failing on a builder or try bot. > > I guess one alternative would be to somehow trigger the log only if the test > fails - do you know if there's any way to do that? You can send it into a particular ASSERT_EQ statement with <<: https://code.google.com/p/googletest/wiki/Primer#Assertions That said, Paweł would know better than I would what the best practice for this is. https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1399: int token, On 2015/04/03 22:49:41, dmazzoni wrote: > On 2015/04/02 23:09:06, Charlie Reis wrote: > > "token" seems like an ambiguous name here. > > Happy to switch to "id" if you prefer, but deferring fixing this until we > resolve the next comment bleow: "id" isn't any less ambiguous, but at least it's consistent with OnJavaScriptExecuteResponse. I guess we can do that until a general approach is added. https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1826: static int next_token = 1; On 2015/04/03 22:49:41, dmazzoni wrote: > On 2015/04/02 23:09:06, Charlie Reis wrote: > > This feels like an awkward pattern. We haven't really needed to track > callbacks > > for individual calls to RFH APIs before. Why is it needed here? I don't see > > any uses of it in this CL, so I don't know alternatives to suggest or why you > > might need multiple outstanding requests. > > FWIW, I copied this code nearly verbatim from ExecuteJavaScript, above. Happy to > replace it, but it doesn't seem that awkward to me - and most importantly, I > think it's more important that RFHI exposes a simple and pretty foolproof API > (make a request, get a callback) so that the slight messiness of waiting for the > IPC response and matching it with the callback is localized here rather than > leaked. Oh, I'd never seen that. Ok, I'll defer, since that seems to go back quite a way. (The pattern doesn't seem scalable to me-- RFHI shouldn't have to track state for every one of its request/response APIs, especially if that state needs maintenance for things like the process dying. Maybe some day that can be made generalizable, but this will do for the moment.) > See the bug for more context on how this might be used. The rest of the code > that's going to make use of this is still being written and I don't know exactly > how it might be used. It seemed the most general possible to implement it this > way. > > The other possibilities would be: > 1. Only allow one outstanding request, reject others while one is pending > 2. Keep a list of all callbacks while one request is pending > > With either of those solutions, we'd probably have to implement a timeout too, > otherwise if a render process dies then no callbacks after that one would > succeed. I'm not sure that's actually any simpler. With the current solution we > don't really need a timeout - if a render process dies then one request might > never return and we might leak the closure, but subsequent requests would be > unaffected. If the renderer process dies, we should be able to clear all the current callbacks, right? We won't hear any response for them after that. (If you agree, Avi and I can do a similar thing for ExecuteJavaScript.) https://codereview.chromium.org/1051923003/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:727: // frame and everything in the same site instance. On 2015/04/03 22:49:42, dmazzoni wrote: > On 2015/04/02 23:09:06, Charlie Reis wrote: > > How will this work for OOPIFs? > > Yes - to address this and your question above, here's why it's not so trivial to > implement OOPIF support as-is. RIght now the OOPIF accessibility code assumes > that you first enable accessibility for the frame, then every time > RenderFrameHostImpl::OnAccessibilityEvents is called we update > FrameAccessibility with the information about which frame is the child of which > host element in its parent frame. > > In order to do a snapshot, we'd need to send a request to each frame, then take > the responses and use that info to combine everything into one frame tree. At a > minimum it will mean refactoring out the part of OnAccessibilityEvents that > deals with the frame info and putting that somewhere else that we could also > call from here. > > Basically it's making me think that there must be a way to simplify the current > code. > > I've been hung up on the idea that I don't want to put a |process id, routing > id| pair directly into an accessibility tree since the data structure is defined > in ui/accessibility (outside of content/) and is also used by views/ and > chromeos/ - I don't like the idea of sticking a field called "routing id" there, > it feels like it's leaking a content/ internal implementation detail out. > > I still like the idea of a frame accessibility id - I'd have each AX tree on the > renderer side make a blocking call to the browser process to get its own frame > accessibiltiy id and the id of any child frames it knows about. That way the > returned accessibility trees would be trivial to reassemble/compose into a > larger tree, it'd make the whole FrameAccessibility class unnecessary. > > We could use the frame tree node id for this, or we could make up a new id, it > doesn't really matter. > > This is nearly exactly the same issue we have trying to implement the automation > API, where an extension process is now the one trying to compose multiple trees > into one - but the extension process doesn't have the ability to directly query > the frame tree node map. That's why I really lean towards the idea of making > each frame's accessibility tree really contain globally-unique ways to identify > its child frames, rather than the process_id/routing_id pair we're using now, > which requires extra work to interpret. I'm open to the idea of using a different set of IDs for your case, but there's still two requirements: 1) We need to be able to verify that renderer isn't lying about which accessibility trees it can access. It's a security bug if evil.com's renderer process can generate fake content for a chrome:// page, for example. 2) We shouldn't be using synchronous IPCs for it. Sync IPCs have caused a lot of headaches in the past, and on top of that they're limited to the IO thread. Most of the accessibility stuff is on the UI thread, right? (Feel free to chat with jam@ for more reasons not to use sync IPCs.) https://codereview.chromium.org/1051923003/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1051923003/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:727: // frame and everything in the same site instance. We should have a bug number for supporting this with OOPIFs.
If the verbose output is only intended for when the test fails, why not use SCOPED_TRACE (https://code.google.com/p/googletest/wiki/V1_6_AdvancedGuide#Adding_Traces_to...
> If the renderer process dies, we should be able to clear all the current > callbacks, right? We won't hear any response for them after that. > > (If you agree, Avi and I can do a similar thing for ExecuteJavaScript.) Agreed. I added some code to OnRenderProcessGone to clean up ax_tree_snapshot_callbacks_ and it makes sense to do something similar for ExecuteJavaScript callbacks, but we'll have to check carefully what the return value should be - if we should call the callback with a failure code or just delete the callback. https://codereview.chromium.org/1051923003/diff/20001/content/browser/accessi... File content/browser/accessibility/snapshot_ax_tree_browsertest.cc (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/accessi... content/browser/accessibility/snapshot_ax_tree_browsertest.cc:64: LOG(INFO) << waiter.snapshot().ToString(); On 2015/04/06 21:56:50, Charlie Reis (slow until 4-15) wrote: > On 2015/04/03 22:49:41, dmazzoni wrote: > > On 2015/04/02 23:09:06, Charlie Reis wrote: > > > LOG(INFO) isn't allowed in new code anymore, is it? Looks like it's very > rare > > > now. > > > > It's banned for browser code, but I don't think it's banned for tests. Here's > my > > reasoning behind leaving it here: if the test passes, you won't see it either > > way so it doesn't matter. But if the test fails, this debug string will give > you > > a lot of insight into *why* it failed. Making it VLOG(1) could work for > > debugging a test locally, but that's not helpful if six months from now it > > starts failing on a builder or try bot. > > > > I guess one alternative would be to somehow trigger the log only if the test > > fails - do you know if there's any way to do that? > > You can send it into a particular ASSERT_EQ statement with <<: > https://code.google.com/p/googletest/wiki/Primer#Assertions > > That said, Paweł would know better than I would what the best practice for this > is. On 2015/04/07 12:59:04, Paweł Hajdan Jr. wrote: > If the verbose output is only intended for when the test fails, why not use > SCOPED_TRACE > (https://code.google.com/p/googletest/wiki/V1_6_AdvancedGuide#Adding_Traces_to... Great, idea, done. https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1399: int token, On 2015/04/06 21:56:51, Charlie Reis (slow until 4-15) wrote: > On 2015/04/03 22:49:41, dmazzoni wrote: > > On 2015/04/02 23:09:06, Charlie Reis wrote: > > > "token" seems like an ambiguous name here. > > > > Happy to switch to "id" if you prefer, but deferring fixing this until we > > resolve the next comment bleow: > > "id" isn't any less ambiguous, but at least it's consistent with > OnJavaScriptExecuteResponse. I guess we can do that until a general approach is > added. Done. https://codereview.chromium.org/1051923003/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:727: // frame and everything in the same site instance. On 2015/04/06 21:56:51, Charlie Reis (slow until 4-15) wrote: > On 2015/04/03 22:49:42, dmazzoni wrote: > > On 2015/04/02 23:09:06, Charlie Reis wrote: > > > How will this work for OOPIFs? > > > > Yes - to address this and your question above, here's why it's not so trivial > to > > implement OOPIF support as-is. RIght now the OOPIF accessibility code assumes > > that you first enable accessibility for the frame, then every time > > RenderFrameHostImpl::OnAccessibilityEvents is called we update > > FrameAccessibility with the information about which frame is the child of > which > > host element in its parent frame. > > > > In order to do a snapshot, we'd need to send a request to each frame, then > take > > the responses and use that info to combine everything into one frame tree. At > a > > minimum it will mean refactoring out the part of OnAccessibilityEvents that > > deals with the frame info and putting that somewhere else that we could also > > call from here. > > > > Basically it's making me think that there must be a way to simplify the > current > > code. > > > > I've been hung up on the idea that I don't want to put a |process id, routing > > id| pair directly into an accessibility tree since the data structure is > defined > > in ui/accessibility (outside of content/) and is also used by views/ and > > chromeos/ - I don't like the idea of sticking a field called "routing id" > there, > > it feels like it's leaking a content/ internal implementation detail out. > > > > I still like the idea of a frame accessibility id - I'd have each AX tree on > the > > renderer side make a blocking call to the browser process to get its own frame > > accessibiltiy id and the id of any child frames it knows about. That way the > > returned accessibility trees would be trivial to reassemble/compose into a > > larger tree, it'd make the whole FrameAccessibility class unnecessary. > > > > We could use the frame tree node id for this, or we could make up a new id, it > > doesn't really matter. > > > > This is nearly exactly the same issue we have trying to implement the > automation > > API, where an extension process is now the one trying to compose multiple > trees > > into one - but the extension process doesn't have the ability to directly > query > > the frame tree node map. That's why I really lean towards the idea of making > > each frame's accessibility tree really contain globally-unique ways to > identify > > its child frames, rather than the process_id/routing_id pair we're using now, > > which requires extra work to interpret. > > I'm open to the idea of using a different set of IDs for your case, but there's > still two requirements: > > 1) We need to be able to verify that renderer isn't lying about which > accessibility trees it can access. It's a security bug if evil.com's renderer > process can generate fake content for a chrome:// page, for example. > > 2) We shouldn't be using synchronous IPCs for it. Sync IPCs have caused a lot > of headaches in the past, and on top of that they're limited to the IO thread. > Most of the accessibility stuff is on the UI thread, right? (Feel free to chat > with jam@ for more reasons not to use sync IPCs.) Note that this is a sync IPC from the renderer to the browser, which is how the renderer currently gets a routing ID for a new frame. I'm not sure how this is any different. Actually it'd probably work fine to use async IPCs. We can just write the logic so that accessibility messages aren't sent while we're waiting on one of those IPCs to get frame IDs. Anyway, let's defer this to a follow-up changelist. I'm hoping it will be possible to land this change in the meantime. https://codereview.chromium.org/1051923003/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1051923003/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:727: // frame and everything in the same site instance. On 2015/04/06 21:56:51, Charlie Reis (slow until 4-15) wrote: > We should have a bug number for supporting this with OOPIFs. Done.
LGTM with nits. Note: in my last render_frame_impl.h comment, s/id/callback_id/. The code review tool is having issues and won't let me go back and edit that comment at the moment. Hopefully it publishes these without a problem. https://codereview.chromium.org/1051923003/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1051923003/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:727: // frame and everything in the same site instance. On 2015/04/09 18:39:01, dmazzoni wrote: > On 2015/04/06 21:56:51, Charlie Reis (slow until 4-15) wrote: > > On 2015/04/03 22:49:42, dmazzoni wrote: > > > On 2015/04/02 23:09:06, Charlie Reis wrote: > > > > How will this work for OOPIFs? > > > > > > Yes - to address this and your question above, here's why it's not so > trivial > > to > > > implement OOPIF support as-is. RIght now the OOPIF accessibility code > assumes > > > that you first enable accessibility for the frame, then every time > > > RenderFrameHostImpl::OnAccessibilityEvents is called we update > > > FrameAccessibility with the information about which frame is the child of > > which > > > host element in its parent frame. > > > > > > In order to do a snapshot, we'd need to send a request to each frame, then > > take > > > the responses and use that info to combine everything into one frame tree. > At > > a > > > minimum it will mean refactoring out the part of OnAccessibilityEvents that > > > deals with the frame info and putting that somewhere else that we could also > > > call from here. > > > > > > Basically it's making me think that there must be a way to simplify the > > current > > > code. > > > > > > I've been hung up on the idea that I don't want to put a |process id, > routing > > > id| pair directly into an accessibility tree since the data structure is > > defined > > > in ui/accessibility (outside of content/) and is also used by views/ and > > > chromeos/ - I don't like the idea of sticking a field called "routing id" > > there, > > > it feels like it's leaking a content/ internal implementation detail out. > > > > > > I still like the idea of a frame accessibility id - I'd have each AX tree on > > the > > > renderer side make a blocking call to the browser process to get its own > frame > > > accessibiltiy id and the id of any child frames it knows about. That way the > > > returned accessibility trees would be trivial to reassemble/compose into a > > > larger tree, it'd make the whole FrameAccessibility class unnecessary. > > > > > > We could use the frame tree node id for this, or we could make up a new id, > it > > > doesn't really matter. > > > > > > This is nearly exactly the same issue we have trying to implement the > > automation > > > API, where an extension process is now the one trying to compose multiple > > trees > > > into one - but the extension process doesn't have the ability to directly > > query > > > the frame tree node map. That's why I really lean towards the idea of making > > > each frame's accessibility tree really contain globally-unique ways to > > identify > > > its child frames, rather than the process_id/routing_id pair we're using > now, > > > which requires extra work to interpret. > > > > I'm open to the idea of using a different set of IDs for your case, but > there's > > still two requirements: > > > > 1) We need to be able to verify that renderer isn't lying about which > > accessibility trees it can access. It's a security bug if evil.com's renderer > > process can generate fake content for a chrome:// page, for example. > > > > 2) We shouldn't be using synchronous IPCs for it. Sync IPCs have caused a lot > > of headaches in the past, and on top of that they're limited to the IO thread. > > > Most of the accessibility stuff is on the UI thread, right? (Feel free to > chat > > with jam@ for more reasons not to use sync IPCs.) > > Note that this is a sync IPC from the renderer to the browser, Which IPC are you referring to by "this is a sync IPC?" > which is how the > renderer currently gets a routing ID for a new frame. I'm not sure how this is > any different. > > Actually it'd probably work fine to use async IPCs. We can just write the logic > so that accessibility messages aren't sent while we're waiting on one of those > IPCs to get frame IDs. > > Anyway, let's defer this to a follow-up changelist. I'm hoping it will be > possible to land this change in the meantime. Agreed. https://codereview.chromium.org/1051923003/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1051923003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1090: iter->second.Run(ui::AXTreeUpdate()); Sanity check: we've seen security bugs in the past where we've run callbacks after the objects they store internally are already deleted. Are we at risk of that by running this after sending out the RendererExited/Terminated notifications? https://codereview.chromium.org/1051923003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1408: int request_id, Actually, let's call this callback_id (throughout the CL). request_id is too easy to confuse with the tracking of network requests in the loader, which isn't related to this. https://codereview.chromium.org/1051923003/diff/60001/content/renderer/access... File content/renderer/accessibility/blink_ax_tree_source.h (right): https://codereview.chromium.org/1051923003/diff/60001/content/renderer/access... content/renderer/accessibility/blink_ax_tree_source.h:22: void SetRoot(blink::WebAXObject root); Maybe add a comment saying why it's useful to override the default GetRoot() behavior? (I'm not sure I know why it's needed.) https://codereview.chromium.org/1051923003/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1051923003/diff/60001/content/renderer/render... content/renderer/render_frame_impl.h:646: void OnSnapshotAccessibilityTree(int token); Let's be consistent and call this id.
dmazzoni@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for OWNERS review of new IPCs in content/common/*messages* https://codereview.chromium.org/1051923003/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1051923003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1090: iter->second.Run(ui::AXTreeUpdate()); On 2015/04/10 20:18:07, Charlie Reis (slow until 4-15) wrote: > Sanity check: we've seen security bugs in the past where we've run callbacks > after the objects they store internally are already deleted. Are we at risk of > that by running this after sending out the RendererExited/Terminated > notifications? Not unless the caller binds the callback to an Unretained pointer or something dangerous like that. Here's the next changelist that makes use of this callback and it looks totally safe, the callback is bound to a scoped java ref. https://codereview.chromium.org/1073983005 https://codereview.chromium.org/1051923003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1408: int request_id, On 2015/04/10 20:18:07, Charlie Reis (slow until 4-15) wrote: > Actually, let's call this callback_id (throughout the CL). request_id is too > easy to confuse with the tracking of network requests in the loader, which isn't > related to this. Done. https://codereview.chromium.org/1051923003/diff/60001/content/renderer/access... File content/renderer/accessibility/blink_ax_tree_source.h (right): https://codereview.chromium.org/1051923003/diff/60001/content/renderer/access... content/renderer/accessibility/blink_ax_tree_source.h:22: void SetRoot(blink::WebAXObject root); On 2015/04/10 20:18:07, Charlie Reis (slow until 4-15) wrote: > Maybe add a comment saying why it's useful to override the default GetRoot() > behavior? (I'm not sure I know why it's needed.) Done. https://codereview.chromium.org/1051923003/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1051923003/diff/60001/content/renderer/render... content/renderer/render_frame_impl.h:646: void OnSnapshotAccessibilityTree(int token); On 2015/04/10 20:18:07, Charlie Reis (slow until 4-15) wrote: > Let's be consistent and call this id. Done.
Thanks, still LGTM. https://codereview.chromium.org/1051923003/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1051923003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1090: iter->second.Run(ui::AXTreeUpdate()); On 2015/04/10 21:57:12, dmazzoni wrote: > On 2015/04/10 20:18:07, Charlie Reis (slow until 4-15) wrote: > > Sanity check: we've seen security bugs in the past where we've run callbacks > > after the objects they store internally are already deleted. Are we at risk > of > > that by running this after sending out the RendererExited/Terminated > > notifications? > > Not unless the caller binds the callback to an Unretained pointer or something > dangerous like that. Here's the next changelist that makes use of this callback > and it looks totally safe, the callback is bound to a scoped java ref. > > https://codereview.chromium.org/1073983005 I'll have to take your word for it; ScopedJavaGlobalRef is foreign to me. I was more concerned about things like AXTreeSnapshotCallback in that CL having a pointer to something that gets deleted before here, or being stored on an object that gets deleted before here. Sounds like it's not an immediate concern.
https://codereview.chromium.org/1051923003/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1051923003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1087: for (auto iter = ax_tree_snapshot_callbacks_.begin(); for (const auto& iter : ax_tree_snapshot_callbacks_) https://codereview.chromium.org/1051923003/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1051923003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:695: std::map<int, AXTreeSnapshotCallback> ax_tree_snapshot_callbacks_; Would it make sense to use IdMap here? https://codereview.chromium.org/1051923003/diff/80001/content/renderer/access... File content/renderer/accessibility/renderer_accessibility.cc (right): https://codereview.chromium.org/1051923003/diff/80001/content/renderer/access... content/renderer/accessibility/renderer_accessibility.cc:38: if (!render_frame || !render_frame->GetWebFrame()) Why would render_frame be null?
https://codereview.chromium.org/1051923003/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1051923003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1087: for (auto iter = ax_tree_snapshot_callbacks_.begin(); On 2015/04/10 22:49:00, dcheng wrote: > for (const auto& iter : ax_tree_snapshot_callbacks_) Done. https://codereview.chromium.org/1051923003/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1051923003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:695: std::map<int, AXTreeSnapshotCallback> ax_tree_snapshot_callbacks_; On 2015/04/10 22:49:00, dcheng wrote: > Would it make sense to use IdMap here? Thanks for making me aware of that, looks like an useful helper class. In this particular case I can't see any of the features of IdMap (thread checks, multiple simultaneous iterator support, etc.) being useful here, so my preference is just to stick with std::map unless you have a strong preference. If you do want to use IdMap, we could do the same for the similar map for ExecuteScript in the same file, in a follow-up change. https://codereview.chromium.org/1051923003/diff/80001/content/renderer/access... File content/renderer/accessibility/renderer_accessibility.cc (right): https://codereview.chromium.org/1051923003/diff/80001/content/renderer/access... content/renderer/accessibility/renderer_accessibility.cc:38: if (!render_frame || !render_frame->GetWebFrame()) On 2015/04/10 22:49:00, dcheng wrote: > Why would render_frame be null? Shouldn't happen. Changed to DCHECK(render_frame).
ipc changes lgtm https://codereview.chromium.org/1051923003/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1051923003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:695: std::map<int, AXTreeSnapshotCallback> ax_tree_snapshot_callbacks_; On 2015/04/13 at 06:37:18, dmazzoni wrote: > On 2015/04/10 22:49:00, dcheng wrote: > > Would it make sense to use IdMap here? > > Thanks for making me aware of that, looks like an useful helper class. In this particular case I can't see any of the features of IdMap (thread checks, multiple simultaneous iterator support, etc.) being useful here, so my preference is just to stick with std::map unless you have a strong preference. > > If you do want to use IdMap, we could do the same for the similar map for ExecuteScript in the same file, in a follow-up change. If there's already precedent for that in this file, then it's fine.
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1051923003/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1051923003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sgurun@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1051923003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The try failures were caused by cleaning up the callbacks too late in RenderFrameHostImpl::OnRenderProcessGone - that function actually results in deleting |this| sometimes, so we need to do cleanup slightly earlier. @sgurun, you were looking over this code with me, could you take a quick look?
lgtm
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1051923003/#ps140001 (title: "Clean up callbacks before end of OnRenderProcessGone")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1051923003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/83ba5c841ef0bbb4fbf229f6cb342fcc78bea224 Cr-Commit-Position: refs/heads/master@{#325011} |
