Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1514073002:
Oilpan: always limit persisted plugin disposal to PluginViews instances. (Closed)
DescriptionOilpan: always limit persisted plugin disposal to PluginViews instances.
With Oilpan enabled, the plugin element needs to synchronously inform
its 'persisted' plugin widget that it is slated for destruction and
call its dispose() method. This is needed so as to have that plugin
unregister in a timely fashion (without waiting on the next GC.)
This disposal step is only needed for PluginView widgets; it is indeed
harmful to call it for a FrameView widget should it be disposed while
being in the middle of performing a full layout.
R=dcheng
BUG=568383
Committed: https://crrev.com/b6ec75dd3ddc303d703afbc0fd3a3a9b12663d0f
Cr-Commit-Position: refs/heads/master@{#364690}
Patch Set 1 #
Total comments: 4
Patch Set 2 : add asserts #Messages
Total messages: 24 (11 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/1514073002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514073002/1
sigbjornf@opera.com changed reviewers: + dcheng@chromium.org, oilpan-reviews@chromium.org
please take a look. This is the minimal incremental change needed, i think. However, i'll separately investigate if we can let go of Widget::dispose and instead rely on eager finalization for WebPluginContainerImpl. I _believe_ we added this explicit dispose over Widgets before we had any ability to eagerly finalize (via prefinalizers or otherwise.) But there might be other problems which prevents this.
Description was changed from ========== Oilpan: always limit persisted plugin disposal to PluginViews instances. With Oilpan enabled, the plugin element needs to synchronously inform its 'persisted' plugin widget that it is slated for destruction and call its dispose() method. This is needed so as to have that plugin unregister in a timely fashion (without waiting on the next GC.) This disposal step is only needed for PluginView widgets; it is indeed harmful to call it for a FrameView widget should it be disposed while being in the middle of performing a full layout. R= BUG= ========== to ========== Oilpan: always limit persisted plugin disposal to PluginViews instances. With Oilpan enabled, the plugin element needs to synchronously inform its 'persisted' plugin widget that it is slated for destruction and call its dispose() method. This is needed so as to have that plugin unregister in a timely fashion (without waiting on the next GC.) This disposal step is only needed for PluginView widgets; it is indeed harmful to call it for a FrameView widget should it be disposed while being in the middle of performing a full layout. R= BUG=568383 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1514073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/1514073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:98: m_persistedPluginWidget->dispose(); Is the only other possible case here that m_persistedPluginWidget->isFrameView() is true? In that case, it's safe to skip it here because it gets cleared as part of detaching the actual frame, right>
https://codereview.chromium.org/1514073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/1514073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:98: m_persistedPluginWidget->dispose(); On 2015/12/11 07:47:09, dcheng wrote: > Is the only other possible case here that m_persistedPluginWidget->isFrameView() > is true? > > In that case, it's safe to skip it here because it gets cleared as part of > detaching the actual frame, right> Yes, I think it is safe to do the disposal unconditionally for this code path. I added the check to be consistent about only calling dispose() for PluginViews over persisted widgets.
It _looks like_ scaling back (but not removing completely) the use of widget disposal is possible w/Oilpan, https://codereview.chromium.org/1511413003/ has the (ongoing) experiment.
lgtm https://codereview.chromium.org/1514073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/1514073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:98: m_persistedPluginWidget->dispose(); On 2015/12/11 at 08:01:18, sof wrote: > On 2015/12/11 07:47:09, dcheng wrote: > > Is the only other possible case here that m_persistedPluginWidget->isFrameView() > > is true? > > > > In that case, it's safe to skip it here because it gets cleared as part of > > detaching the actual frame, right> > > Yes, I think it is safe to do the disposal unconditionally for this code path. I added the check to be consistent about only calling dispose() for PluginViews over persisted widgets. Let's add an else { ASSERT(m_persistedPluginWidget->isFrameView()) } then. https://codereview.chromium.org/1514073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:118: if (m_persistedPluginWidget && m_persistedPluginWidget->isPluginView()) { Ditto, let's split this so we can have an explicit ASSERT.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
thanks, asserts added in both places.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514073002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514073002/20001
Description was changed from ========== Oilpan: always limit persisted plugin disposal to PluginViews instances. With Oilpan enabled, the plugin element needs to synchronously inform its 'persisted' plugin widget that it is slated for destruction and call its dispose() method. This is needed so as to have that plugin unregister in a timely fashion (without waiting on the next GC.) This disposal step is only needed for PluginView widgets; it is indeed harmful to call it for a FrameView widget should it be disposed while being in the middle of performing a full layout. R= BUG=568383 ========== to ========== Oilpan: always limit persisted plugin disposal to PluginViews instances. With Oilpan enabled, the plugin element needs to synchronously inform its 'persisted' plugin widget that it is slated for destruction and call its dispose() method. This is needed so as to have that plugin unregister in a timely fashion (without waiting on the next GC.) This disposal step is only needed for PluginView widgets; it is indeed harmful to call it for a FrameView widget should it be disposed while being in the middle of performing a full layout. R=dcheng BUG=568383 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1514073002/#ps20001 (title: "add asserts")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514073002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514073002/20001
Message was sent while issue was closed.
Description was changed from ========== Oilpan: always limit persisted plugin disposal to PluginViews instances. With Oilpan enabled, the plugin element needs to synchronously inform its 'persisted' plugin widget that it is slated for destruction and call its dispose() method. This is needed so as to have that plugin unregister in a timely fashion (without waiting on the next GC.) This disposal step is only needed for PluginView widgets; it is indeed harmful to call it for a FrameView widget should it be disposed while being in the middle of performing a full layout. R=dcheng BUG=568383 ========== to ========== Oilpan: always limit persisted plugin disposal to PluginViews instances. With Oilpan enabled, the plugin element needs to synchronously inform its 'persisted' plugin widget that it is slated for destruction and call its dispose() method. This is needed so as to have that plugin unregister in a timely fashion (without waiting on the next GC.) This disposal step is only needed for PluginView widgets; it is indeed harmful to call it for a FrameView widget should it be disposed while being in the middle of performing a full layout. R=dcheng BUG=568383 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Oilpan: always limit persisted plugin disposal to PluginViews instances. With Oilpan enabled, the plugin element needs to synchronously inform its 'persisted' plugin widget that it is slated for destruction and call its dispose() method. This is needed so as to have that plugin unregister in a timely fashion (without waiting on the next GC.) This disposal step is only needed for PluginView widgets; it is indeed harmful to call it for a FrameView widget should it be disposed while being in the middle of performing a full layout. R=dcheng BUG=568383 ========== to ========== Oilpan: always limit persisted plugin disposal to PluginViews instances. With Oilpan enabled, the plugin element needs to synchronously inform its 'persisted' plugin widget that it is slated for destruction and call its dispose() method. This is needed so as to have that plugin unregister in a timely fashion (without waiting on the next GC.) This disposal step is only needed for PluginView widgets; it is indeed harmful to call it for a FrameView widget should it be disposed while being in the middle of performing a full layout. R=dcheng BUG=568383 Committed: https://crrev.com/b6ec75dd3ddc303d703afbc0fd3a3a9b12663d0f Cr-Commit-Position: refs/heads/master@{#364690} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b6ec75dd3ddc303d703afbc0fd3a3a9b12663d0f Cr-Commit-Position: refs/heads/master@{#364690} |
