Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2422973003:
Fix TextInputClientMac related crashes of Fullscreen RenderWidget (Closed)
DescriptionTextInputClientObsever should skip commands from fullscreen RenderWidgetHosts
corresponding to plugins.
TextInputClientObserver assumes that the RenderWidget::GetWebWidget() always
returns a WebFrameWidget.
This is not true when the RenderWidget is a RenderWidgetFullScreen. Therefore,
pressing Ctrl+Command+D
on a fullscreen flash will lead to a DCHECK firing in dev builds as well as
crashes in current Canary.
This CL will avoid sending any IPCs from TextInputClientMac to the fullscreen
RenderWidget.
BUG=656525, 663384
Committed: https://crrev.com/aeb3c5c70a6ab7472e802d424c9c4a628e819283
Cr-Commit-Position: refs/heads/master@{#430842}
Patch Set 1 #Patch Set 2 #
Total comments: 8
Patch Set 3 : Avoid sending the IPC to Fullscreen RenderWidget #
Total comments: 10
Patch Set 4 : Addressing alexmos@'s comments #
Total comments: 2
Patch Set 5 : Fixed a comment #Patch Set 6 : Rebased #
Messages
Total messages: 47 (28 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
ekaramad@chromium.org changed reviewers: + avi@chromium.org
Please, take a look. Thanks! https://codereview.chromium.org/2422973003/diff/20001/content/renderer/text_i... File content/renderer/text_input_client_observer.cc (right): https://codereview.chromium.org/2422973003/diff/20001/content/renderer/text_i... content/renderer/text_input_client_observer.cc:89: if (GetWebFrameWidget()) { When this is false, it is not necessarily because of a transient state (full screen flash does not have a frame widget). I think the right thing to do is to not send the IPC for a full screen flash to its RWH, but rather, send it to the RWH of embedder of the flash. I was not sure however what the straightforward way of getting embedder's RWH from full screen flash RWH is.
avi@chromium.org changed reviewers: + dcheng@chromium.org, lfg@chromium.org
Daniel/Lucas, thoughts on fixing this at the IPC level? https://codereview.chromium.org/2422973003/diff/20001/content/renderer/text_i... File content/renderer/text_input_client_observer.cc (right): https://codereview.chromium.org/2422973003/diff/20001/content/renderer/text_i... content/renderer/text_input_client_observer.cc:89: if (GetWebFrameWidget()) { On 2016/10/18 01:59:37, EhsanK wrote: > When this is false, it is not necessarily because of a transient state (full > screen flash does not have a frame widget). > > I think the right thing to do is to not send the IPC for a full screen flash to > its RWH, but rather, send it to the RWH of embedder of the flash. I was not sure > however what the straightforward way of getting embedder's RWH from full screen > flash RWH is. Can we find out?
Thanks avi@! +alexmos@ for review and feedback on fullscreen plugin RWH. PTAL.
ekaramad@chromium.org changed reviewers: + alexmos@chromium.org
Actually adding alexmos@.
https://chromiumcodereview.appspot.com/2422973003/diff/20001/content/renderer... File content/renderer/text_input_client_observer.cc (right): https://chromiumcodereview.appspot.com/2422973003/diff/20001/content/renderer... content/renderer/text_input_client_observer.cc:89: if (GetWebFrameWidget()) { On 2016/10/18 02:04:08, Avi wrote: > On 2016/10/18 01:59:37, EhsanK wrote: > > When this is false, it is not necessarily because of a transient state (full > > screen flash does not have a frame widget). > > > > I think the right thing to do is to not send the IPC for a full screen flash > to > > its RWH, but rather, send it to the RWH of embedder of the flash. I was not > sure > > however what the straightforward way of getting embedder's RWH from full > screen > > flash RWH is. > > Can we find out? Just to clarify, is this whole class for Mac dictionary popup? How should it behave for Flash (fullscreen or not)? If it's not supposed to do anything, then is it possible to detect this in the browser process and not send the IPC (TextInputClientMsg_StringAtPoint?) in the first place? When Flash fullscreen is active, WebContentsImpl maintains the fullscreened RWH's process ID and routing ID, and resolves them up in GetFullscreenRenderWidgetHostView(). So you could check whether the RWH's view is the same RWHV as that to detect whether you're in this mode or not. If the dictionary popup needs to do something for Flash, then I don't know how sending the IPC through the embedder's RWH would help. Feels like either way you'd need some special handling to extract data you need from the PepperWidget (which is the WebWidget that seems to be used for FS Flash) and/or RenderWidgetFullScreenPepper. If you wanted to get to the embedder here on the renderer side, the render_widget_'s opener_id_ should be set to the embedder RenderWidget (it's currently not exposed to the outside though), or alternatively it seems you could cast render_widget_ to RenderWidgetFullScreenPepper and then do plugin()->render_frame() to get to the embedding frame. Mentioning just in case; not sure if you'll need this. This doc is a bit outdated, but still has good explanations about how fullscreen flash works (mostly browser side): http://www.chromium.org/developers/design-documents/embedding-flash-fullscree...
On 2016/10/18 17:52:01, alexmos wrote: > https://chromiumcodereview.appspot.com/2422973003/diff/20001/content/renderer... > File content/renderer/text_input_client_observer.cc (right): > > https://chromiumcodereview.appspot.com/2422973003/diff/20001/content/renderer... > content/renderer/text_input_client_observer.cc:89: if (GetWebFrameWidget()) { > On 2016/10/18 02:04:08, Avi wrote: > > On 2016/10/18 01:59:37, EhsanK wrote: > > > When this is false, it is not necessarily because of a transient state (full > > > screen flash does not have a frame widget). > > > > > > I think the right thing to do is to not send the IPC for a full screen flash > > to > > > its RWH, but rather, send it to the RWH of embedder of the flash. I was not > > sure > > > however what the straightforward way of getting embedder's RWH from full > > screen > > > flash RWH is. > > > > Can we find out? > > Just to clarify, is this whole class for Mac dictionary popup? How should it > behave for Flash (fullscreen or not)? > > If it's not supposed to do anything, then is it possible to detect this in the > browser process and not send the IPC (TextInputClientMsg_StringAtPoint?) in the > first place? When Flash fullscreen is active, WebContentsImpl maintains the > fullscreened RWH's process ID and routing ID, and resolves them up in > GetFullscreenRenderWidgetHostView(). So you could check whether the RWH's view > is the same RWHV as that to detect whether you're in this mode or not. > > If the dictionary popup needs to do something for Flash, then I don't know how > sending the IPC through the embedder's RWH would help. Feels like either way > you'd need some special handling to extract data you need from the PepperWidget > (which is the WebWidget that seems to be used for FS Flash) and/or > RenderWidgetFullScreenPepper. > > If you wanted to get to the embedder here on the renderer side, the > render_widget_'s opener_id_ should be set to the embedder RenderWidget (it's > currently not exposed to the outside though), or alternatively it seems you > could cast render_widget_ to RenderWidgetFullScreenPepper and then do > plugin()->render_frame() to get to the embedding frame. Mentioning just in > case; not sure if you'll need this. > > This doc is a bit outdated, but still has good explanations about how fullscreen > flash works (mostly browser side): > http://www.chromium.org/developers/design-documents/embedding-flash-fullscree... Thanks Alex! > Just to clarify, is this whole class for Mac dictionary popup? How should it > behave for Flash (fullscreen or not)? For IME as well (and two word lookups: right click and Ctrl+Command+D). I noticed we do not even get Look up in context menus inside flash (not full screen). But we can have IME inside flash. I will verify the behavior for IME. > If it's not supposed to do anything, then is it possible to detect this in the > browser process and not send the IPC (TextInputClientMsg_StringAtPoint?) in the > first place? When Flash fullscreen is active, WebContentsImpl maintains the > fullscreened RWH's process ID and routing ID, and resolves them up in > GetFullscreenRenderWidgetHostView(). So you could check whether the RWH's view > is the same RWHV as that to detect whether you're in this mode or not. On a second thought I guess we might need to handle some IPCs for flash, at least for IME (maybe). > If the dictionary popup needs to do something for Flash, then I don't know how > sending the IPC through the embedder's RWH would help. Feels like either way > you'd need some special handling to extract data you need from the PepperWidget > (which is the WebWidget that seems to be used for FS Flash) and/or > RenderWidgetFullScreenPepper. As I understand, normally, for IME, and also word lookup, we send the IPC to embedder and embedder calls the appropriate API on focused pepper plugin. That is why I figured maybe we should send it to the original embedder even when the flash is fullscreen. > If you wanted to get to the embedder here on the renderer side, the > render_widget_'s opener_id_ should be set to the embedder RenderWidget (it's > currently not exposed to the outside though), or alternatively it seems you > could cast render_widget_ to RenderWidgetFullScreenPepper and then do > plugin()->render_frame() to get to the embedding frame. Mentioning just in > case; not sure if you'll need this. I think this might be cleaner, assuming that we can safely detect the RenderWidgetFullScreenPepper, i.e., is it safe to say if the RenderWidget::webwidget is not a WebFrameWidget nor a WebView then it has to be the pepper case? Thanks for the document link!
> > Just to clarify, is this whole class for Mac dictionary popup? How should it > > behave for Flash (fullscreen or not)? > For IME as well (and two word lookups: right click and Ctrl+Command+D). I > noticed we do not even get Look > up in context menus inside flash (not full screen). But we can have IME inside > flash. I will verify the > behavior for IME. > > > If it's not supposed to do anything, then is it possible to detect this in the > > browser process and not send the IPC (TextInputClientMsg_StringAtPoint?) in > the > > first place? When Flash fullscreen is active, WebContentsImpl maintains the > > fullscreened RWH's process ID and routing ID, and resolves them up in > > GetFullscreenRenderWidgetHostView(). So you could check whether the RWH's > view > > is the same RWHV as that to detect whether you're in this mode or not. > On a second thought I guess we might need to handle some IPCs for flash, at > least > for IME (maybe). > > > If the dictionary popup needs to do something for Flash, then I don't know how > > sending the IPC through the embedder's RWH would help. Feels like either way > > you'd need some special handling to extract data you need from the > PepperWidget > > (which is the WebWidget that seems to be used for FS Flash) and/or > > RenderWidgetFullScreenPepper. > As I understand, normally, for IME, and also word lookup, we send the IPC to > embedder and embedder calls the appropriate API on focused pepper plugin. That > is why I figured maybe we should send it to the original embedder even when the > flash is fullscreen. Ah, if that plumbing is already there for similar calls, then it might make sense to do this. Do you know how does that code gets the embedder RWH in the non-fullscreen case, and can that be reused here? > > > If you wanted to get to the embedder here on the renderer side, the > > render_widget_'s opener_id_ should be set to the embedder RenderWidget (it's > > currently not exposed to the outside though), or alternatively it seems you > > could cast render_widget_ to RenderWidgetFullScreenPepper and then do > > plugin()->render_frame() to get to the embedding frame. Mentioning just in > > case; not sure if you'll need this. > I think this might be cleaner, assuming that we can safely detect the > RenderWidgetFullScreenPepper, i.e., > is it safe to say if the RenderWidget::webwidget is not a WebFrameWidget nor a > WebView then it has to be > the pepper case? There's also the popups case with WebPagePopup (e.g., <select> drop-downs, or color/date chooser), which can also receive input events, though I don't know how this interacts with IME.
https://codereview.chromium.org/2422973003/diff/20001/content/renderer/text_i... File content/renderer/text_input_client_observer.cc (right): https://codereview.chromium.org/2422973003/diff/20001/content/renderer/text_i... content/renderer/text_input_client_observer.cc:117: PepperPluginInstanceImpl* focused_plugin = GetFocusedPepperPlugin(); This looks strange to me. Why would flash receive the IME events when it's not fullscreen, but stop when it goes fullscreen?
Description was changed from ========== TextInputClientObsever should skip commands from fullscreen RenderWidgetHosts corresponding to plugins. TextInputClientObserver assumes that the RenderWidget::GetWebWidget() always returns a WebFrameWidget. This is not true when the RenderWidget is a RenderWidgetFullScreen. Therefore, pressing Ctrl+Command+D on a fullscreen flash will lead to a DCHECK firing in dev builds as well as crashes in current Canary. This CL will remove the DCHECK since it is too occurant and instead adds proper code path to handle the scenarios where the WebWidget is not a WebFrameWidget. BUG=656525 ========== to ========== TextInputClientObsever should skip commands from fullscreen RenderWidgetHosts corresponding to plugins. TextInputClientObserver assumes that the RenderWidget::GetWebWidget() always returns a WebFrameWidget. This is not true when the RenderWidget is a RenderWidgetFullScreen. Therefore, pressing Ctrl+Command+D on a fullscreen flash will lead to a DCHECK firing in dev builds as well as crashes in current Canary. This CL will avoid sending any IPCs from TextInputClientMac to the fullscreen RenderWidget. BUG=656525 ==========
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_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Sorry for the delay in response. PTAL. I think at this point we should just ignore fullscreen flash to avoid renderer crashes. I will look into this soon when I start working on fixing other bugs in TextnputClientMac and add proper means to support anything we support for flash for the fullscreen case. https://codereview.chromium.org/2422973003/diff/20001/content/renderer/text_i... File content/renderer/text_input_client_observer.cc (right): https://codereview.chromium.org/2422973003/diff/20001/content/renderer/text_i... content/renderer/text_input_client_observer.cc:89: if (GetWebFrameWidget()) { On 2016/10/18 17:52:01, alexmos wrote: > On 2016/10/18 02:04:08, Avi wrote: > > On 2016/10/18 01:59:37, EhsanK wrote: > > > When this is false, it is not necessarily because of a transient state (full > > > screen flash does not have a frame widget). > > > > > > I think the right thing to do is to not send the IPC for a full screen flash > > to > > > its RWH, but rather, send it to the RWH of embedder of the flash. I was not > > sure > > > however what the straightforward way of getting embedder's RWH from full > > screen > > > flash RWH is. > > > > Can we find out? > > Just to clarify, is this whole class for Mac dictionary popup? How should it > behave for Flash (fullscreen or not)? > > If it's not supposed to do anything, then is it possible to detect this in the > browser process and not send the IPC (TextInputClientMsg_StringAtPoint?) in the > first place? When Flash fullscreen is active, WebContentsImpl maintains the > fullscreened RWH's process ID and routing ID, and resolves them up in > GetFullscreenRenderWidgetHostView(). So you could check whether the RWH's view > is the same RWHV as that to detect whether you're in this mode or not. > > If the dictionary popup needs to do something for Flash, then I don't know how > sending the IPC through the embedder's RWH would help. Feels like either way > you'd need some special handling to extract data you need from the PepperWidget > (which is the WebWidget that seems to be used for FS Flash) and/or > RenderWidgetFullScreenPepper. > > If you wanted to get to the embedder here on the renderer side, the > render_widget_'s opener_id_ should be set to the embedder RenderWidget (it's > currently not exposed to the outside though), or alternatively it seems you > could cast render_widget_ to RenderWidgetFullScreenPepper and then do > plugin()->render_frame() to get to the embedding frame. Mentioning just in > case; not sure if you'll need this. > > This doc is a bit outdated, but still has good explanations about how fullscreen > flash works (mostly browser side): > http://www.chromium.org/developers/design-documents/embedding-flash-fullscree... > Except for OnCharacterIndexForPoint which is for IME. I think for now we should just stop sending the IPC when there is a fullscreen flash. I will have to find a nice way to keep track of the parent of the flash to send the IPC to from the browser side. Also I was not clear on how I could get the parent RenderWidget on the renderer side? Is it possible to use the opener_id_ for that? Although, if we want the IPC to be handled for flash by its parent, then we should as well send it to the parent RenderWidget. https://codereview.chromium.org/2422973003/diff/20001/content/renderer/text_i... content/renderer/text_input_client_observer.cc:117: PepperPluginInstanceImpl* focused_plugin = GetFocusedPepperPlugin(); On 2016/10/19 17:40:32, lfg wrote: > This looks strange to me. Why would flash receive the IME events when it's not > fullscreen, but stop when it goes fullscreen? This is why I think we should send the IPC to the embedder of the flash. I assume the fullscreen flash is still a focused pepper plugin, right? I think for now we should stop sending the IPC. I will look into the IME issue with characterIndexForPoint. But it will not break or crash IME (only perhaps the composition window might appear at incorrect position. I don't think it should stop us from landing this fix first).
A couple of comments, but overall I think not sending these IPCs to fullscreen RWHs makes sense in the short term. https://codereview.chromium.org/2422973003/diff/20001/content/renderer/text_i... File content/renderer/text_input_client_observer.cc (right): https://codereview.chromium.org/2422973003/diff/20001/content/renderer/text_i... content/renderer/text_input_client_observer.cc:89: if (GetWebFrameWidget()) { > Also I was not clear on how I could get the parent RenderWidget on the renderer > side? Is it possible to use the opener_id_ for that? Although, if we want the > IPC to be handled for flash by its parent, then we should as well send it to the > parent RenderWidget. Yes, I think you could opener_id_ to get the the parent RenderWidget, though that's currently not really exposed, and I'd try to avoid using it if possible. I agree that we should just send it to the parent RenderWidget in the first place. https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:207: // Returns the current fullscreen RenderWidgetHostImpl if any. Maybe also note that this is only used for Flash fullscreen and will return nullptr for other kinds of fullscreen, like HTML5 fullscreen. (To prevent someone from misusing it in the future.) https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_client_mac.mm (right): https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_client_mac.mm:24: // IPC to the parent widget of the plugin. Let's add a bug reference here. https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_client_mac.mm:25: bool SendMessageToRenderWidge(RenderWidgetHostImpl* widget, s/SendMessageToRenderWidge/SendMessageToRenderWidget/ or perhaps MaybeSendMessageToRenderWidget? https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_client_mac.mm:28: return false; Do you need to delete the message here as well? https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_client_mac.mm:34: maybe DCHECK that message->routing_id() == widget->GetRoutingID()?
Thanks Alex! PTAL. https://codereview.chromium.org/2422973003/diff/20001/content/renderer/text_i... File content/renderer/text_input_client_observer.cc (right): https://codereview.chromium.org/2422973003/diff/20001/content/renderer/text_i... content/renderer/text_input_client_observer.cc:89: if (GetWebFrameWidget()) { On 2016/11/04 20:34:03, alexmos wrote: > > Also I was not clear on how I could get the parent RenderWidget on the > renderer > > side? Is it possible to use the opener_id_ for that? Although, if we want the > > IPC to be handled for flash by its parent, then we should as well send it to > the > > parent RenderWidget. > > Yes, I think you could opener_id_ to get the the parent RenderWidget, though > that's currently not really exposed, and I'd try to avoid using it if possible. > I agree that we should just send it to the parent RenderWidget in the first > place. Acknowledged. https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:207: // Returns the current fullscreen RenderWidgetHostImpl if any. On 2016/11/04 20:34:03, alexmos wrote: > Maybe also note that this is only used for Flash fullscreen and will return > nullptr for other kinds of fullscreen, like HTML5 fullscreen. (To prevent > someone from misusing it in the future.) Acknowledged. Thanks! Does the new comment sound better? https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_client_mac.mm (right): https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_client_mac.mm:24: // IPC to the parent widget of the plugin. On 2016/11/04 20:34:03, alexmos wrote: > Let's add a bug reference here. Done. https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_client_mac.mm:25: bool SendMessageToRenderWidge(RenderWidgetHostImpl* widget, On 2016/11/04 20:34:03, alexmos wrote: > s/SendMessageToRenderWidge/SendMessageToRenderWidget/ > > or perhaps MaybeSendMessageToRenderWidget? Done. https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_client_mac.mm:28: return false; On 2016/11/04 20:34:03, alexmos wrote: > Do you need to delete the message here as well? Yes. Thanks! https://codereview.chromium.org/2422973003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_client_mac.mm:34: On 2016/11/04 20:34:03, alexmos wrote: > maybe DCHECK that message->routing_id() == widget->GetRoutingID()? Acknowledged. I guess this makes sure we cannot send arbitrary messages to random destinations using this method.
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.
Thanks, LGTM with nits. https://codereview.chromium.org/2422973003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2422973003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:207: // Returns the current flash fullscreen RenderWidgetHostImpl if any. This will nit: s/flash/Flash/ https://codereview.chromium.org/2422973003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:208: // not return other forms of fullscreen widget such as HTML5. Let's just say "This is not intended for use with other types of fullscreen, such as HTML fullscreen, and will return nullptr for those cases." The current sentence implies there's a fullscreen widget for HTML fullscreen, but there actually isn't (instead, the frame's existing widget is resized to fill the entire screen).
Thanks for the reviews Alex! Please take another look.
LGTM
lgtm
Thanks for the reviews! CQ-ing.
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: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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 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 avi@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2422973003/#ps100001 (title: "Rebased")
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.
Description was changed from ========== TextInputClientObsever should skip commands from fullscreen RenderWidgetHosts corresponding to plugins. TextInputClientObserver assumes that the RenderWidget::GetWebWidget() always returns a WebFrameWidget. This is not true when the RenderWidget is a RenderWidgetFullScreen. Therefore, pressing Ctrl+Command+D on a fullscreen flash will lead to a DCHECK firing in dev builds as well as crashes in current Canary. This CL will avoid sending any IPCs from TextInputClientMac to the fullscreen RenderWidget. BUG=656525 ========== to ========== TextInputClientObsever should skip commands from fullscreen RenderWidgetHosts corresponding to plugins. TextInputClientObserver assumes that the RenderWidget::GetWebWidget() always returns a WebFrameWidget. This is not true when the RenderWidget is a RenderWidgetFullScreen. Therefore, pressing Ctrl+Command+D on a fullscreen flash will lead to a DCHECK firing in dev builds as well as crashes in current Canary. This CL will avoid sending any IPCs from TextInputClientMac to the fullscreen RenderWidget. BUG=656525 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== TextInputClientObsever should skip commands from fullscreen RenderWidgetHosts corresponding to plugins. TextInputClientObserver assumes that the RenderWidget::GetWebWidget() always returns a WebFrameWidget. This is not true when the RenderWidget is a RenderWidgetFullScreen. Therefore, pressing Ctrl+Command+D on a fullscreen flash will lead to a DCHECK firing in dev builds as well as crashes in current Canary. This CL will avoid sending any IPCs from TextInputClientMac to the fullscreen RenderWidget. BUG=656525 ========== to ========== TextInputClientObsever should skip commands from fullscreen RenderWidgetHosts corresponding to plugins. TextInputClientObserver assumes that the RenderWidget::GetWebWidget() always returns a WebFrameWidget. This is not true when the RenderWidget is a RenderWidgetFullScreen. Therefore, pressing Ctrl+Command+D on a fullscreen flash will lead to a DCHECK firing in dev builds as well as crashes in current Canary. This CL will avoid sending any IPCs from TextInputClientMac to the fullscreen RenderWidget. BUG=656525 Committed: https://crrev.com/aeb3c5c70a6ab7472e802d424c9c4a628e819283 Cr-Commit-Position: refs/heads/master@{#430842} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/aeb3c5c70a6ab7472e802d424c9c4a628e819283 Cr-Commit-Position: refs/heads/master@{#430842}
Message was sent while issue was closed.
Description was changed from ========== TextInputClientObsever should skip commands from fullscreen RenderWidgetHosts corresponding to plugins. TextInputClientObserver assumes that the RenderWidget::GetWebWidget() always returns a WebFrameWidget. This is not true when the RenderWidget is a RenderWidgetFullScreen. Therefore, pressing Ctrl+Command+D on a fullscreen flash will lead to a DCHECK firing in dev builds as well as crashes in current Canary. This CL will avoid sending any IPCs from TextInputClientMac to the fullscreen RenderWidget. BUG=656525 Committed: https://crrev.com/aeb3c5c70a6ab7472e802d424c9c4a628e819283 Cr-Commit-Position: refs/heads/master@{#430842} ========== to ========== TextInputClientObsever should skip commands from fullscreen RenderWidgetHosts corresponding to plugins. TextInputClientObserver assumes that the RenderWidget::GetWebWidget() always returns a WebFrameWidget. This is not true when the RenderWidget is a RenderWidgetFullScreen. Therefore, pressing Ctrl+Command+D on a fullscreen flash will lead to a DCHECK firing in dev builds as well as crashes in current Canary. This CL will avoid sending any IPCs from TextInputClientMac to the fullscreen RenderWidget. BUG=656525, 663384 Committed: https://crrev.com/aeb3c5c70a6ab7472e802d424c9c4a628e819283 Cr-Commit-Position: refs/heads/master@{#430842} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
