Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12252016:
Prevented connecting drag drop events to a SwappedOut RenderViewHost in WebContentsViewGtk (Closed)
|
Created:
7 years, 10 months ago by mthiesse Modified:
4 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionPrevented connecting events to a SwappedOut RenderViewHost in WebContentsViewGtk
Added a check to see if the RenderWidgetHost passed to CreateViewForWidget is
SwappedOut before connecting events to it, or creating a DragDest from it.
Needed for: https://chromiumcodereview.appspot.com/12086095/
When a postMessage is sent to a guest process, like a BrowserPlugin guest,
WebContentsViewGtk::CreateViewForWidget is called with a swapped out
RenderWidgetHost. This RenderWidgetHost is then used as the WebDragDest for the
window, preventing the window from receiving Drag and Drop events.
BUG=161112, 177667
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188559
Patch Set 1 #
Total comments: 2
Patch Set 2 : A step back... #
Total comments: 1
Patch Set 3 : Use RenderViewSwappedIn to ensure we have a drag dest #
Total comments: 8
Patch Set 4 : #
Total comments: 1
Patch Set 5 : Fixed issues preventing the setting of the drag_dest_ on RenderViewSwappedIn #
Total comments: 14
Patch Set 6 : Addressed Comments #Patch Set 7 : sync #Patch Set 8 : Oops. Fix compile error. #
Messages
Total messages: 21 (0 generated)
https://codereview.chromium.org/12252016/diff/1/content/browser/web_contents/... File content/browser/web_contents/web_contents_view_gtk.cc (right): https://codereview.chromium.org/12252016/diff/1/content/browser/web_contents/... content/browser/web_contents/web_contents_view_gtk.cc:136: if (web_contents_->GetRenderManagerForTesting()->IsSwappedOut(rvh)) { This looks wrong...GetRenderManagerForTesting is supposed to be used in testing only (hence the name). However, I don't necessarily have a better solution. I'll let Evan guide you on this one.
On 2013/02/13 21:15:22, Fady Samuel wrote: > https://codereview.chromium.org/12252016/diff/1/content/browser/web_contents/... > File content/browser/web_contents/web_contents_view_gtk.cc (right): > > https://codereview.chromium.org/12252016/diff/1/content/browser/web_contents/... > content/browser/web_contents/web_contents_view_gtk.cc:136: if > (web_contents_->GetRenderManagerForTesting()->IsSwappedOut(rvh)) { > This looks wrong...GetRenderManagerForTesting is supposed to be used in testing > only (hence the name). > > However, I don't necessarily have a better solution. I'll let Evan guide you on > this one. Yes, I noticed that too, and also didn't have a better solution. I mean, I guess I could add an method in WebContentsImpl to call render_manager_->IsSwappedOut, but that sounds like it could be a pain to get reviewed...
I'm not the best reviewer, try jam@.
I'm not familiar with gtk or swapped out stuff. creis is the expert, redirecting to him
https://codereview.chromium.org/12252016/diff/1/content/browser/web_contents/... File content/browser/web_contents/web_contents_view_gtk.cc (right): https://codereview.chromium.org/12252016/diff/1/content/browser/web_contents/... content/browser/web_contents/web_contents_view_gtk.cc:136: if (web_contents_->GetRenderManagerForTesting()->IsSwappedOut(rvh)) { On 2013/02/13 21:15:22, Fady Samuel wrote: > This looks wrong...GetRenderManagerForTesting is supposed to be used in testing > only (hence the name). > > However, I don't necessarily have a better solution. I'll let Evan guide you on > this one. Fady's right-- this would be a layering violation. RenderViewHostManager is not supposed to be exposed outside WebContentsImpl. An alternative is to use RenderViewHostImpl::is_swapped_out(), but I think that's just masking the underlying problem. This method shouldn't be called with a swapped out RVH in the first place. In most cases, we filter out the messages sent to/from a swapped out RVH (see swapped_out_messages.cc). Is there a comparable place we could be preventing this bug at an earlier stage?
Alright, moved the checks for swapped out renderviews back into RenderViewHostManager and WebContentsImpl.
This won't work. (I'd also recommend running try jobs on these drafts, since I'm pretty sure this would break our cross-process postMessage tests.) https://codereview.chromium.org/12252016/diff/7001/content/browser/web_conten... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/12252016/diff/7001/content/browser/web_conten... content/browser/web_contents/web_contents_impl.cc:3459: // If the RenderViewHost is swapped out, we don't need to create a view for This is not true: we have to create a RenderView for it. We use it as a placeholder so that other windows can call postMessage (and other cross-origin JavaScript calls) on it. It sounds like there's some confusion about what being swapped out means. Let me know if you'd like to find some time to chat about it.
On 2013/02/21 02:22:16, creis wrote: > This won't work. (I'd also recommend running try jobs on these drafts, since > I'm pretty sure this would break our cross-process postMessage tests.) > > https://codereview.chromium.org/12252016/diff/7001/content/browser/web_conten... > File content/browser/web_contents/web_contents_impl.cc (right): > > https://codereview.chromium.org/12252016/diff/7001/content/browser/web_conten... > content/browser/web_contents/web_contents_impl.cc:3459: // If the RenderViewHost > is swapped out, we don't need to create a view for > This is not true: we have to create a RenderView for it. We use it as a > placeholder so that other windows can call postMessage (and other cross-origin > JavaScript calls) on it. > > It sounds like there's some confusion about what being swapped out means. Let > me know if you'd like to find some time to chat about it. There's a bit of confusion in the terminology here. He's not referring to RenderViews but rather browser-side RenderWidgetHostViews. A RenderWidgetHostView (as far as I understand) is the platform-specific code that actually displays the RenderView on screen by creating a native window or whatnot. Swapped out RenderViews are not visible on screen. However, now that you mention it, is it possible for a swapped out RenderView to become visible again? I know we had this discussion a long time ago, but I've since forgotten. I recall swapped out RenderViews permitted maintaining script connections between pages after one tab has done a cross process navigation. Does the swapped out RenderView go back to becoming visible if we go back?
On 2013/02/21 02:31:36, Fady Samuel wrote: > On 2013/02/21 02:22:16, creis wrote: > > This won't work. (I'd also recommend running try jobs on these drafts, since > > I'm pretty sure this would break our cross-process postMessage tests.) > > > > > https://codereview.chromium.org/12252016/diff/7001/content/browser/web_conten... > > File content/browser/web_contents/web_contents_impl.cc (right): > > > > > https://codereview.chromium.org/12252016/diff/7001/content/browser/web_conten... > > content/browser/web_contents/web_contents_impl.cc:3459: // If the > RenderViewHost > > is swapped out, we don't need to create a view for > > This is not true: we have to create a RenderView for it. We use it as a > > placeholder so that other windows can call postMessage (and other cross-origin > > JavaScript calls) on it. > > > > It sounds like there's some confusion about what being swapped out means. Let > > me know if you'd like to find some time to chat about it. > > There's a bit of confusion in the terminology here. He's not referring to > RenderViews but rather browser-side RenderWidgetHostViews. A > RenderWidgetHostView (as far as I understand) is the platform-specific code that > actually displays the RenderView on screen by creating a native window or > whatnot. Swapped out RenderViews are not visible on screen. > > However, now that you mention it, is it possible for a swapped out RenderView to > become visible again? I know we had this discussion a long time ago, but I've > since forgotten. I recall swapped out RenderViews permitted maintaining script > connections between pages after one tab has done a cross process navigation. > Does the swapped out RenderView go back to becoming visible if we go back? My mistake. Still, the swapped out RenderView can definitely be swapped back in, so I think we need to create a RenderWidgetHostView for it as well. (Plus one that was active can later become swapped out.)
PTAL creis. Hopefully this addresses your concerns. I believe, according to WebContentsImpl::RenderViewCreated, if a RVH is created swapped out it won't ever need a RenderWidgetHostView.
I think you're on the right track, but it seems like we can make it simpler. On 2013/02/26 22:26:45, mthiesse wrote: > PTAL creis. > > Hopefully this addresses your concerns. I believe, according to > WebContentsImpl::RenderViewCreated, if a RVH is created swapped out it won't > ever need a RenderWidgetHostView. That's not true. A swapped out RVH can later be swapped in, in which case it needs a RenderWidgetHostView. https://codereview.chromium.org/12252016/diff/16001/content/browser/web_conte... File content/browser/web_contents/web_contents_view_gtk.cc (right): https://codereview.chromium.org/12252016/diff/16001/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.cc:231: // receive any events. This comment doesn't make sense to me. First, creating swapped out RVHs for the opener chain is only one of multiple ways we could get here. (The BrowserPlugin also does this, which was the cause of the bug.) Also, WebUI is unrelated to whether it will receive events. The only reason WebUI was mentioned in RenderViewCreated is that there was WebUI-specific logic below the return statement. In other words, most of this is a red herring for why we want this logic. The real reason is that we don't want to change any of this class's state (e.g., drag_dest_) for RenderViewHosts that are currently swapped out, and thus not visible. It will be important to update this state when we swap in a new RenderViewHost, which happens below in RenderViewSwappedIn. Can you put something like that just above the IsRenderView() conditional? I want it to be clear to future developers that any local state should not be updated for swapped out RVHs, and drag_dest_ is just one example. https://codereview.chromium.org/12252016/diff/16001/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.cc:275: RenderViewHost* old_host) { Shouldn't we update drag_dest_ every time we get here, regardless of whether there's a current one or what the old host had? If we've just changed our RenderViewHost, then it seems like we necessarily need to change our drag_dest_. If that's the case, then we don't need old_host in the API. https://codereview.chromium.org/12252016/diff/16001/content/port/browser/web_... File content/port/browser/web_contents_view_port.h (right): https://codereview.chromium.org/12252016/diff/16001/content/port/browser/web_... content/port/browser/web_contents_view_port.h:45: // swapped in. |old_host| may be null. When will it be null, and what does that mean?
https://codereview.chromium.org/12252016/diff/16001/content/browser/web_conte... File content/browser/web_contents/web_contents_view_gtk.cc (right): https://codereview.chromium.org/12252016/diff/16001/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.cc:231: // receive any events. On 2013/02/27 01:42:35, creis wrote: > This comment doesn't make sense to me. First, creating swapped out RVHs for the > opener chain is only one of multiple ways we could get here. (The BrowserPlugin > also does this, which was the cause of the bug.) Also, WebUI is unrelated to > whether it will receive events. The only reason WebUI was mentioned in > RenderViewCreated is that there was WebUI-specific logic below the return > statement. > > In other words, most of this is a red herring for why we want this logic. > > The real reason is that we don't want to change any of this class's state (e.g., > drag_dest_) for RenderViewHosts that are currently swapped out, and thus not > visible. It will be important to update this state when we swap in a new > RenderViewHost, which happens below in RenderViewSwappedIn. > > Can you put something like that just above the IsRenderView() conditional? I > want it to be clear to future developers that any local state should not be > updated for swapped out RVHs, and drag_dest_ is just one example. Done. https://codereview.chromium.org/12252016/diff/16001/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.cc:275: RenderViewHost* old_host) { On 2013/02/27 01:42:35, creis wrote: > Shouldn't we update drag_dest_ every time we get here, regardless of whether > there's a current one or what the old host had? If we've just changed our > RenderViewHost, then it seems like we necessarily need to change our drag_dest_. > > If that's the case, then we don't need old_host in the API. No, if we update the drag_dest_ every time here, just simple navigation breaks drag and drop. Maybe you have a better idea of why this is the case than I would? https://codereview.chromium.org/12252016/diff/16001/content/port/browser/web_... File content/port/browser/web_contents_view_port.h (right): https://codereview.chromium.org/12252016/diff/16001/content/port/browser/web_... content/port/browser/web_contents_view_port.h:45: // swapped in. |old_host| may be null. On 2013/02/27 01:42:35, creis wrote: > When will it be null, and what does that mean? Well right now, never. I just didn't want to force the RenderViewSwappedIn call to include an old host, as I'm not sure that is necessarily going to be true going forward.
https://codereview.chromium.org/12252016/diff/16001/content/browser/web_conte... File content/browser/web_contents/web_contents_view_gtk.cc (right): https://codereview.chromium.org/12252016/diff/16001/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.cc:275: RenderViewHost* old_host) { On 2013/02/27 14:57:13, mthiesse wrote: > On 2013/02/27 01:42:35, creis wrote: > > Shouldn't we update drag_dest_ every time we get here, regardless of whether > > there's a current one or what the old host had? If we've just changed our > > RenderViewHost, then it seems like we necessarily need to change our > drag_dest_. > > > > If that's the case, then we don't need old_host in the API. > > No, if we update the drag_dest_ every time here, just simple navigation breaks > drag and drop. Maybe you have a better idea of why this is the case than I > would? This sounds like an issue with drag n drop, which I don't have any familiarity with. Can you find someone who knows that feature who might be able to figure out what failure you're seeing? I'm pretty sure we should have to update this every time the RenderViewHost changes, assuming that GetNativeView() is different for each RenderViewHost. Otherwise drag_dest_ will have a stale native view, right? https://codereview.chromium.org/12252016/diff/16001/content/port/browser/web_... File content/port/browser/web_contents_view_port.h (right): https://codereview.chromium.org/12252016/diff/16001/content/port/browser/web_... content/port/browser/web_contents_view_port.h:45: // swapped in. |old_host| may be null. On 2013/02/27 14:57:13, mthiesse wrote: > On 2013/02/27 01:42:35, creis wrote: > > When will it be null, and what does that mean? > > Well right now, never. I just didn't want to force the RenderViewSwappedIn call > to include an old host, as I'm not sure that is necessarily going to be true > going forward. Let's avoid that case if we decide to keep old_host in the API. It's much better to provide clean values than make all callers check, so this should never be null. https://codereview.chromium.org/12252016/diff/16002/content/browser/web_conte... File content/browser/web_contents/web_contents_view_gtk.cc (right): https://codereview.chromium.org/12252016/diff/16002/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.cc:227: // because they will not be visible at this time. Great. Please add: It is important to update this state when we swap in a new RenderViewHost, which happens below in RenderViewSwappedIn.
Phew, found the issues (2 of them). The first issue is that scoped pointer reset created the new object before deleting the old object, so the gtk signal handlers created in the new WebDragDestGtk for the widget were immediately erased when the previous WebDragDestGtk was deleted. The second issue was that the cleanup of WebDragDestGtk was lazy and didn't remove all of the signals, so if swapped back in an RVH and re-connected the gtk signal handlers, the old ones would fire as well and segfault. PTAL
Nice debugging! This seems like the right approach to me. Are there ways to test drag n drop from a browsertest? Seems like it would be good to prevent a regression here if it's not too much effort. (I think RenderViewHostManagerTest.DisownOpener is a good start: it creates two tabs in different processes, each with a swapped out RVH. You might be able to test that drag works before the navigation to title1.html, again after the navigation, and again after going back.) https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... File content/browser/web_contents/web_contents_view_gtk.cc (right): https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.cc:305: if (drag_dest_.get() && content_view == drag_dest_->widget()) nit: I think this would be easier to read if you reversed the equality test: drag_dest_->widget() == content_view https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.cc:310: drag_dest_.reset(NULL); nit: reset() https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.cc:311: // Create the new drag_dest_; End comment with a period, not a semicolon. :) https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... File content/browser/web_contents/web_contents_view_gtk.h (right): https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.h:100: void UpdateDragDest(RenderViewHost* new_host); Please add a comment saying when this needs to be called (i.e., when a new RenderViewHost swap in). https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... File content/browser/web_contents/web_drag_dest_gtk.cc (right): https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_drag_dest_gtk.cc:64: handlers_.reset(new int[5]); Magic numbers aren't good. Better to declare a kNumHandlers constant (just above GetModifierFlags) and add a DCHECK_EQ(kNumHandlers, handlers_.size()) after you've put them all in. https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... File content/browser/web_contents/web_drag_dest_gtk.h (right): https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_drag_dest_gtk.h:95: // Stores Handler IDs for the gtk signal handlers. We have to cancel the the typo: the the https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_drag_dest_gtk.h:97: // we re-create the drag dest with the same same widget that we don't get so that if we later recreate... typo: same same "that we don't" doesn't sound right. I think you want to replace "that" with a comma.
Re: adding tests So as you probably know I'm in the process of getting drag and drop working in BrowserPlugin, which is how I found this issue in the first place. The tests I'm writing there should be sufficient to catch any regressions here, so I'll leave it to you to decide if the mostly duplicated effort of adding more drag and drop tests is worth it. I could always wait until the drag and drop CL is done and commit both of these together so that the tests go in at the same time as these changes if that's a concern. https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... File content/browser/web_contents/web_contents_view_gtk.cc (right): https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.cc:305: if (drag_dest_.get() && content_view == drag_dest_->widget()) On 2013/03/14 22:08:38, creis wrote: > nit: I think this would be easier to read if you reversed the equality test: > drag_dest_->widget() == content_view Done. https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.cc:310: drag_dest_.reset(NULL); On 2013/03/14 22:08:38, creis wrote: > nit: reset() Done. https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.cc:311: // Create the new drag_dest_; On 2013/03/14 22:08:38, creis wrote: > End comment with a period, not a semicolon. :) Haha oh boy was I ever not paying attention when writing these comments. https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... File content/browser/web_contents/web_contents_view_gtk.h (right): https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_contents_view_gtk.h:100: void UpdateDragDest(RenderViewHost* new_host); On 2013/03/14 22:08:38, creis wrote: > Please add a comment saying when this needs to be called (i.e., when a new > RenderViewHost swap in). Done. https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... File content/browser/web_contents/web_drag_dest_gtk.cc (right): https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_drag_dest_gtk.cc:64: handlers_.reset(new int[5]); On 2013/03/14 22:08:38, creis wrote: > Magic numbers aren't good. Better to declare a kNumHandlers constant (just > above GetModifierFlags) and add a DCHECK_EQ(kNumHandlers, handlers_.size()) > after you've put them all in. Done. https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... File content/browser/web_contents/web_drag_dest_gtk.h (right): https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_drag_dest_gtk.h:95: // Stores Handler IDs for the gtk signal handlers. We have to cancel the the On 2013/03/14 22:08:38, creis wrote: > typo: the the Done. https://codereview.chromium.org/12252016/diff/27001/content/browser/web_conte... content/browser/web_contents/web_drag_dest_gtk.h:97: // we re-create the drag dest with the same same widget that we don't get On 2013/03/14 22:08:38, creis wrote: > so that if we later recreate... > typo: same same > > "that we don't" doesn't sound right. I think you want to replace "that" with a > comma. Done.
On 2013/03/15 15:26:49, mthiesse wrote: > Re: adding tests > So as you probably know I'm in the process of getting drag and drop working in > BrowserPlugin, which is how I found this issue in the first place. The tests I'm > writing there should be sufficient to catch any regressions here, so I'll leave > it to you to decide if the mostly duplicated effort of adding more drag and drop > tests is worth it. > I could always wait until the drag and drop CL is done and commit both of these > together so that the tests go in at the same time as these changes if that's a > concern. Ah, that test will be sufficient. Thanks! LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mthiesse@chromium.org/12252016/43001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mthiesse@chromium.org/12252016/57001
Message was sent while issue was closed.
Change committed as 188559 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
