Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1473913003:
Restore original WebView SurfaceId after showing interstitial. (Closed)
|
Created:
5 years ago by wjmaclean Modified:
5 years ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRestore original WebView SurfaceId after showing interstitial.
When interstitials are shown using InterstitialPage with a WebView
guest, the interstitial creates a new RenderWidgetHostViewGuest but uses
it with the existing BrowserPlugin in the embedder's renderer. The new
RenderWidgetHostViewGuest creates a new surface and instructs the
BrowserPlugin to use it, but when the interstitial goes away,
RenderWidgetHostViewGuest::Show() currently fails to restore the
original surface, causing the interstitial's content to continue to
display in place of the desired content.
Resizing and other operations force creation of a new surface, but
otherwise we need to restore the original; this CL modifies Show() to
do this.
BUG=407048
Committed: https://crrev.com/890a88b9e6d266f3e422f335239f85a9aa2ded42
Cr-Commit-Position: refs/heads/master@{#365024}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Create new sequence when re-showing a surface for WebView. #Patch Set 3 : Add clarifying comment. #
Depends on Patchset: Messages
Total messages: 28 (12 generated)
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Description was changed from ========== Restore original WebView SurfaceId after showing interstitial. When interstitials are shown using InterstitialPage with a WebView guest, the interstitial creates a new RenderWidgetHostViewGuest but uses it with the existing BrowserPlugin in the embedder's renderer. The new RenderWidgetHostViewGuest creates a new surface and instructs the BrowserPlugin to use it, but when the interstitial goes away, RenderWidgetHostViewGuest::Show() currently fails to restore the original surface, causing the interstitial's content to continue to display in place of the desired content. Resizing and other operations force creation of a new surface, but otherwise we need to restore the original; this CL modifies Show() to do this. BUG=407048 ========== to ========== Restore original WebView SurfaceId after showing interstitial. When interstitials are shown using InterstitialPage with a WebView guest, the interstitial creates a new RenderWidgetHostViewGuest but uses it with the existing BrowserPlugin in the embedder's renderer. The new RenderWidgetHostViewGuest creates a new surface and instructs the BrowserPlugin to use it, but when the interstitial goes away, RenderWidgetHostViewGuest::Show() currently fails to restore the original surface, causing the interstitial's content to continue to display in place of the desired content. Resizing and other operations force creation of a new surface, but otherwise we need to restore the original; this CL modifies Show() to do this. BUG=407048 ==========
wjmaclean@chromium.org changed reviewers: + kenrb@chromium.org
kenrb@ - Could you please do an initial review of this?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473913003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473913003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1473913003/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1473913003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_widget_host_view_guest.cc:103: surface_sequence_); I wonder if it might be safer to destroy any existing Surface here. The SurfaceManager gets rid of Surfaces that are not being used anymore, because all of their dependencies have been satisfied. The purpose of passing the SurfaceSequence to SetChildFrameSurface is actually because OnSwapCompositorFrame creates a temporary dependency on the new Surface, protecting it from the SurfaceManager's garbage collection. Then the embedding renderer satisfies that dependency after it has created its own (see BrowserPluginGuest::OnSatisfySequence and OnRequireSequence). When the Surface gets replaced in the embedder with the interstitial's Surface, I would worry that the original might get destroyed because it no longer has any destruction dependencies and it becomes unreachable in the surface tree. You might want to check with jbauman@ on this, in any case.
wjmaclean@chromium.org changed reviewers: + jbauman@chromium.org
jbauman@ - do you have any thoughts on kenrb@'s concerns below? When I decided to keep the existing surface, it was partly on account of your comments at: https://code.google.com/p/chromium/issues/detail?id=550217#c10 where you say "Maybe a better fix for the original problem is to avoid destroying the Surface if the guest is detached, but send the existing SurfaceId to the new embedding renderer when it's re-attached." This case seems similar, though kenrb@'s concerns about the surface being destroyed elsewhere worry me. https://codereview.chromium.org/1473913003/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/1473913003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_widget_host_view_guest.cc:103: surface_sequence_); On 2015/11/25 21:44:02, kenrb wrote: > I wonder if it might be safer to destroy any existing Surface here. The > SurfaceManager gets rid of Surfaces that are not being used anymore, because all > of their dependencies have been satisfied. The purpose of passing the > SurfaceSequence to SetChildFrameSurface is actually because > OnSwapCompositorFrame creates a temporary dependency on the new Surface, > protecting it from the SurfaceManager's garbage collection. Then the embedding > renderer satisfies that dependency after it has created its own (see > BrowserPluginGuest::OnSatisfySequence and OnRequireSequence). When the Surface > gets replaced in the embedder with the interstitial's Surface, I would worry > that the original might get destroyed because it no longer has any destruction > dependencies and it becomes unreachable in the surface tree. > > You might want to check with jbauman@ on this, in any case. Acknowledged.
On 2015/11/25 21:44:03, kenrb wrote: > https://codereview.chromium.org/1473913003/diff/1/content/browser/frame_host/... > File content/browser/frame_host/render_widget_host_view_guest.cc (right): > > https://codereview.chromium.org/1473913003/diff/1/content/browser/frame_host/... > content/browser/frame_host/render_widget_host_view_guest.cc:103: > surface_sequence_); > I wonder if it might be safer to destroy any existing Surface here. The > SurfaceManager gets rid of Surfaces that are not being used anymore, because all > of their dependencies have been satisfied. The purpose of passing the > SurfaceSequence to SetChildFrameSurface is actually because > OnSwapCompositorFrame creates a temporary dependency on the new Surface, > protecting it from the SurfaceManager's garbage collection. Then the embedding > renderer satisfies that dependency after it has created its own (see > BrowserPluginGuest::OnSatisfySequence and OnRequireSequence). When the Surface > gets replaced in the embedder with the interstitial's Surface, I would worry > that the original might get destroyed because it no longer has any destruction > dependencies and it becomes unreachable in the surface tree. > > You might want to check with jbauman@ on this, in any case. You should create a new SurfaceSequence every time you pass the SurfaceId to the renderer. The first time a sequence passed to the renderer, that renderer will satisfy it, so the sequence doesn't help the second time. The SurfaceFactory that created it also holds a reference to it, so you don't need to worry about it being destroyed until you call surface_factory_->Destroy(surface_id_) on it.
The CQ bit was checked by wjmaclean@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/1473913003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473913003/20001
Thanks jbauman@, I hadn't realised that the sequence couldn't be reused - creating a new sequence each time actually makes the code simpler. kenrb@, jbauman@ - ptal?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm
wjmaclean@chromium.org changed reviewers: + creis@chromium.org
creis@ - take a look at a small CL please?
I don't really follow this at all, but I'll defer to jbauman and kenrb and give my rubber stamp LGTM. (Would it help to add a comment? It's not clear that this case is about interstitials, though I'm not sure whether the idea applies more generally.)
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, creis@chromium.org, jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/1473913003/#ps40001 (title: "Add clarifying comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473913003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473913003/40001
Message was sent while issue was closed.
Description was changed from ========== Restore original WebView SurfaceId after showing interstitial. When interstitials are shown using InterstitialPage with a WebView guest, the interstitial creates a new RenderWidgetHostViewGuest but uses it with the existing BrowserPlugin in the embedder's renderer. The new RenderWidgetHostViewGuest creates a new surface and instructs the BrowserPlugin to use it, but when the interstitial goes away, RenderWidgetHostViewGuest::Show() currently fails to restore the original surface, causing the interstitial's content to continue to display in place of the desired content. Resizing and other operations force creation of a new surface, but otherwise we need to restore the original; this CL modifies Show() to do this. BUG=407048 ========== to ========== Restore original WebView SurfaceId after showing interstitial. When interstitials are shown using InterstitialPage with a WebView guest, the interstitial creates a new RenderWidgetHostViewGuest but uses it with the existing BrowserPlugin in the embedder's renderer. The new RenderWidgetHostViewGuest creates a new surface and instructs the BrowserPlugin to use it, but when the interstitial goes away, RenderWidgetHostViewGuest::Show() currently fails to restore the original surface, causing the interstitial's content to continue to display in place of the desired content. Resizing and other operations force creation of a new surface, but otherwise we need to restore the original; this CL modifies Show() to do this. BUG=407048 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Restore original WebView SurfaceId after showing interstitial. When interstitials are shown using InterstitialPage with a WebView guest, the interstitial creates a new RenderWidgetHostViewGuest but uses it with the existing BrowserPlugin in the embedder's renderer. The new RenderWidgetHostViewGuest creates a new surface and instructs the BrowserPlugin to use it, but when the interstitial goes away, RenderWidgetHostViewGuest::Show() currently fails to restore the original surface, causing the interstitial's content to continue to display in place of the desired content. Resizing and other operations force creation of a new surface, but otherwise we need to restore the original; this CL modifies Show() to do this. BUG=407048 ========== to ========== Restore original WebView SurfaceId after showing interstitial. When interstitials are shown using InterstitialPage with a WebView guest, the interstitial creates a new RenderWidgetHostViewGuest but uses it with the existing BrowserPlugin in the embedder's renderer. The new RenderWidgetHostViewGuest creates a new surface and instructs the BrowserPlugin to use it, but when the interstitial goes away, RenderWidgetHostViewGuest::Show() currently fails to restore the original surface, causing the interstitial's content to continue to display in place of the desired content. Resizing and other operations force creation of a new surface, but otherwise we need to restore the original; this CL modifies Show() to do this. BUG=407048 Committed: https://crrev.com/890a88b9e6d266f3e422f335239f85a9aa2ded42 Cr-Commit-Position: refs/heads/master@{#365024} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/890a88b9e6d266f3e422f335239f85a9aa2ded42 Cr-Commit-Position: refs/heads/master@{#365024} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
