Make NavigationScheduler post loading tasks instead of timers

Make NavigationScheduler post loading tasks instead of timers

Similar to https://codereview.chromium.org/1312353004/

We would like to be able to prioritize loading tasks, but in order to
do that we need to make sure loading tasks are posted to the right queue
to make sure tasks run in the expected order.  If this task is posted as
a timer, and loading tasks are prioritzed, it's possible
FrameHostMsg_DidStopLoading will be sent before the corresponding
FrameHostMsg_DidStartLoading ipc which causes various browser tests to
break.

Must be submitted after https://codereview.chromium.org/1303153005/

BUG=497761, 510398
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201780

[alex clarke (OOO till 29th), 2015-09-02 16:52:40]

alexclarke@chromium.org changed reviewers:
+ skyostil@chromium.org

[alex clarke (OOO till 29th), 2015-09-03 11:35:28]

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

[commit-bot: I haz the power, 2015-09-03 11:35:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1305933007/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1305933007/1

[commit-bot: I haz the power, 2015-09-03 11:49:35]

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

[commit-bot: I haz the power, 2015-09-03 11:49:36]

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

[alex clarke (OOO till 29th), 2015-09-03 18:45:09]

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

[commit-bot: I haz the power, 2015-09-03 18:45:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1305933007/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1305933007/1

[alex clarke (OOO till 29th), 2015-09-03 19:52:34]

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

[alex clarke (OOO till 29th), 2015-09-03 19:52:34]

haraken@chromium.org: Please review changes in this patch.  I did try this with
a oilpan build and didn't see any (new) leaks but considering the fun with the
last patch I thought you'd make a good reviewer.

[commit-bot: I haz the power, 2015-09-03 20:01:23]

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

[commit-bot: I haz the power, 2015-09-03 20:01:24]

Dry run: This issue passed the CQ dry run.

[haraken, 2015-09-03 23:43:29]

haraken@chromium.org changed reviewers:
+ sigbjornf@opera.com

[haraken, 2015-09-03 23:43:30]

https://codereview.chromium.org/1305933007/diff/1/Source/core/loader/Navigati...
File Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/1305933007/diff/1/Source/core/loader/Navigati...
Source/core/loader/NavigationScheduler.cpp:270: ,
m_navigateTaskFactory(WTF::bind(&NavigationScheduler::navigateTask, this))

Sigbjorn: NavigationScheduler is a part of object. What happens if we create a
closure that points to a part of object? (i.e., how does the closure keep it
alive?)

[sof, 2015-09-04 06:28:34]

https://codereview.chromium.org/1305933007/diff/1/Source/core/loader/Navigati...
File Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/1305933007/diff/1/Source/core/loader/Navigati...
Source/core/loader/NavigationScheduler.cpp:270: ,
m_navigateTaskFactory(WTF::bind(&NavigationScheduler::navigateTask, this))
On 2015/09/03 23:43:30, haraken wrote:
> 
> Sigbjorn: NavigationScheduler is a part of object. What happens if we create a
> closure that points to a part of object? (i.e., how does the closure keep it
> alive?)

It will work out fine, but https://codereview.chromium.org/1312843009/ makes
these task factories no longer be part objects.

If the task (CancellableTask, in this case) has a weak pointer back to its
factory (the CancellableTaskFactory part object), then it doesn't matter where
the factory object is allocated -- once it is destructed, the weak pointers are
revoked & the task will no longer be able to access the closure.

(cf. CancellableTaskFactory::CancellableTask::run().)

[haraken, 2015-09-04 06:32:41]

LGTM

https://codereview.chromium.org/1305933007/diff/1/Source/core/loader/Navigati...
File Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/1305933007/diff/1/Source/core/loader/Navigati...
Source/core/loader/NavigationScheduler.cpp:270: ,
m_navigateTaskFactory(WTF::bind(&NavigationScheduler::navigateTask, this))
On 2015/09/04 06:28:34, sof wrote:
> On 2015/09/03 23:43:30, haraken wrote:
> > 
> > Sigbjorn: NavigationScheduler is a part of object. What happens if we create
a
> > closure that points to a part of object? (i.e., how does the closure keep it
> > alive?)
> 
> It will work out fine, but https://codereview.chromium.org/1312843009/ makes
> these task factories no longer be part objects.
> 
> If the task (CancellableTask, in this case) has a weak pointer back to its
> factory (the CancellableTaskFactory part object), then it doesn't matter where
> the factory object is allocated -- once it is destructed, the weak pointers
are
> revoked & the task will no longer be able to access the closure.
> 
> (cf. CancellableTaskFactory::CancellableTask::run().)

Makes sense.

[alex clarke (OOO till 29th), 2015-09-04 07:40:25]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2015-09-04 07:40:34]

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

[commit-bot: I haz the power, 2015-09-04 07:44:48]

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

[pdr., 2015-09-04 17:29:42]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1303153008/ by pdr@chromium.org.

The reason for reverting is: I think this is causing timeouts:
http://crbug.com/528352..

[jochen (gone - plz use gerrit), 2015-09-07 14:50:58]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-09-07 14:50:59]

https://codereview.chromium.org/1305933007/diff/1/Source/core/loader/Navigati...
File Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/1305933007/diff/1/Source/core/loader/Navigati...
Source/core/loader/NavigationScheduler.cpp:423: FROM_HERE,
m_navigateTaskFactory.cancelAndCreate(), m_redirect->delay());
this is wrong: startOneShot takes seconds, postDelayedTask takes milliseconds.