Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2054163003:
Fix SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame on CFI Bots. (Closed)
|
Created:
4 years, 6 months ago by EhsanK Modified:
4 years, 5 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, nona+watch_chromium.org, darin-cc_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. |
DescriptionFix SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame on CFI Bots.
Currently, the test SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame
uses stale pointers of the views agains a map to verify that TextInputManager does not
track the view anymore. This is causing issues in Linux CFI bots.
This CL changes the test so that we now check for the number of views being tracked and
verify that after crashing a frame, the number drops by one.
BUG=619147
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/936536dbbfd6cb9cf468c80aba316cabafb0f002
Cr-Commit-Position: refs/heads/master@{#402736}
Patch Set 1 #Patch Set 2 : Fixed Compile Error on Comparing int and unsigned int #Patch Set 3 : Do not assume we only have 2 views registered in the begining. #
Total comments: 8
Patch Set 4 : Addressing creis@ comments #Patch Set 5 : Rebased + Remove internal observers from TextInputManager's observer list #
Total comments: 11
Patch Set 6 : Addressing creis@'s comments #Patch Set 7 : Removed the added code to RenderWidgetHostViewChildFrame::Destroy(). #
Total comments: 6
Patch Set 8 : Addressing creis@'s comments #
Total comments: 2
Patch Set 9 : Addressing sky@'s comments #
Messages
Total messages: 42 (12 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/patch-status/2054163003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ekaramad@chromium.org changed reviewers: + creis@chromium.org, krasin@chromium.org, sky@chromium.org
PTAL. FYI: I am trying to run this patch with 'is_cfi=true' on linux.
https://codereview.chromium.org/2054163003/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2054163003/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:326: size_t registered_views_count = I was expecting this number to be 2U as it was in most bots. But on Linux for some reason we receive a TextInputStateChanged from perhaps the tab's RWHV (so the number is 3). It is unrelated to this test and browser side tracking altogether.
I've added a CFI trybot, let's wait and see. Btw, did you have any troubles with building it locally with is_cfi=true?
On 2016/06/11 20:54:48, krasin wrote: > I've added a CFI trybot, let's wait and see. > > Btw, did you have any troubles with building it locally with is_cfi=true? On windows, build\download_gold_plugin.py failed at update (throwing an error that clang is already up to date). On Linux (ubiquity instance) it failed at linking with a tmalloc error. I assume this has more to do with statically linking than CFI. I never build static so I had not seen this before.
On 2016/06/11 22:58:34, ehsaaan wrote: > On 2016/06/11 20:54:48, krasin wrote: > > I've added a CFI trybot, let's wait and see. > > > > Btw, did you have any troubles with building it locally with is_cfi=true? > > On windows, build\download_gold_plugin.py failed at update (throwing an error > that clang is already up to date). > On Linux (ubiquity instance) it failed at linking with a tmalloc error. I assume > this has more to do with statically linking than CFI. I never build static so I > had not seen this before. Ah, yes. CFI is a Linux-only thing at the moment. Sorry. And Ubiquity does not have enough RAM. It needs at least 64 GB RAM on the machine to build Chrome with CFI. This is because CFI relies on LTO (Link Time Optimization), and that's super memory hungry.
That said, we have a try bot. :) Also, I had successfully built Chrome with CFI on Google Compute Engine, which has machines with up to 240 GB RAM. All are workarounds, though. Next time, I will keep in mind that not everyone has the primary desktop running Linux.
On 2016/06/11 23:06:50, krasin wrote: > That said, we have a try bot. :) > > Also, I had successfully built Chrome with CFI on Google Compute Engine, which > has machines with up to 240 GB RAM. > > All are workarounds, though. Next time, I will keep in mind that not everyone > has the primary desktop running Linux. That makes sense then. Actually I verified that my instance has only 12 GB of RAM which makes it perhaps close to impossible to link normal chrome statically. Btw, thanks for adding the bot.
The current trybot run will be red, as there's another failure introduced by https://codereview.chromium.org/2015463004/ That failure broke Bluetooth stuff in browser_tests and content_unittests, so we'll be able to understand if the current fix is accepted by the bot.
Unfortunately, it's not. :( https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... Let's postpone this until Monday. Please, have a good weekend, and thank you for working on the fix.
On 2016/06/11 23:13:56, krasin wrote: > Unfortunately, it's not. :( > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... > > Let's postpone this until Monday. Please, have a good weekend, and thank you for > working on the fix. This is strange. I could not get anything our of the logs. > Let's postpone this until Monday. Please, have a good weekend, and thank you for > working on the fix. No problem. Sounds good.
On 2016/06/12 00:06:08, ehsaaan wrote: > On 2016/06/11 23:13:56, krasin wrote: > > Unfortunately, it's not. :( > > > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... > > > > Let's postpone this until Monday. Please, have a good weekend, and thank you > for > > working on the fix. > > This is strange. I could not get anything our of the logs. > It's because the test is running in multi-process mode, and while CFI prints something, the error message does not show up. That's why I used --single_process while debugging locally: gdb --args ./out/gn-cfi/interactive_ui_tests --gtest_filter=SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame --no-sandbox --single_process use_cfi_diag also adds more diagnostics, see https://bugs.chromium.org/p/chromium/issues/detail?id=619147#c1 and https://www.chromium.org/developers/testing/control-flow-integrity
[+site-isolation-reviews] Thanks. Just a few comments while we figure out how to get the CFI bots to pass. https://codereview.chromium.org/2054163003/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2054163003/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:327: content::GetAllViewsRegisteredWithTextInputManager(active_contents()) We should just expose the count if we don't need to look at the views themselves (GetRegisteredViewCountFromTextInputManager). https://codereview.chromium.org/2054163003/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/2054163003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:86: GetAllViewsRegisteredWithTextInputManager(WebContents* web_contents); I'm not thrilled with having lots of raw test methods declared as friends. I'd requested avoiding the first one as well, but I think that request fell between the cracks: https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... I'd much rather have public *ForTesting methods on this class, which makes it clearer what's exposed and why. Can we update both of these? https://codereview.chromium.org/2054163003/diff/40001/content/public/test/tex... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/2054163003/diff/40001/content/public/test/tex... content/public/test/text_input_test_utils.h:36: // This method returns the set of all the views which are currently being nit: all views nit: currently registered Also, let's make this return the count rather than the set, since the test doesn't need more than that. We can then call TextInputManager::GetRegisteredViewCountForTesting() in the body.
Description was changed from ========== Fix SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame on CFI Bots. Currently, the test SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame uses stale pointers of the views agains a map to verify that TextInputManager does not track the view anymore. This is causing issues in Linux CFI bots. This CL changes the test so that we now check for the number of views being tracked and verify that after crashing a frame, the number drops by one. BUG=619147 ========== to ========== Fix SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame on CFI Bots. Currently, the test SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame uses stale pointers of the views agains a map to verify that TextInputManager does not track the view anymore. This is causing issues in Linux CFI bots. This CL changes the test so that we now check for the number of views being tracked and verify that after crashing a frame, the number drops by one. BUG=619147 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Patchset #5 (id:80001) has been deleted
PTAL. https://codereview.chromium.org/2054163003/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2054163003/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:327: content::GetAllViewsRegisteredWithTextInputManager(active_contents()) On 2016/06/13 18:46:13, Charlie Reis (OOO til June 18) wrote: > We should just expose the count if we don't need to look at the views themselves > (GetRegisteredViewCountFromTextInputManager). Acknowledged. https://codereview.chromium.org/2054163003/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/2054163003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:86: GetAllViewsRegisteredWithTextInputManager(WebContents* web_contents); On 2016/06/13 18:46:13, Charlie Reis (OOO til June 18) wrote: > I'm not thrilled with having lots of raw test methods declared as friends. I'd > requested avoiding the first one as well, but I think that request fell between > the cracks: > https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... > > I'd much rather have public *ForTesting methods on this class, which makes it > clearer what's exposed and why. Can we update both of these? Apparently I had miss(e|understoo)d that comment. Sorry for that. I did add only one method for testing which returns the whole map itself. I use this to obtain both type and the count in public API. Do you think this is OK, or should I have two limited testing methods her, one for count and another for type? https://codereview.chromium.org/2054163003/diff/40001/content/public/test/tex... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/2054163003/diff/40001/content/public/test/tex... content/public/test/text_input_test_utils.h:36: // This method returns the set of all the views which are currently being On 2016/06/13 18:46:13, Charlie Reis (OOO til June 18) wrote: > nit: all views > nit: currently registered > > Also, let's make this return the count rather than the set, since the test > doesn't need more than that. We can then call > TextInputManager::GetRegisteredViewCountForTesting() in the body. This method is changed to return the count only now. https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:78: tester_.reset(nullptr); This is the public API's helper, which owns an internal observer. Since we add callbacks for observing to make it general purpose. Destroying it here makes sense because none of the observer helper classes here are intended to be called twice on their Wait() method. This was causing CFI errors because some of the observers in the test were still observing the TextInputManager after their view was destroyed. Sepcifically, I believe the major one is the one on line 310 (ViewTextInputTypeObserver) which was causing an issue on line 175. After we killed frames[0] in the test, we were still observing the TextInputManager using the observer instance in line 310. When the second view is killed, we get an observer call and the using the |view_| inside the observer which is now dangling.
Looks good apart from the change to RenderWidgetHostViewChildFrame::Destroy(). That seems problematic to me, and I'm not sure why it's needed. https://codereview.chromium.org/2054163003/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/2054163003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:86: GetAllViewsRegisteredWithTextInputManager(WebContents* web_contents); On 2016/06/25 00:10:35, EhsanK wrote: > On 2016/06/13 18:46:13, Charlie Reis (OOO til June 18) wrote: > > I'm not thrilled with having lots of raw test methods declared as friends. > I'd > > requested avoiding the first one as well, but I think that request fell > between > > the cracks: > > > https://codereview.chromium.org/1948343002/diff/280001/content/browser/render... > > > > I'd much rather have public *ForTesting methods on this class, which makes it > > clearer what's exposed and why. Can we update both of these? > > Apparently I had miss(e|understoo)d that comment. Sorry for that. > > I did add only one method for testing which returns the whole map itself. I use > this to obtain both type and the count in public API. > > Do you think this is OK, or should I have two limited testing methods her, one > for count and another for type? I like how patch set 5 has two limited testing methods (one for count and another for type). https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:78: tester_.reset(nullptr); On 2016/06/25 00:10:35, EhsanK wrote: > This is the public API's helper, which owns an internal observer. Since we add > callbacks for observing to make it general purpose. > > Destroying it here makes sense because none of the observer helper classes here > are intended to be called twice on their Wait() method. > > This was causing CFI errors because some of the observers in the test were still > observing the TextInputManager after their view was destroyed. Sepcifically, I > believe the major one is the one on line 310 (ViewTextInputTypeObserver) which > was causing an issue on line 175. > > After we killed frames[0] in the test, we were still observing the > TextInputManager using the observer instance in line 310. When the second view > is killed, we get an observer call and the using the |view_| inside the observer > which is now dangling. Acknowledged. https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:333: DCHECK_LT(2U, registered_views_count); This is probably easier to read as: // There are at least 2 views at this point, possibly more. DCHECK_GT(registered_views_count, 2U); https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:343: // Verifying that the TextInputManager is no longer tracking TextInputState nit: Verify https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:348: // Now crash the second <iframe> which has an active view. nit: Add blank line before. https://codereview.chromium.org/2054163003/diff/100001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2054163003/diff/100001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:293: text_input_manager_->Unregister(this); I thought RWHVBase took care of this. Won't this cause a problem when we get to ~RWHVBase, when text_input_manager_ is still defined but the view is no longer registered? If we do actually need this, do all RWHV subclasses need it?
ekaramad@chromium.org changed reviewers: + kenrb@chromium.org
ekaramad@chromium.org changed reviewers: + kenrb@chromium.org
Thanks Charlie! Could you please take another look? Re your last question: I think it should be only RWHVCF since it has a deferred deletion. kenrb@ might have a better opinion on this? https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:333: DCHECK_LT(2U, registered_views_count); On 2016/06/28 18:12:41, Charlie Reis wrote: > This is probably easier to read as: > > // There are at least 2 views at this point, possibly more. > DCHECK_GT(registered_views_count, 2U); Acknowledged. https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:343: // Verifying that the TextInputManager is no longer tracking TextInputState On 2016/06/28 18:12:41, Charlie Reis wrote: > nit: Verify Done. https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:348: // Now crash the second <iframe> which has an active view. On 2016/06/28 18:12:41, Charlie Reis wrote: > nit: Add blank line before. Done. https://codereview.chromium.org/2054163003/diff/100001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2054163003/diff/100001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:293: text_input_manager_->Unregister(this); On 2016/06/28 18:12:41, Charlie Reis wrote: > I thought RWHVBase took care of this. Won't this cause a problem when we get to > ~RWHVBase, when text_input_manager_ is still defined but the view is no longer > registered? > > If we do actually need this, do all RWHV subclasses need it? When we unregister, TextInputManager calls the method DidUnregisterFromTextInputManager, at which point, the view loses its reference to TextInputManager: https://cs.chromium.org/chromium/src/content/browser/renderer_host/text_input... So I believe we are safe here. But as for why are doing it here, I have to defer to kenrb@ for better reasons. My justification is that at this point we are done with the RWHVCF and for what is worth, the observer call for destruction has already been called on line 287 plus the host is detached from the view. So there will be no more incoming IPC for TextInputState at this stage. On the other hand, I do not think the way it was before is wrong. But there will be a period of time from here until the dtor of base class where we might still have an active view but no active widget. This will not causes crashes (since we check against nullptr before using GetActiveWidget()), but is weird.
Thanks Charlie! Could you please take another look? Re your last question: I think it should be only RWHVCF since it has a deferred deletion. kenrb@ might have a better opinion on this? https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:333: DCHECK_LT(2U, registered_views_count); On 2016/06/28 18:12:41, Charlie Reis wrote: > This is probably easier to read as: > > // There are at least 2 views at this point, possibly more. > DCHECK_GT(registered_views_count, 2U); Acknowledged. https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:343: // Verifying that the TextInputManager is no longer tracking TextInputState On 2016/06/28 18:12:41, Charlie Reis wrote: > nit: Verify Done. https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:348: // Now crash the second <iframe> which has an active view. On 2016/06/28 18:12:41, Charlie Reis wrote: > nit: Add blank line before. Done. https://codereview.chromium.org/2054163003/diff/100001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2054163003/diff/100001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:293: text_input_manager_->Unregister(this); On 2016/06/28 18:12:41, Charlie Reis wrote: > I thought RWHVBase took care of this. Won't this cause a problem when we get to > ~RWHVBase, when text_input_manager_ is still defined but the view is no longer > registered? > > If we do actually need this, do all RWHV subclasses need it? When we unregister, TextInputManager calls the method DidUnregisterFromTextInputManager, at which point, the view loses its reference to TextInputManager: https://cs.chromium.org/chromium/src/content/browser/renderer_host/text_input... So I believe we are safe here. But as for why are doing it here, I have to defer to kenrb@ for better reasons. My justification is that at this point we are done with the RWHVCF and for what is worth, the observer call for destruction has already been called on line 287 plus the host is detached from the view. So there will be no more incoming IPC for TextInputState at this stage. On the other hand, I do not think the way it was before is wrong. But there will be a period of time from here until the dtor of base class where we might still have an active view but no active widget. This will not causes crashes (since we check against nullptr before using GetActiveWidget()), but is weird.
On 2016/06/28 18:56:54, EhsanK wrote: > Thanks Charlie! Could you please take another look? > > Re your last question: I think it should be only RWHVCF since it has a deferred > deletion. kenrb@ might have a better opinion on this? > > https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... > File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc > (right): > > https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... > chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:333: > DCHECK_LT(2U, registered_views_count); > On 2016/06/28 18:12:41, Charlie Reis wrote: > > This is probably easier to read as: > > > > // There are at least 2 views at this point, possibly more. > > DCHECK_GT(registered_views_count, 2U); > > Acknowledged. > > https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... > chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:343: // > Verifying that the TextInputManager is no longer tracking TextInputState > On 2016/06/28 18:12:41, Charlie Reis wrote: > > nit: Verify > > Done. > > https://codereview.chromium.org/2054163003/diff/100001/chrome/browser/rendere... > chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:348: // > Now crash the second <iframe> which has an active view. > On 2016/06/28 18:12:41, Charlie Reis wrote: > > nit: Add blank line before. > > Done. > > https://codereview.chromium.org/2054163003/diff/100001/content/browser/frame_... > File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): > > https://codereview.chromium.org/2054163003/diff/100001/content/browser/frame_... > content/browser/frame_host/render_widget_host_view_child_frame.cc:293: > text_input_manager_->Unregister(this); > On 2016/06/28 18:12:41, Charlie Reis wrote: > > I thought RWHVBase took care of this. Won't this cause a problem when we get > to > > ~RWHVBase, when text_input_manager_ is still defined but the view is no longer > > registered? > > > > If we do actually need this, do all RWHV subclasses need it? > > When we unregister, TextInputManager calls the method > DidUnregisterFromTextInputManager, at which point, the view loses its reference > to TextInputManager: > https://cs.chromium.org/chromium/src/content/browser/renderer_host/text_input... > > So I believe we are safe here. But as for why are doing it here, I have to defer > to kenrb@ for better reasons. My justification is that at this point we are done > with the RWHVCF and for what is worth, the observer call for destruction has > already been called on line 287 plus the host is detached from the view. So > there will be no more incoming IPC for TextInputState at this stage. > > On the other hand, I do not think the way it was before is wrong. But there will > be a period of time from here until the dtor of base class where we might still > have an active view but no active widget. This will not causes crashes (since we > check against nullptr before using GetActiveWidget()), but is weird. BTW, added kenrb@ for input on the change to RWHVCF::Destroy(). Ken could you please take a look?
https://codereview.chromium.org/2054163003/diff/100001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2054163003/diff/100001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:293: text_input_manager_->Unregister(this); On 2016/06/28 18:56:54, EhsanK wrote: > On 2016/06/28 18:12:41, Charlie Reis wrote: > > I thought RWHVBase took care of this. Won't this cause a problem when we get > to > > ~RWHVBase, when text_input_manager_ is still defined but the view is no longer > > registered? > > > > If we do actually need this, do all RWHV subclasses need it? > > When we unregister, TextInputManager calls the method > DidUnregisterFromTextInputManager, at which point, the view loses its reference > to TextInputManager: > https://cs.chromium.org/chromium/src/content/browser/renderer_host/text_input... > > So I believe we are safe here. But as for why are doing it here, I have to defer > to kenrb@ for better reasons. My justification is that at this point we are done > with the RWHVCF and for what is worth, the observer call for destruction has > already been called on line 287 plus the host is detached from the view. So > there will be no more incoming IPC for TextInputState at this stage. > > On the other hand, I do not think the way it was before is wrong. But there will > be a period of time from here until the dtor of base class where we might still > have an active view but no active widget. This will not causes crashes (since we > check against nullptr before using GetActiveWidget()), but is weird. It isn't necessary, as I understand it, because you added checks in other places to verify that this view's host is non-NULL. I believe unregistering here is redundant with those checks because it creates the guarantee that GetActiveWidget() will return an actual widget. fwiw, I just filed https://crbug.com/624065 to see about removing the DeleteSoon call and getting rid of the troublesome state of having living RenderWidgetHostViews with no RenderWidgetHosts.
Thanks Ken, Charlie! PTAL.
Thanks, LGTM with nits. https://codereview.chromium.org/2054163003/diff/140001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2054163003/diff/140001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:31: #include "content/browser/renderer_host/text_input_manager.h" nit: No longer needed. https://codereview.chromium.org/2054163003/diff/140001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:291: nit: No longer needed. (This file shouldn't need to change anymore.) https://codereview.chromium.org/2054163003/diff/140001/content/public/test/te... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/2054163003/diff/140001/content/public/test/te... content/public/test/text_input_test_utils.h:10: #include <unordered_set> Is this stale? (Maybe it belongs in the .cc file?)
Thanks for the reviews! Missing sky@'s LGTM for chrome/*. sky@: Could you please take a look, Thanks! https://codereview.chromium.org/2054163003/diff/140001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2054163003/diff/140001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:31: #include "content/browser/renderer_host/text_input_manager.h" On 2016/06/28 21:39:34, Charlie Reis wrote: > nit: No longer needed. Acknowledged. https://codereview.chromium.org/2054163003/diff/140001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.cc:291: On 2016/06/28 21:39:34, Charlie Reis wrote: > nit: No longer needed. (This file shouldn't need to change anymore.) Acknowledged. https://codereview.chromium.org/2054163003/diff/140001/content/public/test/te... File content/public/test/text_input_test_utils.h (right): https://codereview.chromium.org/2054163003/diff/140001/content/public/test/te... content/public/test/text_input_test_utils.h:10: #include <unordered_set> On 2016/06/28 21:39:34, Charlie Reis wrote: > Is this stale? (Maybe it belongs in the .cc file?) Thanks for pointing this out. This goes to cc and the ones above and below are removed.
LGTM with EXPECT https://codereview.chromium.org/2054163003/diff/160001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2054163003/diff/160001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:333: DCHECK_GT(registered_views_count, 2U); EXPECT?
Thanks for the reviews! I will CQ after dry-run. https://codereview.chromium.org/2054163003/diff/160001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2054163003/diff/160001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:333: DCHECK_GT(registered_views_count, 2U); On 2016/06/29 03:16:06, sky wrote: > EXPECT? Done.
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: This issue passed the CQ 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 creis@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2054163003/#ps180001 (title: "Addressing sky@'s comments")
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 #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Fix SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame on CFI Bots. Currently, the test SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame uses stale pointers of the views agains a map to verify that TextInputManager does not track the view anymore. This is causing issues in Linux CFI bots. This CL changes the test so that we now check for the number of views being tracked and verify that after crashing a frame, the number drops by one. BUG=619147 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame on CFI Bots. Currently, the test SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame uses stale pointers of the views agains a map to verify that TextInputManager does not track the view anymore. This is causing issues in Linux CFI bots. This CL changes the test so that we now check for the number of views being tracked and verify that after crashing a frame, the number drops by one. BUG=619147 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/936536dbbfd6cb9cf468c80aba316cabafb0f002 Cr-Commit-Position: refs/heads/master@{#402736} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/936536dbbfd6cb9cf468c80aba316cabafb0f002 Cr-Commit-Position: refs/heads/master@{#402736} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
