Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
348623005:
DesktopVideoCaptureMachine: do not cache window layer pointer. (Closed)
|
Created:
6 years, 6 months ago by hshi1 Modified:
6 years, 6 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, jam, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionDesktopVideoCaptureMachine: do not cache window layer pointer.
The aura window's layer pointer is subject to change due to layer recreation,
so the cached pointer |desktop_layer_| may be invalid.
Always use aura::Window::layer() accessor instead.
BUG=385751
TEST=start desktop sharing and switch between windowed/immersive mode.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278695
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comments. #
Total comments: 7
Patch Set 3 : DCHECK that window pointer matches #Messages
Total messages: 12 (0 generated)
PTAL. For testing I was able to get a consistent local repro of the bug first, then verified that the crash is fixed after this patch.
https://codereview.chromium.org/348623005/diff/1/content/browser/media/captur... File content/browser/media/capture/desktop_capture_device_aura.cc (right): https://codereview.chromium.org/348623005/diff/1/content/browser/media/captur... content/browser/media/capture/desktop_capture_device_aura.cc:269: if (!layer) Why are there so many checks for layer when Start() returns false if there's no layer?
https://codereview.chromium.org/348623005/diff/1/content/browser/media/captur... File content/browser/media/capture/desktop_capture_device_aura.cc (right): https://codereview.chromium.org/348623005/diff/1/content/browser/media/captur... content/browser/media/capture/desktop_capture_device_aura.cc:237: compositor->RemoveObserver(this); If the layer can change, how do you know the compositor is the same as the one in Start() ? (well, even if it doesn't change, that may not even be true). How about observing the window and check when it's added/removed from the tree, and attach/detach from the compositor (that you get from the WindowTreeHost via desktop_window_->GetHost()). This is what we do in RenderWidgetHostViewAura.
PTAL patch set #2 https://codereview.chromium.org/348623005/diff/1/content/browser/media/captur... File content/browser/media/capture/desktop_capture_device_aura.cc (right): https://codereview.chromium.org/348623005/diff/1/content/browser/media/captur... content/browser/media/capture/desktop_capture_device_aura.cc:237: compositor->RemoveObserver(this); On 2014/06/19 21:59:19, piman wrote: > If the layer can change, how do you know the compositor is the same as the one > in Start() ? > > (well, even if it doesn't change, that may not even be true). > > How about observing the window and check when it's added/removed from the tree, > and attach/detach from the compositor (that you get from the WindowTreeHost via > desktop_window_->GetHost()). > > This is what we do in RenderWidgetHostViewAura. Done. https://codereview.chromium.org/348623005/diff/1/content/browser/media/captur... content/browser/media/capture/desktop_capture_device_aura.cc:269: if (!layer) On 2014/06/19 21:43:28, danakj wrote: > Why are there so many checks for layer when Start() returns false if there's no > layer? Done.
Thanks, one more Q https://codereview.chromium.org/348623005/diff/20001/content/browser/media/ca... File content/browser/media/capture/desktop_capture_device_aura.cc (right): https://codereview.chromium.org/348623005/diff/20001/content/browser/media/ca... content/browser/media/capture/desktop_capture_device_aura.cc:258: // Do not capture if the desktop window is already destroyed. Can this happen now that we watch for OnWindowRemovingFromRootWindow? (not sure if it can, but wondering)
LGTM+nits/suggestions. https://codereview.chromium.org/348623005/diff/20001/content/browser/media/ca... File content/browser/media/capture/desktop_capture_device_aura.cc (right): https://codereview.chromium.org/348623005/diff/20001/content/browser/media/ca... content/browser/media/capture/desktop_capture_device_aura.cc:258: // Do not capture if the desktop window is already destroyed. On 2014/06/19 22:22:05, danakj wrote: > Can this happen now that we watch for OnWindowRemovingFromRootWindow? (not sure > if it can, but wondering) I think currently it does because OnCompositingEnded posts a task and so there could still be such a task in flight when OnWindowRemovingFromRootWindow is called. However I don't think OnCompositingEnded needs to post a task (UI->UI), if that is fixed then I think it'd be unnecessary. https://codereview.chromium.org/348623005/diff/20001/content/browser/media/ca... content/browser/media/capture/desktop_capture_device_aura.cc:493: if (window == desktop_window_) nit: you can DCHECK, since that's the only window you're observing. https://codereview.chromium.org/348623005/diff/20001/content/browser/media/ca... content/browser/media/capture/desktop_capture_device_aura.cc:500: if (window == desktop_window_) same here.
https://codereview.chromium.org/348623005/diff/20001/content/browser/media/ca... File content/browser/media/capture/desktop_capture_device_aura.cc (right): https://codereview.chromium.org/348623005/diff/20001/content/browser/media/ca... content/browser/media/capture/desktop_capture_device_aura.cc:258: // Do not capture if the desktop window is already destroyed. On 2014/06/19 23:11:48, piman wrote: > On 2014/06/19 22:22:05, danakj wrote: > > Can this happen now that we watch for OnWindowRemovingFromRootWindow? (not > sure > > if it can, but wondering) > > I think currently it does because OnCompositingEnded posts a task and so there > could still be such a task in flight when OnWindowRemovingFromRootWindow is > called. > > However I don't think OnCompositingEnded needs to post a task (UI->UI), if that > is fixed then I think it'd be unnecessary. Ah, then LGTM but it sounds like it'd be nice to fix :)
https://codereview.chromium.org/348623005/diff/20001/content/browser/media/ca... File content/browser/media/capture/desktop_capture_device_aura.cc (right): https://codereview.chromium.org/348623005/diff/20001/content/browser/media/ca... content/browser/media/capture/desktop_capture_device_aura.cc:493: if (window == desktop_window_) On 2014/06/19 23:11:49, piman wrote: > nit: you can DCHECK, since that's the only window you're observing. Done. https://codereview.chromium.org/348623005/diff/20001/content/browser/media/ca... content/browser/media/capture/desktop_capture_device_aura.cc:500: if (window == desktop_window_) On 2014/06/19 23:11:49, piman wrote: > same here. Done.
The CQ bit was checked by hshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/348623005/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Message was sent while issue was closed.
Change committed as 278695 |
