Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1652093002:
Support reviving a disposed plugin container. (Closed)
DescriptionSupport reviving a disposed plugin container.
When a plugin element updates its 'persisted' renderless widget (a plugin
container most likely), we notify the previous one kept as having been
detached and disposed of. The plugin container must then promptly clear
the reference to its external WebPlugin (and others), for safety.
It is however possible for the embedder to still keep references to that
plugin container and afterwards revive it by assigning it a replacement
plugin. Support such revivification.
R=haraken
BUG=582811
Committed: https://crrev.com/1e3f5cea12f2e519f5f353fffa91e640aa758bfd
Cr-Commit-Position: refs/heads/master@{#372907}
Patch Set 1 #Patch Set 2 : tidy event listener removal #
Total comments: 2
Patch Set 3 : rename m_inDispose to m_isDisposed #
Messages
Total messages: 22 (10 generated)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1652093002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1652093002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Support reviving a plugin container that was marked as disposed. When a plugin element updates its 'persisted' renderless widget (a plugin container most likely), we mark the previous one kept as having been disposed of. It is possible for the embedder to keep references to that plugin container and revive it by assigning it a replacement plugin. Support such revivification. R= BUG=582811 ========== to ========== Support reviving a disposed plugin container. When a plugin element updates its 'persisted' renderless widget (a plugin container most likely), we notify the previous one kept as having been detached and disposed of. The plugin container must then promptly clear the reference to its external WebPlugin (and others), for safety. It is however possible for the embedder to still keep references to that plugin container and afterwards revive it by assigning it a replacement plugin. Support such revivification. R= BUG=582811 ==========
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look.
haraken@chromium.org changed reviewers: + dcheng@chromium.org, haraken@chromium.org
> It is however possible for the embedder to still keep references to that > plugin container and afterwards revive it by assigning it a replacement > plugin. Support such revivification. Is this a valid scenario? GraphicsLayer::unregisterContentsLayer(m_webLayer) has already been called (when the WebPluginContainerImpl was disposed). Is it valid (does it work) to revive the already disposed WebPluginContainerImpl? https://codereview.chromium.org/1652093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/1652093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:706: m_inDispose = true; I'd rename m_inDispose to m_isDisposed.
On 2016/02/02 01:33:52, haraken wrote: > > It is however possible for the embedder to still keep references to that > > plugin container and afterwards revive it by assigning it a replacement > > plugin. Support such revivification. > > Is this a valid scenario? > > GraphicsLayer::unregisterContentsLayer(m_webLayer) has already been called (when > the WebPluginContainerImpl was disposed). Is it valid (does it work) to revive > the already disposed WebPluginContainerImpl? > The embedder code handling 'web layer' for a plugin, updates it with the current upon the plugin becoming shown. If it attempts to replace a plugin with another, like is the case here, I expect it to set that layer as part of that. The container that ended up being disposed, was rendererless widgets in any case, which would have lost its web layer already.
On 2016/02/02 06:20:05, sof wrote: > On 2016/02/02 01:33:52, haraken wrote: > > > It is however possible for the embedder to still keep references to that > > > plugin container and afterwards revive it by assigning it a replacement > > > plugin. Support such revivification. > > > > Is this a valid scenario? > > > > GraphicsLayer::unregisterContentsLayer(m_webLayer) has already been called > (when > > the WebPluginContainerImpl was disposed). Is it valid (does it work) to revive > > the already disposed WebPluginContainerImpl? > > > > The embedder code handling 'web layer' for a plugin, updates it with the current > upon the plugin becoming shown. > > If it attempts to replace a plugin with another, like is the case here, I expect > it to set that layer as part of that. The container that ended up being > disposed, was rendererless widgets in any case, which would have lost its web > layer already. Thanks for the clarification; LGTM.
Description was changed from ========== Support reviving a disposed plugin container. When a plugin element updates its 'persisted' renderless widget (a plugin container most likely), we notify the previous one kept as having been detached and disposed of. The plugin container must then promptly clear the reference to its external WebPlugin (and others), for safety. It is however possible for the embedder to still keep references to that plugin container and afterwards revive it by assigning it a replacement plugin. Support such revivification. R= BUG=582811 ========== to ========== Support reviving a disposed plugin container. When a plugin element updates its 'persisted' renderless widget (a plugin container most likely), we notify the previous one kept as having been detached and disposed of. The plugin container must then promptly clear the reference to its external WebPlugin (and others), for safety. It is however possible for the embedder to still keep references to that plugin container and afterwards revive it by assigning it a replacement plugin. Support such revivification. R=haraken BUG=582811 ==========
https://codereview.chromium.org/1652093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/1652093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:706: m_inDispose = true; On 2016/02/02 01:33:51, haraken wrote: > > I'd rename m_inDispose to m_isDisposed. Done.
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1652093002/#ps40001 (title: "rename m_inDispose to m_isDisposed")
On 2016/02/02 at 06:34:01, sigbjornf wrote: > https://codereview.chromium.org/1652093002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): > > https://codereview.chromium.org/1652093002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:706: m_inDispose = true; > On 2016/02/02 01:33:51, haraken wrote: > > > > I'd rename m_inDispose to m_isDisposed. > > Done. Any chance of testing this somehow? We have a PPAPI test plugin now, is there some way we could use that?
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1652093002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1652093002/40001
On 2016/02/02 06:36:16, dcheng wrote: > On 2016/02/02 at 06:34:01, sigbjornf wrote: > > > https://codereview.chromium.org/1652093002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): > > > > > https://codereview.chromium.org/1652093002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:706: m_inDispose = > true; > > On 2016/02/02 01:33:51, haraken wrote: > > > > > > I'd rename m_inDispose to m_isDisposed. > > > > Done. > > Any chance of testing this somehow? We have a PPAPI test plugin now, is there > some way we could use that? I'll look into doing that & followup.
Message was sent while issue was closed.
Description was changed from ========== Support reviving a disposed plugin container. When a plugin element updates its 'persisted' renderless widget (a plugin container most likely), we notify the previous one kept as having been detached and disposed of. The plugin container must then promptly clear the reference to its external WebPlugin (and others), for safety. It is however possible for the embedder to still keep references to that plugin container and afterwards revive it by assigning it a replacement plugin. Support such revivification. R=haraken BUG=582811 ========== to ========== Support reviving a disposed plugin container. When a plugin element updates its 'persisted' renderless widget (a plugin container most likely), we notify the previous one kept as having been detached and disposed of. The plugin container must then promptly clear the reference to its external WebPlugin (and others), for safety. It is however possible for the embedder to still keep references to that plugin container and afterwards revive it by assigning it a replacement plugin. Support such revivification. R=haraken BUG=582811 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Support reviving a disposed plugin container. When a plugin element updates its 'persisted' renderless widget (a plugin container most likely), we notify the previous one kept as having been detached and disposed of. The plugin container must then promptly clear the reference to its external WebPlugin (and others), for safety. It is however possible for the embedder to still keep references to that plugin container and afterwards revive it by assigning it a replacement plugin. Support such revivification. R=haraken BUG=582811 ========== to ========== Support reviving a disposed plugin container. When a plugin element updates its 'persisted' renderless widget (a plugin container most likely), we notify the previous one kept as having been detached and disposed of. The plugin container must then promptly clear the reference to its external WebPlugin (and others), for safety. It is however possible for the embedder to still keep references to that plugin container and afterwards revive it by assigning it a replacement plugin. Support such revivification. R=haraken BUG=582811 Committed: https://crrev.com/1e3f5cea12f2e519f5f353fffa91e640aa758bfd Cr-Commit-Position: refs/heads/master@{#372907} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1e3f5cea12f2e519f5f353fffa91e640aa758bfd Cr-Commit-Position: refs/heads/master@{#372907} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
