Make sure pages that are closing but not yet closed are still suspended.

Make sure pages that are closing but not yet closed are still suspended.

BUG=671102
Committed: https://crrev.com/0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72
Cr-Commit-Position: refs/heads/master@{#440376}

[dcheng, 2016-12-15 11:20:18]

Description was changed from

==========
Make sure pages that are closing but not yet closed, are still suspended

BUG=671102
==========

to

==========
Make sure pages that are closing but not yet closed are still suspended.

BUG=671102
==========

[dcheng, 2016-12-15 11:22:40]

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

[commit-bot: I haz the power, 2016-12-15 11:22:54]

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, 2016-12-15 12:45:13]

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

[commit-bot: I haz the power, 2016-12-15 12:45:14]

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_...)

[dcheng, 2016-12-15 16:27:20]

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

[commit-bot: I haz the power, 2016-12-15 16:27:53]

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, 2016-12-15 18:34:08]

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

[commit-bot: I haz the power, 2016-12-15 18:34:09]

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_...)

[dcheng, 2016-12-15 19:07:51]

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

[commit-bot: I haz the power, 2016-12-15 19:08:13]

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

[dcheng, 2016-12-15 19:10:37]

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

[commit-bot: I haz the power, 2016-12-15 19:11:12]

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

[dcheng, 2016-12-15 19:11:37]

dcheng@chromium.org changed reviewers:
+ japhet@chromium.org

[dcheng, 2016-12-15 19:11:38]

A possible alternative is https://codereview.chromium.org/2575923002/, but
maintaining 3 sets seems more horrible.

https://codereview.chromium.org/2580703003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/2580703003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FrameTree.cpp:433: if (otherPage == page ||
otherPage->isClosing())
Another alternative is to check the main frame's window's closed attribute.

https://codereview.chromium.org/2580703003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/Page.cpp (right):

https://codereview.chromium.org/2580703003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/Page.cpp:157: // TODO(dcheng): Try to remove
this in a followup, it's not obviously needed.
Originally I wanted to remove this as the fix (since this can run JS). However,
I realized that this isn't sufficient, since we can just trigger a nested
message loop after window.close().

[commit-bot: I haz the power, 2016-12-15 21:30:40]

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

[commit-bot: I haz the power, 2016-12-15 21:30:41]

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_...)

[dcheng, 2016-12-19 18:29:59]

dcheng@chromium.org changed reviewers:
+ kinuko@chromium.org

[dcheng, 2016-12-19 18:30:00]

+kinuko, mind taking a look at this? Many loading people seem to be OOO right
now =)

[kinuko, 2016-12-20 08:09:28]

https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/Page.h (right):

https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/Page.h:198: // is also suspended.
A bit tangential, but: afaik we're going to introduce suspend state for
background tabs but from the new comment this 'pause' state seems to mean a bit
different thing, or are we going to merge them?

(I think this used to be called defersLoading or something)

[dcheng, 2016-12-20 08:19:04]

https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/Page.h (right):

https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/Page.h:198: // is also suspended.
On 2016/12/20 08:09:28, kinuko wrote:
> A bit tangential, but: afaik we're going to introduce suspend state for
> background tabs but from the new comment this 'pause' state seems to mean a
bit
> different thing, or are we going to merge them?
> 
> (I think this used to be called defersLoading or something)

Yeah, it's definitely confusing. This was renamed for ScopedPageLoadDeferrer to
https://codereview.chromium.org/2526163002, because it also suspends
ActiveDOMObjects.

[dcheng, 2016-12-20 08:23:48]

dcheng@chromium.org changed reviewers:
+ haraken@chromium.org

[dcheng, 2016-12-20 08:23:49]

On 2016/12/20 08:19:04, dcheng wrote:
>
https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/page/Page.h (right):
> 
>
https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/page/Page.h:198: // is also suspended.
> On 2016/12/20 08:09:28, kinuko wrote:
> > A bit tangential, but: afaik we're going to introduce suspend state for
> > background tabs but from the new comment this 'pause' state seems to mean a
> bit
> > different thing, or are we going to merge them?
> > 
> > (I think this used to be called defersLoading or something)
> 
> Yeah, it's definitely confusing. This was renamed for ScopedPageLoadDeferrer
to
> https://codereview.chromium.org/2526163002, because it also suspends
> ActiveDOMObjects.

Oops, I didn't answer the second part of your comment. I don't think there's any
current plan to merge the concepts, but we should definitely think about the
naming. +haraken

[haraken, 2016-12-20 10:05:38]

https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/Page.h (right):

https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/Page.h:198: // is also suspended.
On 2016/12/20 08:19:04, dcheng wrote:
> On 2016/12/20 08:09:28, kinuko wrote:
> > A bit tangential, but: afaik we're going to introduce suspend state for
> > background tabs but from the new comment this 'pause' state seems to mean a
> bit
> > different thing, or are we going to merge them?
> > 
> > (I think this used to be called defersLoading or something)
> 
> Yeah, it's definitely confusing. This was renamed for ScopedPageLoadDeferrer
to
> https://codereview.chromium.org/2526163002, because it also suspends
> ActiveDOMObjects.

Yeah, this is confusing...

Just to clarify: The "pause" in the spec means the "suspend" state of
SuspendableObjects, right?

Maybe we should rename the "suspend" state of the tab suspension to something
else.

[haraken, 2016-12-20 10:06:41]

On 2016/12/20 10:05:38, haraken wrote:
>
https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/page/Page.h (right):
> 
>
https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/page/Page.h:198: // is also suspended.
> On 2016/12/20 08:19:04, dcheng wrote:
> > On 2016/12/20 08:09:28, kinuko wrote:
> > > A bit tangential, but: afaik we're going to introduce suspend state for
> > > background tabs but from the new comment this 'pause' state seems to mean
a
> > bit
> > > different thing, or are we going to merge them?
> > > 
> > > (I think this used to be called defersLoading or something)
> > 
> > Yeah, it's definitely confusing. This was renamed for ScopedPageLoadDeferrer
> to
> > https://codereview.chromium.org/2526163002, because it also suspends
> > ActiveDOMObjects.
> 
> Yeah, this is confusing...
> 
> Just to clarify: The "pause" in the spec means the "suspend" state of
> SuspendableObjects, right?
> 
> Maybe we should rename the "suspend" state of the tab suspension to something
> else.

A better idea might be:

- rename the "suspend" state of SuspendableObjects to "pause", per the spec
- keep the "suspend" state of the tab suspension as is

[dcheng, 2016-12-20 10:09:43]

On 2016/12/20 10:06:41, haraken wrote:
> On 2016/12/20 10:05:38, haraken wrote:
> >
>
https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/page/Page.h (right):
> > 
> >
>
https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/page/Page.h:198: // is also suspended.
> > On 2016/12/20 08:19:04, dcheng wrote:
> > > On 2016/12/20 08:09:28, kinuko wrote:
> > > > A bit tangential, but: afaik we're going to introduce suspend state for
> > > > background tabs but from the new comment this 'pause' state seems to
mean
> a
> > > bit
> > > > different thing, or are we going to merge them?
> > > > 
> > > > (I think this used to be called defersLoading or something)
> > > 
> > > Yeah, it's definitely confusing. This was renamed for
ScopedPageLoadDeferrer
> > to
> > > https://codereview.chromium.org/2526163002, because it also suspends
> > > ActiveDOMObjects.
> > 
> > Yeah, this is confusing...
> > 
> > Just to clarify: The "pause" in the spec means the "suspend" state of
> > SuspendableObjects, right?
> > 
> > Maybe we should rename the "suspend" state of the tab suspension to
something
> > else.
> 
> A better idea might be:
> 
> - rename the "suspend" state of SuspendableObjects to "pause", per the spec
> - keep the "suspend" state of the tab suspension as is

pause()+resume() makes sense to me, and I'm happy to do this rename (after the
Blink rename ;-), but I wonder if resume() can still be confused with resume
from tab suspension?

Either way, I think we can address this independently of this CL.

[haraken, 2016-12-20 10:14:24]

On 2016/12/20 10:09:43, dcheng wrote:
> On 2016/12/20 10:06:41, haraken wrote:
> > On 2016/12/20 10:05:38, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/page/Page.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/page/Page.h:198: // is also suspended.
> > > On 2016/12/20 08:19:04, dcheng wrote:
> > > > On 2016/12/20 08:09:28, kinuko wrote:
> > > > > A bit tangential, but: afaik we're going to introduce suspend state
for
> > > > > background tabs but from the new comment this 'pause' state seems to
> mean
> > a
> > > > bit
> > > > > different thing, or are we going to merge them?
> > > > > 
> > > > > (I think this used to be called defersLoading or something)
> > > > 
> > > > Yeah, it's definitely confusing. This was renamed for
> ScopedPageLoadDeferrer
> > > to
> > > > https://codereview.chromium.org/2526163002, because it also suspends
> > > > ActiveDOMObjects.
> > > 
> > > Yeah, this is confusing...
> > > 
> > > Just to clarify: The "pause" in the spec means the "suspend" state of
> > > SuspendableObjects, right?
> > > 
> > > Maybe we should rename the "suspend" state of the tab suspension to
> something
> > > else.
> > 
> > A better idea might be:
> > 
> > - rename the "suspend" state of SuspendableObjects to "pause", per the spec
> > - keep the "suspend" state of the tab suspension as is
> 
> pause()+resume() makes sense to me, and I'm happy to do this rename (after the
> Blink rename ;-), but I wonder if resume() can still be confused with resume
> from tab suspension?
> 
> Either way, I think we can address this independently of this CL.

Yes.

[dcheng, 2016-12-21 09:35:53]

ping =)

[haraken, 2016-12-21 09:50:31]

I guess kinuko-san would be a better reviewer for this.

[kinuko, 2016-12-21 13:38:17]

Thanks for the renaming discussion, pause sounds clearer to me too.

lgtm mod one question

https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/Page.cpp (right):

https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/Page.cpp:159:
toLocalFrame(m_mainFrame)->loader().stopAllLoaders();
Is the TODO the reason you replaced mainFrame->stopLoading() with more explicit
code like this?

[dcheng, 2016-12-22 08:39:11]

https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/Page.cpp (right):

https://codereview.chromium.org/2580703003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/Page.cpp:159:
toLocalFrame(m_mainFrame)->loader().stopAllLoaders();
On 2016/12/21 13:38:17, kinuko wrote:
> Is the TODO the reason you replaced mainFrame->stopLoading() with more
explicit
> code like this?

Yes: we were calling generically through mainFrame->stopLoading() before, but
it's already a no-op for remote frames.

Since we're going to detach the frames soon after this (which will also call
stop loading, as well as correctly stop all recursive loads), my theory is we
can just remove this completely.

[dcheng, 2016-12-22 08:39:39]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-12-22 08:39:50]

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, 2016-12-22 10:36:53]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482395979554580,
"parent_rev": "b15e9f8ead5f48a2b8183a5ccace2e80f6b795d3", "commit_rev":
"df0c51e0c69376693647323e6f6c88459cf445c7"}

[commit-bot: I haz the power, 2016-12-22 10:37:25]

Description was changed from

==========
Make sure pages that are closing but not yet closed are still suspended.

BUG=671102
==========

to

==========
Make sure pages that are closing but not yet closed are still suspended.

BUG=671102

Review-Url: https://codereview.chromium.org/2580703003
==========

[commit-bot: I haz the power, 2016-12-22 10:37:26]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-22 10:39:59]

Description was changed from

==========
Make sure pages that are closing but not yet closed are still suspended.

BUG=671102

Review-Url: https://codereview.chromium.org/2580703003
==========

to

==========
Make sure pages that are closing but not yet closed are still suspended.

BUG=671102

Committed: https://crrev.com/0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72
Cr-Commit-Position: refs/heads/master@{#440376}
==========

[commit-bot: I haz the power, 2016-12-22 10:40:00]

Patchset 5 (id:??) landed as
https://crrev.com/0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72
Cr-Commit-Position: refs/heads/master@{#440376}