Persist plugins over reattach.

Persist plugins over reattach.

Currently we have logic to allow plugins to opt-in to persisting
when detached. This CL updates the logic to always persist when
we are doing a reattach. This allows us to remember the state of
things like Flash files as we reattach the DOM nodes.

The original persist CL (r196024) was reverted because it caused timeouts for:
  Reason: breaks the following browser_tests:
    WebViewTest.EmbedderVisibilityChanged,
    WebViewTest.GuestVisibilityChanged,
    WebViewTest.Shim_TestResizeWebviewWithDisplayNoneResizesContent

  and the following extensions_browsertests:
    WebViewAPITest.EmbedderVisibilityChanged
    WebViewAPITest.GuestVisibilityChanged

This Cl removes the call to widget->dispose() in the HTMLPluginElement::attach
call which fixes this issue.

BUG=370459
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201401

[dsinclair, 2015-08-17 16:56:35]

dsinclair@chromium.org changed reviewers:
+ esprehn@chromium.org

[dsinclair, 2015-08-17 16:56:36]

PTAL.

The first patchset is the original CL, the second patchset is the change from
the original CL.

I removed the call to widget->dispose in the ::attach handler. Reading through
the ::detach handler, we would not have called this call. With the dispose call
in there we do not receive the correct notifications on the webview side about
the content being hidden which causes the browser tests to hang.

[dsinclair, 2015-08-18 13:38:50]

dsinclair@chromium.org changed reviewers:
+ eae@chromium.org, leviw@chromium.org

[eae, 2015-08-18 16:05:41]

LGTM

[dsinclair, 2015-08-18 16:16:18]

esprehn@ for Source/web OWNERS

[esprehn, 2015-08-18 16:29:20]

This will leak the plugin if we do a lazyReattach and then you remove the
plugin. You need that same clean up logic in ::removedFrom. You can write a test
for that by putting the plugin in a shadow root, offsetTop, then adding another
shadow root to the host, then remove the host.

PerformingReattach is kind ofna foot gun, need to think of ways to keep bugs
like this from creeping in.

[dsinclair, 2015-08-19 14:47:15]

On 2015/08/18 16:29:20, esprehn wrote:
> This will leak the plugin if we do a lazyReattach and then you remove the
> plugin. You need that same clean up logic in ::removedFrom. You can write a
test
> for that by putting the plugin in a shadow root, offsetTop, then adding
another
> shadow root to the host, then remove the host.
> 
> PerformingReattach is kind ofna foot gun, need to think of ways to keep bugs
> like this from creeping in.


I've added the ::removedFrom override. For the test, am I trying to test that it
doesn't leak? I'm not quite sure how to write the test to make sure the loaded
plugin didn't leak out of the HTMLPlugInElement.

[esprehn, 2015-08-19 23:47:52]

https://codereview.chromium.org/1284403003/diff/40001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/1284403003/diff/40001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:166: if (!layoutObject()) {
I think this wants || useFallbackContent()

https://codereview.chromium.org/1284403003/diff/40001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:170: RefPtrWillBeRawPtr<Widget> widget =
m_persistedPluginWidget;
Why are you saving the plugin in a refptr here? That doesn't make sense, it just
keeps the plugin alive slightly longer than the call to
setPersistedPluginWidget, but still dead after this block.

https://codereview.chromium.org/1284403003/diff/40001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:176: if (useFallbackContent())
This isn't what the old code did, if we have a LayoutObject, but we're supposed
to use fallback content you're not going to run the clean up code above.

https://codereview.chromium.org/1284403003/diff/40001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:209: RefPtrWillBeRawPtr<Widget> widget =
m_persistedPluginWidget;
ditto, this RefPtr doesn't make sense.

Also now that I think about this I suspect you're making script run inside here
which is not safe. We do:

    HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates;

to avoid the script running in updateStyle, but maybe the RELEASE_ASSERT for
ScriptForbiddenScope::isScriptForbidden will save us from the security bugs
these days? I guess we should try it and find out.

[dsinclair, 2015-08-20 14:31:37]

https://codereview.chromium.org/1284403003/diff/40001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/1284403003/diff/40001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:166: if (!layoutObject()) {
On 2015/08/19 23:47:52, esprehn wrote:
> I think this wants || useFallbackContent()

Done.

https://codereview.chromium.org/1284403003/diff/40001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:170: RefPtrWillBeRawPtr<Widget> widget =
m_persistedPluginWidget;
On 2015/08/19 23:47:52, esprehn wrote:
> Why are you saving the plugin in a refptr here? That doesn't make sense, it
just
> keeps the plugin alive slightly longer than the call to
> setPersistedPluginWidget, but still dead after this block.

Done.

https://codereview.chromium.org/1284403003/diff/40001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:176: if (useFallbackContent())
On 2015/08/19 23:47:52, esprehn wrote:
> This isn't what the old code did, if we have a LayoutObject, but we're
supposed
> to use fallback content you're not going to run the clean up code above.

Done.

https://codereview.chromium.org/1284403003/diff/40001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:209: RefPtrWillBeRawPtr<Widget> widget =
m_persistedPluginWidget;
On 2015/08/19 23:47:52, esprehn wrote:
> ditto, this RefPtr doesn't make sense.
> 
> Also now that I think about this I suspect you're making script run inside
here
> which is not safe. We do:
> 
>     HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates;
> 
> to avoid the script running in updateStyle, but maybe the RELEASE_ASSERT for
> ScriptForbiddenScope::isScriptForbidden will save us from the security bugs
> these days? I guess we should try it and find out.


Done. Added in the suspend scope to be on the safe side.

[dsinclair, 2015-08-25 15:15:39]

ping.

[esprehn, 2015-08-28 12:54:57]

The CQ bit was checked by esprehn@chromium.org

[esprehn, 2015-08-28 12:54:58]

lgtm

[esprehn, 2015-08-28 12:54:58]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1284403003/#ps80001
(title: "Rebase to master")

[commit-bot: I haz the power, 2015-08-28 12:55:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1284403003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1284403003/80001

[commit-bot: I haz the power, 2015-08-28 14:04:40]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201401

[philipj_slow, 2015-08-31 14:23:31]

Sweet! This should unblock
https://code.google.com/p/chromium/issues/detail?id=240576, do you intend to
look reviving that effort at that as well?

[dsinclair, 2015-08-31 14:29:45]

On 2015/08/31 14:23:31, philipj wrote:
> Sweet! This should unblock
> https://code.google.com/p/chromium/issues/detail?id=240576, do you intend to
> look reviving that effort at that as well?

Yes. I have an old CL https://codereview.chromium.org/1139033006/ that I'm going
to try to update. But, I want to let this one sit for a week or so to see if any
security issues or other problems pop up again.