Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(502)

Issue 357443005: Oilpan: make shutdown of plugin container objects work better. (Closed)

Created:
6 years, 6 months ago by sof
Modified:
6 years, 6 months ago
CC:
blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Oilpan: make shutdown of plugin container objects work better. The plugin container object/widget that's kept by the plugin element has a destructor that accesses its element's document in a couple of ways: - Optionally notifying that a touch event handler has been removed. - Tries to notify element (and its document) that compositing update is needed. - Indirectly clears out the script objects registered on the document frame's ScriptController when destroying the container's plugin object. With Oilpan, none of these can be done during finalization as the document cannot be assumed to be alive. To address, introduce an explicit detach() step for when the element is cleanly removed from the document (along with its container.) Otherwise, the plugin container is assumed to go away at the same time as the element (and the document.) The container's destructor will then only notify any off-heap objects. R=tkent@chromium.org,haraken@chromium.org BUG=340522 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176967

Patch Set 1 #

Patch Set 2 : Conditionally define detach() #

Total comments: 13

Patch Set 3 : Have WebPluginContainerImpl be a FrameDestructionObserver #

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -6 lines) Patch
M Source/core/html/HTMLPlugInElement.cpp View 1 2 3 chunks +5 lines, -1 line 0 comments Download
M Source/platform/Widget.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M Source/web/WebPluginContainerImpl.h View 1 2 4 chunks +9 lines, -1 line 0 comments Download
M Source/web/WebPluginContainerImpl.cpp View 1 2 5 chunks +33 lines, -4 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
sof
Please take a look/peek. Testing is a bit wonky this evening, so I don't have ...
6 years, 6 months ago (2014-06-24 21:24:35 UTC) #1
tkent
lgtm. This looks reasonable. https://codereview.chromium.org/357443005/diff/20001/Source/platform/Widget.h File Source/platform/Widget.h (right): https://codereview.chromium.org/357443005/diff/20001/Source/platform/Widget.h#newcode42 Source/platform/Widget.h:42: #if ENABLE(OILPAN) nit: You don't ...
6 years, 6 months ago (2014-06-25 05:05:40 UTC) #2
haraken
https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlugInElement.cpp File Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlugInElement.cpp#newcode206 Source/core/html/HTMLPlugInElement.cpp:206: plugin->detach(); Is it OK to detach the plugin that ...
6 years, 6 months ago (2014-06-25 05:40:16 UTC) #3
sof
https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginContainerImpl.cpp File Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginContainerImpl.cpp#newcode436 Source/web/WebPluginContainerImpl.cpp:436: m_scriptController->cleanupScriptObjectsForPlugin(this); On 2014/06/25 05:40:16, haraken wrote: > > Maybe ...
6 years, 6 months ago (2014-06-25 05:44:25 UTC) #4
haraken
https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginContainerImpl.cpp File Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/357443005/diff/20001/Source/web/WebPluginContainerImpl.cpp#newcode436 Source/web/WebPluginContainerImpl.cpp:436: m_scriptController->cleanupScriptObjectsForPlugin(this); On 2014/06/25 05:44:25, sof wrote: > On 2014/06/25 ...
6 years, 6 months ago (2014-06-25 05:54:24 UTC) #5
sof
https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlugInElement.cpp File Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlugInElement.cpp#newcode206 Source/core/html/HTMLPlugInElement.cpp:206: plugin->detach(); On 2014/06/25 05:40:16, haraken wrote: > > Is ...
6 years, 6 months ago (2014-06-25 07:09:32 UTC) #6
haraken
I looked into the code and understood what this CL is doing. I still think ...
6 years, 6 months ago (2014-06-25 09:20:50 UTC) #7
sof
https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlugInElement.cpp File Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlugInElement.cpp#newcode206 Source/core/html/HTMLPlugInElement.cpp:206: plugin->detach(); On 2014/06/25 07:09:32, sof wrote: > On 2014/06/25 ...
6 years, 6 months ago (2014-06-25 09:23:50 UTC) #8
haraken
https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlugInElement.cpp File Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/357443005/diff/20001/Source/core/html/HTMLPlugInElement.cpp#newcode206 Source/core/html/HTMLPlugInElement.cpp:206: plugin->detach(); On 2014/06/25 09:23:50, sof wrote: > On 2014/06/25 ...
6 years, 6 months ago (2014-06-25 09:31:06 UTC) #9
sof
On 2014/06/25 at 09:20:50, haraken wrote: > I looked into the code and understood what ...
6 years, 6 months ago (2014-06-25 10:31:24 UTC) #10
haraken
On 2014/06/25 10:31:24, sof wrote: > On 2014/06/25 at 09:20:50, haraken wrote: > > I ...
6 years, 6 months ago (2014-06-25 10:44:26 UTC) #11
tkent
On 2014/06/25 10:44:26, haraken wrote: > > In the interest of code simplicity - acceptable ...
6 years, 6 months ago (2014-06-26 03:53:43 UTC) #12
haraken
LGTM
6 years, 6 months ago (2014-06-26 04:10:54 UTC) #13
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 6 months ago (2014-06-26 05:20:11 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/357443005/40001
6 years, 6 months ago (2014-06-26 05:21:17 UTC) #15
commit-bot: I haz the power
6 years, 6 months ago (2014-06-26 06:59:52 UTC) #16
Message was sent while issue was closed.
Change committed as 176967

Powered by Google App Engine
This is Rietveld 408576698