Defer loads in new pages/frames if ScopedPageLoadDeferral is active

Defer loads in new pages/frames if ScopedPageLoadDeferral is active

BUG=628942
Committed: https://crrev.com/19ad54cb204cde45db95e773c5d54b04b2f178d4
Cr-Commit-Position: refs/heads/master@{#408354}

[dcheng, 2016-07-25 06:16:23]

Description was changed from

==========
Moo


Moo


Moo


Moo


Hammertime

BUG=
==========

to

==========
Defer loads in new pages/frames if ScopedPageLoadDeferral is active

BUG=628942
==========

[dcheng, 2016-07-25 06:16:23]

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

[dcheng, 2016-07-25 06:48:18]

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

[dcheng, 2016-07-25 06:48:18]

I'm not a huge fan of this patch but it does seem to work.

+japhet / +dominicc, what are your thoughts? In theory, this shouldn't be needed
but thanks to it's necessary atm due to the interface we currently provide to
the embedder for running scripts.

https://codereview.chromium.org/2174263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2174263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:278:
DCHECK(m_frame->loader().stateMachine()->creatingInitialEmptyDocument()
This is called during FrameLoader::init() to create the initial empty document.
It "should" be OK to allow this, as the document will always be same origin.

https://codereview.chromium.org/2174263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2174263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:211:
setDefersLoading(true);
This makes me pretty sad. When we initially create a Page, we don't actually
have a frame associated with it. Thus, we have to tell the page to self-defer
once the initial empty page is synchronously loaded.

I'm also uncertain of the correctness of doing it here: I believe
startLoadingMainResource() also runs events, including the iframe load event. I
think it's OK because the navigations will get scheduled, and the defers loading
call here will suspend them.. but it's pretty dodgy.

https://codereview.chromium.org/2174263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/2174263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:3238: TEST_F(WebViewTest,
NestedLoadDeferrals)
I haven't figured out why, but this test (unexpectedly) passes without my patch
even though there's no tracking of the defer count...

[dcheng, 2016-07-25 06:49:29]

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

[commit-bot: I haz the power, 2016-07-25 06:49:40]

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

[dominicc (has gone to gerrit), 2016-07-25 07:40:14]

What's not to like? Deferral should nest. Getting rid of that unused exclusion
thing looks like a win.

Is the problem with the test that the m_webViewHelper ends up reusing something?

[commit-bot: I haz the power, 2016-07-25 07:53:23]

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

[commit-bot: I haz the power, 2016-07-25 07:53:23]

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-07-25 09:33:07]

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

[commit-bot: I haz the power, 2016-07-25 09:33:26]

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

[dcheng, 2016-07-25 09:35:41]

On 2016/07/25 07:40:14, dominicc wrote:
> What's not to like? Deferral should nest. Getting rid of that unused exclusion
> thing looks like a win.

The unused exclusion thing was just incidental cleanup. I think my main issue
with this patch is twofold:
1) when we have to nest deferrals, we're already solidly into the dangerous
zone: script is executing synchronously when it shouldn't be.
2) "deferred" now means "mostly deferred": we can't really defer the initial
empty document, so we have to allow it... which means more script running.

> 
> Is the problem with the test that the m_webViewHelper ends up reusing
something?

That should be OK... in theory. I'll investigate more later, but this isn't the
highest priority for me atm.

[commit-bot: I haz the power, 2016-07-25 11:54:06]

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

[commit-bot: I haz the power, 2016-07-25 11:54:07]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-07-25 17:11:39]

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

[Nate Chapin, 2016-07-25 17:11:43]

The CQ bit was unchecked by japhet@chromium.org

[dcheng, 2016-07-27 08:12:11]

PTAL. I simplified things a little bit: since we only defer 'ordinary pages', I
just use Page::ordinaryPages() directly now rather than keeping a separate set.
It was also a bit weird, because the set of deferred pages could keep a Page
object alive.

While we could try deferring all pages, I'm not certain of the correctness of
that change... so I'd prefer to make the more conservative change for now.

FWIW, I think we'll need this anyway because of things like
https://crbug.com/631521.

[dcheng, 2016-07-27 08:12:24]

+alexmos as FYI

[Nate Chapin, 2016-07-27 17:30:47]

LGTM...I guess.

https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:210: if (m_frame->page()
&& m_frame->page()->defersLoading())
This makes me saddest of all :(

https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp (right):

https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp:57: if
(--s_deferralCount == 0) {
Nit: it bothers my sense of aesthetics that the constructor early-exits and the
destructor doesn't :)

[dominicc (has gone to gerrit), 2016-07-28 00:58:09]

dominicc@chromium.org changed reviewers:
- dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-07-28 00:58:36]

-r=me, japhet is a better person to review this!

[dcheng, 2016-07-28 01:23:28]

https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:210: if (m_frame->page()
&& m_frame->page()->defersLoading())
On 2016/07/27 17:30:47, Nate Chapin wrote:
> This makes me saddest of all :(

Yes... I would have to completely agree =(

https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp (right):

https://codereview.chromium.org/2174263002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp:57: if
(--s_deferralCount == 0) {
On 2016/07/27 17:30:47, Nate Chapin wrote:
> Nit: it bothers my sense of aesthetics that the constructor early-exits and
the
> destructor doesn't :)

Done.

[dcheng, 2016-07-28 01:23:36]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-07-28 01:23:37]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2174263002/#ps120001
(title: "Make the unfortunate hack slightly less ugly.")

[commit-bot: I haz the power, 2016-07-28 01:24:08]

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-07-28 01:46:15]

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

[commit-bot: I haz the power, 2016-07-28 01:46:18]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[dcheng, 2016-07-28 02:15:16]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-07-28 02:15:31]

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-07-28 04:17:30]

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

[commit-bot: I haz the power, 2016-07-28 04:17:32]

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

[dcheng, 2016-07-28 04:20:50]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-07-28 04:21:00]

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-07-28 07:34:20]

Description was changed from

==========
Defer loads in new pages/frames if ScopedPageLoadDeferral is active

BUG=628942
==========

to

==========
Defer loads in new pages/frames if ScopedPageLoadDeferral is active

BUG=628942
==========

[commit-bot: I haz the power, 2016-07-28 07:34:23]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-28 07:36:26]

Description was changed from

==========
Defer loads in new pages/frames if ScopedPageLoadDeferral is active

BUG=628942
==========

to

==========
Defer loads in new pages/frames if ScopedPageLoadDeferral is active

BUG=628942

Committed: https://crrev.com/19ad54cb204cde45db95e773c5d54b04b2f178d4
Cr-Commit-Position: refs/heads/master@{#408354}
==========

[commit-bot: I haz the power, 2016-07-28 07:36:28]

Patchset 7 (id:??) landed as
https://crrev.com/19ad54cb204cde45db95e773c5d54b04b2f178d4
Cr-Commit-Position: refs/heads/master@{#408354}