[Reland #1] Don't create layout objects for children of display-none iframes.

[Reland #1] Don't create layout objects for children of display-none iframes.

The initial CL leaked DOM objects by calling
documentElement()->lazyReattachIfAttached() during Document::shutdown(). This
reland advances m_lifecycle to DocumentLifecycle::Stopping slightly earlier in
Document::shutdown(), and then only triggers
documentElement()->lazyReattachIfAttached() if the state is earlier than
DocumentLifecycle::Stopping.

Partial stack of the DOM leak:
  ...
  documentElement()->lazyReattachIfAttached();
  blink::Document::willChangeFrameOwnerProperties()\n                                                                                                                                                 
  blink::HTMLFrameOwnerElement::setWidget()\n                                
  blink::FrameView::dispose()\n                                              
  blink::Document::shutdown()\n                                              
  blink::FrameLoader::prepareForCommit()\n                                   
  blink::FrameLoader::commitProvisionalLoad()\n                              
  blink::DocumentLoader::commitIfReady()\n                                                           
  blink::DocumentLoader::processData()    

> Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
>   * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
>   * A local frame is "DisplayNone" if and only if it has no widget [which should
>     correspond to the actual display:none style].
>   * The DisplayNone property is propagated to remote frames via
>     FrameOwnerProperties.
>
> Spec discussion: https://github.com/whatwg/html/issues/1813

BUG=650433
Review-Url: https://codereview.chromium.org/2743053003
Cr-Commit-Position: refs/heads/master@{#460646}
Committed: https://chromium.googlesource.com/chromium/src/+/85bab14c835935e79a0b63f24b47f671840373de

[erikchen, 2017-03-10 17:38:56]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 17:39:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 17:50:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 17:50:35]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2017-03-10 22:21:35]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 22:22:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 23:46:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 23:46:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2017-03-11 00:46:33]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-11 00:47:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 02:07:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-11 02:07:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2017-03-13 21:24:44]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 21:25:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-03-13 21:31:23]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 21:32:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 23:20:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 23:20:21]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[erikchen, 2017-03-14 17:35:11]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 17:35:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 19:14:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 19:14:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2017-03-14 21:50:12]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 21:51:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-03-14 21:52:33]

Description was changed from

==========
[Reland #1] Don't create layout objects for children of display-none iframes.

> Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
>   * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
>   * A local frame is "DisplayNone" if and only if it has no widget [which
should
>     correspond to the actual display:none style].
>   * The DisplayNone property is propagated to remote frames via
>     FrameOwnerProperties.
>
> Spec discussion: https://github.com/whatwg/html/issues/1813

BUG=650433
==========

to

==========
[Reland #1] Don't create layout objects for children of display-none iframes.

The initial CL leaked DOM objects by calling
documentElement()->lazyReattachIfAttached() during Document::shutdown(). This
reland advances m_lifecycle to DocumentLifecycle::Stopping slightly earlier in
Document::shutdown(), and then only triggers
documentElement()->lazyReattachIfAttached() if the state is earlier than
DocumentLifecycle::Stopping.

> Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
>   * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
>   * A local frame is "DisplayNone" if and only if it has no widget [which
should
>     correspond to the actual display:none style].
>   * The DisplayNone property is propagated to remote frames via
>     FrameOwnerProperties.
>
> Spec discussion: https://github.com/whatwg/html/issues/1813

BUG=650433
==========

[commit-bot: I haz the power, 2017-03-14 23:46:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 23:46:28]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-03-14 23:54:39]

erikchen@chromium.org changed reviewers:
+ avi@chromium.org, dcheng@chromium.org, esprehn@chromium.org

[erikchen, 2017-03-14 23:54:41]

esprehn, dcheng, avi: PTAL

esprehn: Please review third_party/WebKit
avi: Please review content/
dcheng: Please review content/common/frame_messages.h

[dcheng, 2017-03-15 09:25:00]

On 2017/03/14 23:54:41, erikchen wrote:
> esprehn, dcheng, avi: PTAL
> 
> esprehn: Please review third_party/WebKit
> avi: Please review content/
> dcheng: Please review content/common/frame_messages.h

Any chance that before vs after patch sets can be uploaded, without all the
intermediate patch sets? That will make this easier to re-review

[erikchen, 2017-03-15 21:01:20]

On 2017/03/15 09:25:00, dcheng wrote:
> On 2017/03/14 23:54:41, erikchen wrote:
> > esprehn, dcheng, avi: PTAL
> > 
> > esprehn: Please review third_party/WebKit
> > avi: Please review content/
> > dcheng: Please review content/common/frame_messages.h
> 
> Any chance that before vs after patch sets can be uploaded, without all the
> intermediate patch sets? That will make this easier to re-review

Done. Please diff PS11 against PS10.

[dcheng, 2017-03-16 05:54:19]

The change LGTM, but it might be helpful to include the stack of how we end up
calling this function from Document::shutdown() in the CL description.

[erikchen, 2017-03-17 00:23:43]

Description was changed from

==========
[Reland #1] Don't create layout objects for children of display-none iframes.

The initial CL leaked DOM objects by calling
documentElement()->lazyReattachIfAttached() during Document::shutdown(). This
reland advances m_lifecycle to DocumentLifecycle::Stopping slightly earlier in
Document::shutdown(), and then only triggers
documentElement()->lazyReattachIfAttached() if the state is earlier than
DocumentLifecycle::Stopping.

> Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
>   * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
>   * A local frame is "DisplayNone" if and only if it has no widget [which
should
>     correspond to the actual display:none style].
>   * The DisplayNone property is propagated to remote frames via
>     FrameOwnerProperties.
>
> Spec discussion: https://github.com/whatwg/html/issues/1813

BUG=650433
==========

to

==========
[Reland #1] Don't create layout objects for children of display-none iframes.

The initial CL leaked DOM objects by calling
documentElement()->lazyReattachIfAttached() during Document::shutdown(). This
reland advances m_lifecycle to DocumentLifecycle::Stopping slightly earlier in
Document::shutdown(), and then only triggers
documentElement()->lazyReattachIfAttached() if the state is earlier than
DocumentLifecycle::Stopping.

Partial stack of the DOM leak:
  ...
  documentElement()->lazyReattachIfAttached();
  blink::Document::willChangeFrameOwnerProperties()\n                           
                                                                                
                                    
  blink::HTMLFrameOwnerElement::setWidget()\n                                
  blink::FrameView::dispose()\n                                              
  blink::Document::shutdown()\n                                              
  blink::FrameLoader::prepareForCommit()\n                                   
  blink::FrameLoader::commitProvisionalLoad()\n                              
  blink::DocumentLoader::commitIfReady()\n                                      
                    
  blink::DocumentLoader::processData()    

> Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
>   * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
>   * A local frame is "DisplayNone" if and only if it has no widget [which
should
>     correspond to the actual display:none style].
>   * The DisplayNone property is propagated to remote frames via
>     FrameOwnerProperties.
>
> Spec discussion: https://github.com/whatwg/html/issues/1813

BUG=650433
==========

[erikchen, 2017-03-17 00:23:55]

On 2017/03/16 05:54:19, dcheng wrote:
> The change LGTM, but it might be helpful to include the stack of how we end up
> calling this function from Document::shutdown() in the CL description.

Done.

[erikchen, 2017-03-21 00:59:56]

avi, esprehn: Ping.

[Avi (use Gerrit), 2017-03-21 01:21:43]

content lgtm

[esprehn, 2017-03-28 21:49:24]

lgtm w/ the check fixed.

https://codereview.chromium.org/2743053003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2743053003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4573: if (m_lifecycle.state() <
DocumentLifecycle::Stopping)
remove this, lazyReattachIfAttached() checks inActiveDocument() which already
checks this for you.

In general explicit checks on the lifecycle are frowned upon, isActive() is
better.

[erikchen, 2017-03-29 20:16:54]

https://codereview.chromium.org/2743053003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2743053003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4573: if (m_lifecycle.state() <
DocumentLifecycle::Stopping)
On 2017/03/28 21:49:24, esprehn wrote:
> remove this, lazyReattachIfAttached() checks inActiveDocument() which already
> checks this for you.
> 
> In general explicit checks on the lifecycle are frowned upon, isActive() is
> better.

Done.

[erikchen, 2017-03-29 21:16:20]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 21:16:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 23:23:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 23:23:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2017-03-30 01:30:50]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-03-30 01:30:50]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
esprehn@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2743053003/#ps280001
(title: "rebaseline.")

[commit-bot: I haz the power, 2017-03-30 01:31:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 03:37:26]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1490837450028660,
"parent_rev": "174eb44312202a5132de900553f7c302da0feef6", "commit_rev":
"85bab14c835935e79a0b63f24b47f671840373de"}

[commit-bot: I haz the power, 2017-03-30 03:38:16]

Description was changed from

==========
[Reland #1] Don't create layout objects for children of display-none iframes.

The initial CL leaked DOM objects by calling
documentElement()->lazyReattachIfAttached() during Document::shutdown(). This
reland advances m_lifecycle to DocumentLifecycle::Stopping slightly earlier in
Document::shutdown(), and then only triggers
documentElement()->lazyReattachIfAttached() if the state is earlier than
DocumentLifecycle::Stopping.

Partial stack of the DOM leak:
  ...
  documentElement()->lazyReattachIfAttached();
  blink::Document::willChangeFrameOwnerProperties()\n                           
                                                                                
                                    
  blink::HTMLFrameOwnerElement::setWidget()\n                                
  blink::FrameView::dispose()\n                                              
  blink::Document::shutdown()\n                                              
  blink::FrameLoader::prepareForCommit()\n                                   
  blink::FrameLoader::commitProvisionalLoad()\n                              
  blink::DocumentLoader::commitIfReady()\n                                      
                    
  blink::DocumentLoader::processData()    

> Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
>   * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
>   * A local frame is "DisplayNone" if and only if it has no widget [which
should
>     correspond to the actual display:none style].
>   * The DisplayNone property is propagated to remote frames via
>     FrameOwnerProperties.
>
> Spec discussion: https://github.com/whatwg/html/issues/1813

BUG=650433
==========

to

==========
[Reland #1] Don't create layout objects for children of display-none iframes.

The initial CL leaked DOM objects by calling
documentElement()->lazyReattachIfAttached() during Document::shutdown(). This
reland advances m_lifecycle to DocumentLifecycle::Stopping slightly earlier in
Document::shutdown(), and then only triggers
documentElement()->lazyReattachIfAttached() if the state is earlier than
DocumentLifecycle::Stopping.

Partial stack of the DOM leak:
  ...
  documentElement()->lazyReattachIfAttached();
  blink::Document::willChangeFrameOwnerProperties()\n                           
                                                                                
                                    
  blink::HTMLFrameOwnerElement::setWidget()\n                                
  blink::FrameView::dispose()\n                                              
  blink::Document::shutdown()\n                                              
  blink::FrameLoader::prepareForCommit()\n                                   
  blink::FrameLoader::commitProvisionalLoad()\n                              
  blink::DocumentLoader::commitIfReady()\n                                      
                    
  blink::DocumentLoader::processData()    

> Most of this CL is plumbing for FrameOwnerProperties. Interesting changes:
>   * LayoutView can no longer have children if its FrameOwner is "DisplayNone".
>   * A local frame is "DisplayNone" if and only if it has no widget [which
should
>     correspond to the actual display:none style].
>   * The DisplayNone property is propagated to remote frames via
>     FrameOwnerProperties.
>
> Spec discussion: https://github.com/whatwg/html/issues/1813

BUG=650433

Review-Url: https://codereview.chromium.org/2743053003
Cr-Commit-Position: refs/heads/master@{#460646}
Committed:
https://chromium.googlesource.com/chromium/src/+/85bab14c835935e79a0b63f24b47...
==========

[commit-bot: I haz the power, 2017-03-30 03:38:17]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/85bab14c835935e79a0b63f24b47...