Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2208583005:
Request to start/stop calculating composition info from RenderWidget when it is active/inactive. (Closed)
|
Created:
4 years, 4 months ago by EhsanK Modified:
4 years, 4 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRequest to start/stop calculating composition info from RenderWidget when it is active/inactive.
This CL sends the request to start/stop updating composition info to the
widget which became active/inactive, rather than sending it only to the
top level's render widget.
This CL also revises the test TrackCompositionRangeForAllFrames so that
it addresses OOPIFs.
BUG=578168, 602723, 624714
Committed: https://crrev.com/56a714e089de70102b0308f82bfe9b181b891022
Cr-Commit-Position: refs/heads/master@{#411278}
Patch Set 1 #Patch Set 2 : Fixing a Crash due to Pure Virtual Call #
Total comments: 7
Patch Set 3 : Addressing creis@ comments #
Total comments: 7
Patch Set 4 : Addressing kenrb@'s Comments #Patch Set 5 : Rebased (nona@'s patch relanded) #
Messages
Total messages: 38 (22 generated)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ekaramad@chromium.org changed reviewers: + creis@chromium.org, kenrb@chromium.org, nasko@chromium.org, nona@chromium.org
PTAL. Adding nona@ who implemented CL 2029423003. https://codereview.chromium.org/2208583005/diff/20001/content/public/test/tex... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/2208583005/diff/20001/content/public/test/tex... content/public/test/text_input_test_utils.h:56: bool RequestCompositionInfoFromActiveWidget(WebContents* web_contents); This is a direct request to send composition info, which I think serves the purpose of the test to verify that we do track state from all frames.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A few minor thoughts below, but otherwise I'm happy to approve once Ken does. https://codereview.chromium.org/2208583005/diff/20001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2208583005/diff/20001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:230: LOG(INFO) << "View: " << tester()->GetUpdatedView(); Did you mean to leave these in? https://codereview.chromium.org/2208583005/diff/20001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:643: LOG(INFO) << "Wait for view is over: " << view; Should probably remove this as well. https://codereview.chromium.org/2208583005/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/2208583005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.h:691: RenderWidgetHostImpl* last_active_widget_; What guarantees we won't get a use-after-free here? (Maybe it would be safer to store the routing ID and look it up later?)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Charlie! PTAL. https://codereview.chromium.org/2208583005/diff/20001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2208583005/diff/20001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:230: LOG(INFO) << "View: " << tester()->GetUpdatedView(); On 2016/08/03 22:16:23, Charlie Reis (OOO til 8-8) wrote: > Did you mean to leave these in? No sorry. Removed. Thanks! https://codereview.chromium.org/2208583005/diff/20001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:643: LOG(INFO) << "Wait for view is over: " << view; On 2016/08/03 22:16:23, Charlie Reis (OOO til 8-8) wrote: > Should probably remove this as well. Done. https://codereview.chromium.org/2208583005/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/2208583005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.h:691: RenderWidgetHostImpl* last_active_widget_; On 2016/08/03 22:16:23, Charlie Reis (OOO til 8-8) wrote: > What guarantees we won't get a use-after-free here? (Maybe it would be safer to > store the routing ID and look it up later?) Actually, there was a way to crash this. Good catch! Thanks! I changed the code to use routing/process id instead. The crash happens because we detach from TextInputManager in the dtor of RenderWidgetHostViewBase. A child frame deletes itself in a post task. So we could lose the widget and then get to RWHVAura::OnUpdateTextInputStateCalled() when we hit ~RWHVBase() which is a UaF. Crash happens if we suddenly remove RWH when we have an <input> which is focused (i.e., detach a frame with javascript code while the <input> inside the frame is still focused). But before all this, if we could safely call RWHVBase::GetRenderWidgetHost(), I could simply use |updated_view| in RWHVAura::OnUpdateTextInputStateCalled(). Unfortunately, when destroying active view we would get a crash due to pure virtual call.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
One other small issue below, but otherwise still happy to stamp when Ken approves. https://codereview.chromium.org/2208583005/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2208583005/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:260: } nit: No change needed here. https://codereview.chromium.org/2208583005/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2208583005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:3022: last_active_widget_process_id_ = MSG_ROUTING_NONE; You need to clear the routing ID as well, especially since that's what you're checking for above. Plus process_id_ should be -1, not MSG_ROUTING_NONE (to be consistent with the value from the constructor). Actually, maybe we should be using ChildProcessHost::kInvalidUniqueID for the process IDs, though it's not a big deal.
The issue which caused the regression on collecting composition info is reverted (2029423003).
On 2016/08/08 17:12:48, EhsanK wrote: > The issue which caused the regression on collecting composition info is reverted > (2029423003). Can you elaborate? I'm not sure what you mean.
On 2016/08/08 17:45:36, Charlie Reis wrote: > On 2016/08/08 17:12:48, EhsanK wrote: > > The issue which caused the regression on collecting composition info is > reverted > > (2029423003). > > Can you elaborate? I'm not sure what you mean. Sorry, I meant this original CL: https://codereview.chromium.org/2121953002/ regressed composition range tracking for child frames because it was only enabling and disabling the computation on the renderer side for the tab's RenderWidget only. Since our tests for this were not covering OOPIF we did not see them fail. This CL here was supposed to fix the regression and also enable the test. Now I will have to only enable the tests but since they last patch relies on the IPC introduced in https://codereview.chromium.org/2121953002/ I will have to either wait for https://codereview.chromium.org/2121953002/ to reland, or modify the testing method (I think the latter makes more sense).
lgtm with a nit https://codereview.chromium.org/2208583005/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2208583005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:3016: last_active_widget_process_id_, last_active_widget_routing_id_); It feels weird to have this RenderWidgetHostView sending an IPC through a different RenderWidgetHost (i.e. not the RWH for which it is the view), but it might not be worth trying to do it another way. It doesn't feel important enough to route it back to TextInputStateManager. https://codereview.chromium.org/2208583005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:3018: return; nit: It might be better to have if (last_active_widget) { // [Send IPC, etc] } It's a bit more future-friendly, in case somebody tries to add code other code to the end of this method and doesn't notice the early return here.
Thank you Ken for the reviews! I have addressed the issues, but, the original CL which caused this regression is now reverted (see: https://codereview.chromium.org/2121953002/). So there is not sense in trying to land this now I suppose. I suggest waiting a week and keeping this CL alive to see if the other CL lands. Otherwise, I will have to submit a new CL to find a way to make the composition range tracking test work with oopif, so that we can avoid regressions like this one. Thanks! https://codereview.chromium.org/2208583005/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2208583005/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:260: } On 2016/08/08 17:12:01, Charlie Reis wrote: > nit: No change needed here. Acknowledged. https://codereview.chromium.org/2208583005/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2208583005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:3016: last_active_widget_process_id_, last_active_widget_routing_id_); On 2016/08/08 19:15:50, kenrb wrote: > It feels weird to have this RenderWidgetHostView sending an IPC through a > different RenderWidgetHost (i.e. not the RWH for which it is the view), but it > might not be worth trying to do it another way. It doesn't feel important enough > to route it back to TextInputStateManager. How about adding a method to RWH? That will be more compatible with the way TextInputManager is used to use a widget and call the method which send the IPC. https://codereview.chromium.org/2208583005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:3018: return; On 2016/08/08 19:15:50, kenrb wrote: > nit: It might be better to have > if (last_active_widget) { > // [Send IPC, etc] > } > > It's a bit more future-friendly, in case somebody tries to add code other code > to the end of this method and doesn't notice the early return here. Acknowledged.
On 2016/08/09 16:42:29, EhsanK wrote: > Thank you Ken for the reviews! > > I have addressed the issues, but, the original CL which caused this regression > is now reverted (see: https://codereview.chromium.org/2121953002/). So there is > not sense in trying to land this now I suppose. > > I suggest waiting a week and keeping this CL alive to see if the other CL lands. > Otherwise, I will have to submit a new CL to find a way to make the composition > range tracking test work with oopif, so that we can avoid regressions like this > one. > > Thanks! Sounds good. content/ LGTM in case you end up needing to land this later while I'm away.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for reviews! FYI, nona@'s patch has landed so I have rebased and can try to CQ after dry-run.
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2208583005/#ps80001 (title: "Rebased (nona@'s patch relanded)")
The CQ bit was unchecked by ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ekaramad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Request to start/stop calculating composition info from RenderWidget when it is active/inactive. This CL sends the request to start/stop updating composition info to the widget which became active/inactive, rather than sending it only to the top level's render widget. This CL also revises the test TrackCompositionRangeForAllFrames so that it addresses OOPIFs. BUG=578168,602723,624714 ========== to ========== Request to start/stop calculating composition info from RenderWidget when it is active/inactive. This CL sends the request to start/stop updating composition info to the widget which became active/inactive, rather than sending it only to the top level's render widget. This CL also revises the test TrackCompositionRangeForAllFrames so that it addresses OOPIFs. BUG=578168,602723,624714 Committed: https://crrev.com/56a714e089de70102b0308f82bfe9b181b891022 Cr-Commit-Position: refs/heads/master@{#411278} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/56a714e089de70102b0308f82bfe9b181b891022 Cr-Commit-Position: refs/heads/master@{#411278} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
