Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2522353002:
Remove the CHECK in RenderWidget::GetInputMethodController (Closed)
DescriptionRemove the CHECK in RenderWidget::GetInputMethodController
Is is possible to trigger CHECK(GetWidget()->isWebFrameWidget()) by sending IPCs
after the RenderViewImpl is swapped out. For the brief
duration that RenderViewImpl is swapped out but the frame widget is
destroyed, we get WebViewImpl from the call to GetWebWidget().
Since the issue reproduces quite often, this CL replaces the CHECK with an if-block and returns early when WebWidget is not a frame widget.
BUG=668106
.
Committed: https://crrev.com/5a9f52a658eab5f7827159109d425ce0d9176d58
Cr-Commit-Position: refs/heads/master@{#436824}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Alternative Approach: Move getActiveWebInputMethodController() to WebWidget (from WebFrameWidget). #Patch Set 3 : Removing the override from WebFrameWidget #
Total comments: 2
Patch Set 4 : Addressing creis@'s comments + Removing unused forward dec. #
Total comments: 3
Patch Set 5 : Back to checking for isWebFrameWidget() #Messages
Total messages: 40 (19 generated)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
ekaramad@chromium.org changed reviewers: + creis@chromium.org, nick@chromium.org
PTAL. https://codereview.chromium.org/2522353002/diff/1/content/renderer/render_wid... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2522353002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:2315: return nullptr; I should also note that this method can return null pointer for other reasons so as far as returning nullptr goes, it is OK. getActiveWebInputMethodController() can legitimately return nullptr, e.g., if the focused frame on the page belongs to different local root.
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Replace CHECK with if block since the CHECK is reproducible Is is possible to trigger the removed check by pumping IPCs related to IME while the page is navigating. If the IME arrives when the frame widget is destroyed, RenderViewImpl might be around which will return its WebViewImpl instead. BUG=668106. ========== to ========== Replace CHECK with if block since the CHECK is reproducible Is is possible to trigger CHECK(GetWidget()->isWebFrameWidget()) by sending IPCs after the RenderViewImpl is swapped out. For the brief duration that RenderViewImpl is swapped out but the frame widget is destroyed, we get WebViewImpl from the call to GetWebWidget(). In such cases, we should either check if GetWebWidget() is a WebFrameWidget, or move the corresponding logic, if possible, to WebWidget.h to avoid cast altogether. In the case of IME the latter seems a viable solution. BUG=668106. ==========
lgtm
ekaramad@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@chromium.org: Please review changes in
Thank you Nick! cc:+lfg@ I will wait for creis@ reviews as well since we had already discussed this CHECK in earlier patches (for context please take a look at discussions in this closed-but-not-landed CL:https://codereview.chromium.org/2497473003/)
lfg@chromium.org changed reviewers: + lfg@chromium.org
The change lgtm, but we should still figure out why are we sending IME messages to swapped-out RenderViews.
content/ LGTM, but I'll defer to dcheng on whether it makes sense in Blink. I'm glad we're removing the risk of bad cast (and thus the need for a CHECK guarding it), but it sounds unexpected that we'd get into this state in the first place. https://codereview.chromium.org/2522353002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebWidget.h (right): https://codereview.chromium.org/2522353002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/web/WebWidget.h:239: // This should only be overrode by fame widgets. nit: overridden nit: frame
Any idea where this is coming from? We've had this bug for awhile, but no one's looked into it (also see WebViewImpl::handleInputEvent).
Thanks for the reviews! dcheng@: As I understand we send a SwapOut message to old frame when navigating and if it is a main frame that will set RenderViewImpl swap out. Then an ACK is sent which will destroy the RenderWidgetHost when RenderFrameHostImpl is deleted. During this time there seems to be no checking in RenderWidgetHostImpl on what IPCs should go (some precautions seems to be taken in RenderFrameHostManager::SwapOutOldFrame but none seem to be taking care of this issue). Maybe we should have a flag in RenderWidgetHostImpl letting it know when the RenderWidget might be swapped out and then do not send IPCs?
Answering comments and apologies for the unfortunate rebase while addressing creis@'s comments. I was also thinking of filing a bug for this (untimely IPC transmission) issue given lfg@ and dcheng@'s comments. https://codereview.chromium.org/2522353002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebWidget.h (right): https://codereview.chromium.org/2522353002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/web/WebWidget.h:239: // This should only be overrode by fame widgets. On 2016/11/28 19:14:33, Charlie Reis (slow) wrote: > nit: overridden > nit: frame Acknowledged.
Description was changed from ========== Replace CHECK with if block since the CHECK is reproducible Is is possible to trigger CHECK(GetWidget()->isWebFrameWidget()) by sending IPCs after the RenderViewImpl is swapped out. For the brief duration that RenderViewImpl is swapped out but the frame widget is destroyed, we get WebViewImpl from the call to GetWebWidget(). In such cases, we should either check if GetWebWidget() is a WebFrameWidget, or move the corresponding logic, if possible, to WebWidget.h to avoid cast altogether. In the case of IME the latter seems a viable solution. BUG=668106. ========== to ========== Remove the CHECK in RenderWidget::GetInputMethodController Is is possible to trigger CHECK(GetWidget()->isWebFrameWidget()) by sending IPCs after the RenderViewImpl is swapped out. For the brief duration that RenderViewImpl is swapped out but the frame widget is destroyed, we get WebViewImpl from the call to GetWebWidget(). To avoid the potential errors in dealing with a WebViewImpl, the method getActiveInputMethodController() is moved from WebFrameWidget to WebWidget which will then obviate the need for GetInputMethodController() in RenderWidget as well as probing GetWebWidget() for a WebFrameWidget. BUG=668106. ==========
Description was changed from ========== Remove the CHECK in RenderWidget::GetInputMethodController Is is possible to trigger CHECK(GetWidget()->isWebFrameWidget()) by sending IPCs after the RenderViewImpl is swapped out. For the brief duration that RenderViewImpl is swapped out but the frame widget is destroyed, we get WebViewImpl from the call to GetWebWidget(). To avoid the potential errors in dealing with a WebViewImpl, the method getActiveInputMethodController() is moved from WebFrameWidget to WebWidget which will then obviate the need for GetInputMethodController() in RenderWidget as well as probing GetWebWidget() for a WebFrameWidget. BUG=668106. ========== to ========== Remove the CHECK in RenderWidget::GetInputMethodController Is is possible to trigger CHECK(GetWidget()->isWebFrameWidget()) by sending IPCs after the RenderViewImpl is swapped out. For the brief duration that RenderViewImpl is swapped out but the frame widget is destroyed, we get WebViewImpl from the call to GetWebWidget(). To avoid the potential errors in dealing with a WebViewImpl, the method getActiveInputMethodController() is moved from WebFrameWidget to WebWidget which will then obviate the need for GetInputMethodController() in RenderWidget as well as probing GetWebWidget() for a WebFrameWidget. BUG=668106. ==========
I'm not sure that extending a CanSendWhileSwappedOut-like approach to RenderWidgetHost is the right direction to take this code. On mojo it's not possible to switch on the IPC::Message type, so we'd have to figure out a different way to do it in the near future. I'd rather we drop the IPCs at the sender side or the reciever side, as appropriate. RenderWidget already has an is_swapped_out bit; are we certain that it's true in the crash reports? On Mon, Nov 28, 2016 at 11:30 AM, <ekaramad@chromium.org> wrote: > Answering comments and apologies for the unfortunate rebase while > addressing > creis@'s comments. > > I was also thinking of filing a bug for this (untimely IPC transmission) > issue > given lfg@ and dcheng@'s comments. > > > https://codereview.chromium.org/2522353002/diff/40001/ > third_party/WebKit/public/web/WebWidget.h > File third_party/WebKit/public/web/WebWidget.h (right): > > https://codereview.chromium.org/2522353002/diff/40001/ > third_party/WebKit/public/web/WebWidget.h#newcode239 > third_party/WebKit/public/web/WebWidget.h:239: // This should only be > overrode by fame widgets. > On 2016/11/28 19:14:33, Charlie Reis (slow) wrote: > > nit: overridden > > nit: frame > > Acknowledged. > > https://codereview.chromium.org/2522353002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/28 19:45:16, ncarter wrote: > I'm not sure that extending a CanSendWhileSwappedOut-like approach to > RenderWidgetHost is the right direction to take this code. On mojo it's not > possible to switch on the IPC::Message type, so we'd have to figure out a > different way to do it in the near future. > > I'd rather we drop the IPCs at the sender side or the reciever side, as > appropriate. RenderWidget already has an is_swapped_out bit; are we certain > that it's true in the crash reports? > > > > On Mon, Nov 28, 2016 at 11:30 AM, <mailto:ekaramad@chromium.org> wrote: > > > Answering comments and apologies for the unfortunate rebase while > > addressing > > creis@'s comments. > > > > I was also thinking of filing a bug for this (untimely IPC transmission) > > issue > > given lfg@ and dcheng@'s comments. > > > > > > https://codereview.chromium.org/2522353002/diff/40001/ > > third_party/WebKit/public/web/WebWidget.h > > File third_party/WebKit/public/web/WebWidget.h (right): > > > > https://codereview.chromium.org/2522353002/diff/40001/ > > third_party/WebKit/public/web/WebWidget.h#newcode239 > > third_party/WebKit/public/web/WebWidget.h:239: // This should only be > > overrode by fame widgets. > > On 2016/11/28 19:14:33, Charlie Reis (slow) wrote: > > > nit: overridden > > > nit: frame > > > > Acknowledged. > > > > https://codereview.chromium.org/2522353002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I am not a 100% sure if that is the case, but synthetically pumping some IPC and then navigating always end up with WebViewImpl having a WebRemoteFrame as main frame (which I guess means swapped_out is set).
On 2016/11/28 20:33:41, EhsanK wrote: > On 2016/11/28 19:45:16, ncarter wrote: > > I'm not sure that extending a CanSendWhileSwappedOut-like approach to > > RenderWidgetHost is the right direction to take this code. On mojo it's not > > possible to switch on the IPC::Message type, so we'd have to figure out a > > different way to do it in the near future. > > > > I'd rather we drop the IPCs at the sender side or the reciever side, as > > appropriate. RenderWidget already has an is_swapped_out bit; are we certain > > that it's true in the crash reports? > > > > > > > > On Mon, Nov 28, 2016 at 11:30 AM, <mailto:ekaramad@chromium.org> wrote: > > > > > Answering comments and apologies for the unfortunate rebase while > > > addressing > > > creis@'s comments. > > > > > > I was also thinking of filing a bug for this (untimely IPC transmission) > > > issue > > > given lfg@ and dcheng@'s comments. > > > > > > > > > https://codereview.chromium.org/2522353002/diff/40001/ > > > third_party/WebKit/public/web/WebWidget.h > > > File third_party/WebKit/public/web/WebWidget.h (right): > > > > > > https://codereview.chromium.org/2522353002/diff/40001/ > > > third_party/WebKit/public/web/WebWidget.h#newcode239 > > > third_party/WebKit/public/web/WebWidget.h:239: // This should only be > > > overrode by fame widgets. > > > On 2016/11/28 19:14:33, Charlie Reis (slow) wrote: > > > > nit: overridden > > > > nit: frame > > > > > > Acknowledged. > > > > > > https://codereview.chromium.org/2522353002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > I am not a 100% sure if that is the case, but synthetically pumping some IPC and > then navigating always end up with WebViewImpl > having a WebRemoteFrame as main frame (which I guess means swapped_out is set). Also I don't yet know why this specific CHECK seems to be happening only on Mac and and not on Aura?
LGTM with comments addressed, to highlight that this is a workaround. https://codereview.chromium.org/2522353002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebWidget.h (right): https://codereview.chromium.org/2522353002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebWidget.h:240: virtual WebInputMethodController* getActiveWebInputMethodController() { Please add a comment here that the presence of this method in WebWidget is temporary (the fact that we're getting it on something not associated iwth a WebFrameWidget means the browser is routing events to the wrong processes), and reference the relevant bugs.
https://codereview.chromium.org/2522353002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebWidget.h (right): https://codereview.chromium.org/2522353002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebWidget.h:240: virtual WebInputMethodController* getActiveWebInputMethodController() { On 2016/12/05 19:19:49, dcheng wrote: > Please add a comment here that the presence of this method in WebWidget is > temporary (the fact that we're getting it on something not associated iwth a > WebFrameWidget means the browser is routing events to the wrong processes), and > reference the relevant bugs. When I reviewed this, I considered this to be a more permanent solution (i.e., I thought it made sense to associate a WebInputMehtodController with a WebWidget, instead of just a WebFrameWidget). If we are doing this as a temporary fix, I'd rather use isWebFrameWidget() when calling getActiveWebInputMethodController() in RenderWidget.
https://codereview.chromium.org/2522353002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebWidget.h (right): https://codereview.chromium.org/2522353002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebWidget.h:240: virtual WebInputMethodController* getActiveWebInputMethodController() { On 2016/12/05 19:26:33, lfg wrote: > On 2016/12/05 19:19:49, dcheng wrote: > > Please add a comment here that the presence of this method in WebWidget is > > temporary (the fact that we're getting it on something not associated iwth a > > WebFrameWidget means the browser is routing events to the wrong processes), > and > > reference the relevant bugs. > > When I reviewed this, I considered this to be a more permanent solution (i.e., I > thought it made sense to associate a WebInputMehtodController with a WebWidget, > instead of just a WebFrameWidget). > > If we are doing this as a temporary fix, I'd rather use isWebFrameWidget() when > calling getActiveWebInputMethodController() in RenderWidget. dcheng@: I actually think it might be better to have this method in WebWidget.h for a while (even after the IPC issues is fixed). I think text input and IME are not necessarily limited to WebFrameWidget. Specifically, InputMethodController belongs to frame and WebInputMethodController is supposed to be the public wrapper. Methods such as setComposition() should be called on a WebFrameWidget. But textInputInfo() and textInputType() might have to stay inside WebWidget for a while. The logic for those is inside InputMethodController and to have them inside WebInputMethodController we will need to make the WebInputMethodController accessible through WebWidget for now (I encountered them in this refactoring CL: https://codereview.chromium.org/2508363003/ which is now blocked on this bug). In 'render_widget_unittest.cc', for example, we have a MockWebWidget which is a WebWidget and not a WebFrameWidget. Are we switching all those to WebFrameWidget? I guess not. So I think at least for a while getActiveInputMethodController() should be inside WebWidget and putting it in WebFrameWidget was not great. In summary, my suggestion is to keep the method in WebWidget. Finish all the IME and text input refactor and then revisit this if it is 'required' to move this to WebFrameWidget. WDYT?
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...
I reverted back to Pacth 1 with a comment and a TODO + bug for fixing IPC sent from browser when it is not supposed to be (for swapped out RW). PTAL.
On 2016/12/07 00:38:33, EhsanK wrote: > I reverted back to Pacth 1 with a comment and a TODO + bug for fixing IPC sent > from browser when it is not supposed to be (for swapped out RW). > > PTAL. lgtm
The CQ bit was unchecked by ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, creis@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2522353002/#ps80001 (title: "Back to checking for isWebFrameWidget()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the reviews! I will try to CQ.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481072384211570,
"parent_rev": "46a52b5dd3916f7da08a054ebdc8e8c171c606b8", "commit_rev":
"8a12514d1d154e6a4094e2a7d73a29d0ffbba1e1"}
Message was sent while issue was closed.
Description was changed from ========== Remove the CHECK in RenderWidget::GetInputMethodController Is is possible to trigger CHECK(GetWidget()->isWebFrameWidget()) by sending IPCs after the RenderViewImpl is swapped out. For the brief duration that RenderViewImpl is swapped out but the frame widget is destroyed, we get WebViewImpl from the call to GetWebWidget(). To avoid the potential errors in dealing with a WebViewImpl, the method getActiveInputMethodController() is moved from WebFrameWidget to WebWidget which will then obviate the need for GetInputMethodController() in RenderWidget as well as probing GetWebWidget() for a WebFrameWidget. BUG=668106. ========== to ========== Remove the CHECK in RenderWidget::GetInputMethodController Is is possible to trigger CHECK(GetWidget()->isWebFrameWidget()) by sending IPCs after the RenderViewImpl is swapped out. For the brief duration that RenderViewImpl is swapped out but the frame widget is destroyed, we get WebViewImpl from the call to GetWebWidget(). To avoid the potential errors in dealing with a WebViewImpl, the method getActiveInputMethodController() is moved from WebFrameWidget to WebWidget which will then obviate the need for GetInputMethodController() in RenderWidget as well as probing GetWebWidget() for a WebFrameWidget. BUG=668106. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Remove the CHECK in RenderWidget::GetInputMethodController Is is possible to trigger CHECK(GetWidget()->isWebFrameWidget()) by sending IPCs after the RenderViewImpl is swapped out. For the brief duration that RenderViewImpl is swapped out but the frame widget is destroyed, we get WebViewImpl from the call to GetWebWidget(). To avoid the potential errors in dealing with a WebViewImpl, the method getActiveInputMethodController() is moved from WebFrameWidget to WebWidget which will then obviate the need for GetInputMethodController() in RenderWidget as well as probing GetWebWidget() for a WebFrameWidget. BUG=668106. ========== to ========== Remove the CHECK in RenderWidget::GetInputMethodController Is is possible to trigger CHECK(GetWidget()->isWebFrameWidget()) by sending IPCs after the RenderViewImpl is swapped out. For the brief duration that RenderViewImpl is swapped out but the frame widget is destroyed, we get WebViewImpl from the call to GetWebWidget(). To avoid the potential errors in dealing with a WebViewImpl, the method getActiveInputMethodController() is moved from WebFrameWidget to WebWidget which will then obviate the need for GetInputMethodController() in RenderWidget as well as probing GetWebWidget() for a WebFrameWidget. BUG=668106. Committed: https://crrev.com/5a9f52a658eab5f7827159109d425ce0d9176d58 Cr-Commit-Position: refs/heads/master@{#436824} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5a9f52a658eab5f7827159109d425ce0d9176d58 Cr-Commit-Position: refs/heads/master@{#436824}
Message was sent while issue was closed.
Description was changed from ========== Remove the CHECK in RenderWidget::GetInputMethodController Is is possible to trigger CHECK(GetWidget()->isWebFrameWidget()) by sending IPCs after the RenderViewImpl is swapped out. For the brief duration that RenderViewImpl is swapped out but the frame widget is destroyed, we get WebViewImpl from the call to GetWebWidget(). To avoid the potential errors in dealing with a WebViewImpl, the method getActiveInputMethodController() is moved from WebFrameWidget to WebWidget which will then obviate the need for GetInputMethodController() in RenderWidget as well as probing GetWebWidget() for a WebFrameWidget. BUG=668106. Committed: https://crrev.com/5a9f52a658eab5f7827159109d425ce0d9176d58 Cr-Commit-Position: refs/heads/master@{#436824} ========== to ========== Remove the CHECK in RenderWidget::GetInputMethodController Is is possible to trigger CHECK(GetWidget()->isWebFrameWidget()) by sending IPCs after the RenderViewImpl is swapped out. For the brief duration that RenderViewImpl is swapped out but the frame widget is destroyed, we get WebViewImpl from the call to GetWebWidget(). Since the issue reproduces quite often, this CL replaces the CHECK with an if-block and returns early when WebWidget is not a frame widget. BUG=668106. Committed: https://crrev.com/5a9f52a658eab5f7827159109d425ce0d9176d58 Cr-Commit-Position: refs/heads/master@{#436824} ========== |
