[Reland] Post loading tasks on the appropriate WebFrameScheduler's queue

Post loading tasks on the appropriate WebFrameScheduler's queue.

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

BUG=510398, 538660
Committed: https://crrev.com/bcc4ce71b07ea71a1056c3227c5bc381c52256fd
Cr-Commit-Position: refs/heads/master@{#352036}

Committed: https://crrev.com/f3080498facd16066076e7d459ba7cfda11e582c
Cr-Commit-Position: refs/heads/master@{#352866}

[alex clarke (OOO till 29th), 2015-09-25 12:39:53]

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

[Sami, 2015-09-25 13:31:27]

Overall plumbing looks reasonable. As usual I'm a little paranoid about object
lifetimes.

https://codereview.chromium.org/1366883002/diff/20001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/1366883002/diff/20001/content/child/resource_...
content/child/resource_dispatcher.cc:118: // dispatch the message there,
otherwise dispatch it here.
I guess the other alternative would be to dispatch this on the correct runner
from the IO thread. Would it make sense to have a message filter which does
that? That would avoid this extra main thread hop.

I was thinking back to your earlier patch that forwards ranges of IPC messages
to different task runners, but this would need to be more general since we need
to inspect the message before we can route it. Also, |request_info| is probably
not thread safe so we might need to be smarter about how we read it.

WDYT?

https://codereview.chromium.org/1366883002/diff/20001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1366883002/diff/20001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:1072: scheduler::WebTaskRunnerImpl*
web_task_runner_impl =
Is this downcast safe? I think anyone could call this with an arbitrary
WebTaskRunner.

https://codereview.chromium.org/1366883002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/1366883002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:144:
m_loader->setLoadingTaskRunner(m_fetcher->loadingTaskRunner());
nit: Move this below the assert?

https://codereview.chromium.org/1366883002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/1366883002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:172: ,
m_loadingTaskRunner(Platform::current()->currentThread()->scheduler()->loadingTaskRunner())
I'm guessing this isn't used anymore after the document fragment is attached to
a real document? I.e., there's no reason to switch this to be the frame's
loading task runner at that point?

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

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

[commit-bot: I haz the power, 2015-09-28 11:09:00]

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

[commit-bot: I haz the power, 2015-09-28 12:19:15]

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

[commit-bot: I haz the power, 2015-09-28 12:19:16]

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-28 17:24:53]

I want to fix the lifetime of WebTaskRunner in chromium, however other than that
I think this addresses your concerns.

https://codereview.chromium.org/1366883002/diff/20001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/1366883002/diff/20001/content/child/resource_...
content/child/resource_dispatcher.cc:118: // dispatch the message there,
otherwise dispatch it here.
On 2015/09/25 13:31:27, Sami wrote:
> I guess the other alternative would be to dispatch this on the correct runner
> from the IO thread. Would it make sense to have a message filter which does
> that? That would avoid this extra main thread hop.
> 
> I was thinking back to your earlier patch that forwards ranges of IPC messages
> to different task runners, but this would need to be more general since we
need
> to inspect the message before we can route it. Also, |request_info| is
probably
> not thread safe so we might need to be smarter about how we read it.
> 
> WDYT?

Done. Turned out to be much less invasive than I'd expected.

https://codereview.chromium.org/1366883002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/1366883002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:144:
m_loader->setLoadingTaskRunner(m_fetcher->loadingTaskRunner());
On 2015/09/25 13:31:27, Sami wrote:
> nit: Move this below the assert?

Done.

https://codereview.chromium.org/1366883002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/1366883002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:172: ,
m_loadingTaskRunner(Platform::current()->currentThread()->scheduler()->loadingTaskRunner())
On 2015/09/25 13:31:27, Sami wrote:
> I'm guessing this isn't used anymore after the document fragment is attached
to
> a real document? 

I think that's correct, m_loadingTaskRunner is passed to the
BackgroundHTMLParser::start and presumably the fragment can't be attached to a
real document until that's completed.

I.e., there's no reason to switch this to be the frame's
> loading task runner at that point?

Yes, however I think we can do it here (most of the time).  The DocumentFragment
contains a refrence to the parent document and we can use the loading task
runner from that document's frame (if there is one).

[alex clarke (OOO till 29th), 2015-09-29 10:32:56]

PTAL

[alex clarke (OOO till 29th), 2015-09-29 10:33:05]

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

[commit-bot: I haz the power, 2015-09-29 10:33:38]

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

[commit-bot: I haz the power, 2015-09-29 10:49:44]

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

[commit-bot: I haz the power, 2015-09-29 10:49:45]

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

[alex clarke (OOO till 29th), 2015-09-29 11:08:20]

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

[commit-bot: I haz the power, 2015-09-29 11:08:40]

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

[commit-bot: I haz the power, 2015-09-29 11:22:09]

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

[commit-bot: I haz the power, 2015-09-29 11:22:10]

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

[Sami, 2015-09-29 11:22:47]

Great that you found a way around the main thread trampoline.

https://codereview.chromium.org/1366883002/diff/100001/components/scheduler/c...
File components/scheduler/child/web_task_runner_impl.h (left):

https://codereview.chromium.org/1366883002/diff/100001/components/scheduler/c...
components/scheduler/child/web_task_runner_impl.h:45:
DISALLOW_COPY_AND_ASSIGN(WebTaskRunnerImpl);
We can still keep this, right?

https://codereview.chromium.org/1366883002/diff/100001/content/child/blink_pl...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1366883002/diff/100001/content/child/blink_pl...
content/child/blink_platform_impl.cc:482: new
scheduler::WebTaskRunnerImpl(LoadingTaskRunnerForCurrentThread()));
Since LoadingTaskRunnerForCurrentThread is only used here, I wonder if it would
be cleaner to just reimplement createURLLoader in RendererBlinkPlatformImpl.
WDYT?

https://codereview.chromium.org/1366883002/diff/100001/content/child/blink_pl...
content/child/blink_platform_impl.cc:1368:
BlinkPlatformImpl::LoadingTaskRunnerForCurrentThread() {
I think you could replace all of this with base::ThreadTaskRunnerHandle::Get().

https://codereview.chromium.org/1366883002/diff/100001/content/child/request_...
File content/child/request_info.h (right):

https://codereview.chromium.org/1366883002/diff/100001/content/child/request_...
content/child/request_info.h:119: blink::WebTaskRunner* loading_web_task_runner;
Who owns this? Should this be a scoped_ptr?

https://codereview.chromium.org/1366883002/diff/100001/content/child/resource...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/1366883002/diff/100001/content/child/resource...
content/child/resource_dispatcher.cc:598: request_id,
request_info.loading_web_task_runner->clone());
|loading_web_task_runner| was optional so should we null check?

https://codereview.chromium.org/1366883002/diff/100001/content/child/resource...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/1366883002/diff/100001/content/child/resource...
content/child/resource_dispatcher.h:135: ResourceSchedulingFilter*
resource_scheduling_filter);
Hmm, ResourceSchedulingFilter is refcounted, so there's no way to take ownership
of it?

https://codereview.chromium.org/1366883002/diff/100001/content/child/resource...
File content/child/resource_scheduling_filter.cc (right):

https://codereview.chromium.org/1366883002/diff/100001/content/child/resource...
content/child/resource_scheduling_filter.cc:66:
blink::WebTraceLocation(__FUNCTION__, __FILE__),
Is this because we can't use the FROM_HERE from Blink and base in the same file?
Could you add a todo for fixing it?

https://codereview.chromium.org/1366883002/diff/100001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1366883002/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:259: blink::WebTaskRunner* task_runner);
scoped_ptr here too?

https://codereview.chromium.org/1366883002/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:273: void
SetWebTaskRunner(blink::WebTaskRunner* task_runner);
Could this be a scoped_ptr? This would mean the caller has to do the clone()
which I think would also be clearer.

https://codereview.chromium.org/1366883002/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:379:
::blink::WebTraceLocation(__FUNCTION__, __FILE__),
Ditto about FROM_HERE

https://codereview.chromium.org/1366883002/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:439:
::blink::WebTraceLocation(__FUNCTION__, __FILE__),
Ditto.

https://codereview.chromium.org/1366883002/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:503: request_info.loading_web_task_runner =
web_task_runner_.get();
nit: s/get/Pass/

https://codereview.chromium.org/1366883002/diff/100001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1366883002/diff/100001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1043: ResourceSchedulingFilter* filter =
scoped_ptr?

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

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4848: fprintf(stderr,
"didUpdateSecurityOrigin %p\n", m_frame.get());
Oops?-)

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:147: ,
m_loadingTaskRunner(document.frame() ?
document.frame()->frameScheduler()->loadingTaskRunner()
nit: might be worth adding a loadingTaskRunner() getter which hides these null
checks.

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebTaskRunner.h (right):

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebTaskRunner.h:43: virtual WebTaskRunner*
clone() { return nullptr; }
nit: I think these can be pure virtual now that we're in the same repo.

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLLoader.h (right):

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLLoader.h:84: virtual void
setLoadingTaskRunner(WebTaskRunner*) {}
Ditto about pure virtual.

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

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

[commit-bot: I haz the power, 2015-09-29 15:45:25]

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

[alex clarke (OOO till 29th), 2015-09-29 15:57:06]

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

[commit-bot: I haz the power, 2015-09-29 15:57:56]

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

[alex clarke (OOO till 29th), 2015-09-29 16:10:36]

https://codereview.chromium.org/1366883002/diff/100001/components/scheduler/c...
File components/scheduler/child/web_task_runner_impl.h (left):

https://codereview.chromium.org/1366883002/diff/100001/components/scheduler/c...
components/scheduler/child/web_task_runner_impl.h:45:
DISALLOW_COPY_AND_ASSIGN(WebTaskRunnerImpl);
On 2015/09/29 11:22:46, Sami wrote:
> We can still keep this, right?

Done.

https://codereview.chromium.org/1366883002/diff/100001/content/child/request_...
File content/child/request_info.h (right):

https://codereview.chromium.org/1366883002/diff/100001/content/child/request_...
content/child/request_info.h:119: blink::WebTaskRunner* loading_web_task_runner;
On 2015/09/29 11:22:46, Sami wrote:
> Who owns this? Should this be a scoped_ptr?

Done.

https://codereview.chromium.org/1366883002/diff/100001/content/child/resource...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/1366883002/diff/100001/content/child/resource...
content/child/resource_dispatcher.cc:598: request_id,
request_info.loading_web_task_runner->clone());
On 2015/09/29 11:22:46, Sami wrote:
> |loading_web_task_runner| was optional so should we null check?

Done.

https://codereview.chromium.org/1366883002/diff/100001/content/child/resource...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/1366883002/diff/100001/content/child/resource...
content/child/resource_dispatcher.h:135: ResourceSchedulingFilter*
resource_scheduling_filter);
On 2015/09/29 11:22:46, Sami wrote:
> Hmm, ResourceSchedulingFilter is refcounted, so there's no way to take
ownership
> of it?

Done.

https://codereview.chromium.org/1366883002/diff/100001/content/child/resource...
File content/child/resource_scheduling_filter.cc (right):

https://codereview.chromium.org/1366883002/diff/100001/content/child/resource...
content/child/resource_scheduling_filter.cc:66:
blink::WebTraceLocation(__FUNCTION__, __FILE__),
On 2015/09/29 11:22:46, Sami wrote:
> Is this because we can't use the FROM_HERE from Blink and base in the same
file?
> Could you add a todo for fixing it?

Done.

https://codereview.chromium.org/1366883002/diff/100001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1366883002/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:259: blink::WebTaskRunner* task_runner);
On 2015/09/29 11:22:46, Sami wrote:
> scoped_ptr here too?

Done.

https://codereview.chromium.org/1366883002/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:273: void
SetWebTaskRunner(blink::WebTaskRunner* task_runner);
On 2015/09/29 11:22:46, Sami wrote:
> Could this be a scoped_ptr? This would mean the caller has to do the clone()
> which I think would also be clearer.

Done, but It already did call clone.

https://codereview.chromium.org/1366883002/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:379:
::blink::WebTraceLocation(__FUNCTION__, __FILE__),
On 2015/09/29 11:22:46, Sami wrote:
> Ditto about FROM_HERE

Done.

https://codereview.chromium.org/1366883002/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:439:
::blink::WebTraceLocation(__FUNCTION__, __FILE__),
On 2015/09/29 11:22:47, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/1366883002/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:503: request_info.loading_web_task_runner =
web_task_runner_.get();
On 2015/09/29 11:22:46, Sami wrote:
> nit: s/get/Pass/

Needs to be a clone().

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

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4848: fprintf(stderr,
"didUpdateSecurityOrigin %p\n", m_frame.get());
On 2015/09/29 11:22:47, Sami wrote:
> Oops?-)

Done.

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebTaskRunner.h (right):

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebTaskRunner.h:43: virtual WebTaskRunner*
clone() { return nullptr; }
On 2015/09/29 11:22:47, Sami wrote:
> nit: I think these can be pure virtual now that we're in the same repo.

Done.

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLLoader.h (right):

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLLoader.h:84: virtual void
setLoadingTaskRunner(WebTaskRunner*) {}
On 2015/09/29 11:22:47, Sami wrote:
> Ditto about pure virtual.

Done.

[commit-bot: I haz the power, 2015-09-29 16:14:01]

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

[commit-bot: I haz the power, 2015-09-29 16:14:02]

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

[alex clarke (OOO till 29th), 2015-09-29 16:28:51]

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

[commit-bot: I haz the power, 2015-09-29 16:29:16]

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

[alex clarke (OOO till 29th), 2015-09-29 16:37:38]

PTAL

https://codereview.chromium.org/1366883002/diff/100001/content/child/blink_pl...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1366883002/diff/100001/content/child/blink_pl...
content/child/blink_platform_impl.cc:482: new
scheduler::WebTaskRunnerImpl(LoadingTaskRunnerForCurrentThread()));
On 2015/09/29 11:22:46, Sami wrote:
> Since LoadingTaskRunnerForCurrentThread is only used here, I wonder if it
would
> be cleaner to just reimplement createURLLoader in RendererBlinkPlatformImpl.
> WDYT?

Done.

https://codereview.chromium.org/1366883002/diff/100001/content/child/blink_pl...
content/child/blink_platform_impl.cc:1368:
BlinkPlatformImpl::LoadingTaskRunnerForCurrentThread() {
On 2015/09/29 11:22:46, Sami wrote:
> I think you could replace all of this with
base::ThreadTaskRunnerHandle::Get().

Done.

https://codereview.chromium.org/1366883002/diff/100001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1366883002/diff/100001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1043: ResourceSchedulingFilter* filter =
On 2015/09/29 11:22:47, Sami wrote:
> scoped_ptr?

Done.

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/1366883002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:147: ,
m_loadingTaskRunner(document.frame() ?
document.frame()->frameScheduler()->loadingTaskRunner()
On 2015/09/29 11:22:47, Sami wrote:
> nit: might be worth adding a loadingTaskRunner() getter which hides these null
> checks.

Done.

[alex clarke (OOO till 29th), 2015-09-29 16:38:04]

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

[commit-bot: I haz the power, 2015-09-29 16:38:24]

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

[commit-bot: I haz the power, 2015-09-29 16:52:16]

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

[commit-bot: I haz the power, 2015-09-29 16:52:17]

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

[alex clarke (OOO till 29th), 2015-09-29 16:56:21]

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

[commit-bot: I haz the power, 2015-09-29 16:56:59]

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

[commit-bot: I haz the power, 2015-09-29 17:14:40]

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

[commit-bot: I haz the power, 2015-09-29 17:14:41]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[alex clarke (OOO till 29th), 2015-09-30 09:46:01]

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

[commit-bot: I haz the power, 2015-09-30 09:46:24]

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

[commit-bot: I haz the power, 2015-09-30 10:10:44]

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

[commit-bot: I haz the power, 2015-09-30 10:10:45]

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

[alex clarke (OOO till 29th), 2015-09-30 10:15:35]

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

[commit-bot: I haz the power, 2015-09-30 10:16:05]

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

[commit-bot: I haz the power, 2015-09-30 11:28:43]

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

[commit-bot: I haz the power, 2015-09-30 11:28:44]

Dry run: This issue passed the CQ dry run.

[Sami, 2015-09-30 13:03:16]

lgtm with some nits.

https://codereview.chromium.org/1366883002/diff/270001/components/html_viewer...
File components/html_viewer/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1366883002/diff/270001/components/html_viewer...
components/html_viewer/web_url_loader_impl.cc:317: // There isnt a scheduler so
we can safely ignore this.
Hmm, html_viewer does use the renderer scheduler. Not sure if we need to hook
this up right away, but maybe worth seeing how difficult it would be?

https://codereview.chromium.org/1366883002/diff/270001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1366883002/diff/270001/content/child/web_url_...
content/child/web_url_loader_impl.cc:303: explicit HandleDataURLTask(Context*
context) : context_(context) {}
nit: scoped_refptr

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.h (right):

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.h:154: WebTaskRunner*
m_loadingTaskRunner;
nit: add a comment about the lifetime here too?

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:291:
ScriptStreamer::startStreaming(m_parserBlockingScript,
PendingScript::ParsingBlocking, m_document->frame()->settings(), scriptState,
m_document->frame()->frameScheduler()->loadingTaskRunner());
nit: use the new accessor here?

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:307:
ScriptStreamer::startStreaming(pendingScript, PendingScript::Deferred,
m_document->frame()->settings(), scriptState,
m_document->frame()->frameScheduler()->loadingTaskRunner());
Ditto.

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/AssociatedURLLoader.cpp (right):

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/AssociatedURLLoader.cpp:388: // TODO(alexclarke):
Maybe support this one day if it proves worthwhile.
Based on a quick look it seems like this is used for things like plugins,
favicons and manifests, so probably not that urgent.

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLLoader.h (right):

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLLoader.h:83: // Sets the task runner
for which any loading tasks should be posted on.
nit: takes ownership?

[alex clarke (OOO till 29th), 2015-09-30 13:36:27]

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

[commit-bot: I haz the power, 2015-09-30 13:37:10]

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

[alex clarke (OOO till 29th), 2015-09-30 13:37:15]

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

[alex clarke (OOO till 29th), 2015-09-30 13:37:17]

jochen@ please do an OWNERS review :)

https://codereview.chromium.org/1366883002/diff/270001/components/html_viewer...
File components/html_viewer/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1366883002/diff/270001/components/html_viewer...
components/html_viewer/web_url_loader_impl.cc:317: // There isnt a scheduler so
we can safely ignore this.
On 2015/09/30 13:03:16, Sami wrote:
> Hmm, html_viewer does use the renderer scheduler. Not sure if we need to hook
> this up right away, but maybe worth seeing how difficult it would be?

Acknowledged.

https://codereview.chromium.org/1366883002/diff/270001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1366883002/diff/270001/content/child/web_url_...
content/child/web_url_loader_impl.cc:303: explicit HandleDataURLTask(Context*
context) : context_(context) {}
On 2015/09/30 13:03:16, Sami wrote:
> nit: scoped_refptr

Done.

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.h (right):

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.h:154: WebTaskRunner*
m_loadingTaskRunner;
On 2015/09/30 13:03:16, Sami wrote:
> nit: add a comment about the lifetime here too?

I think we can just clone it and use an OwnPtr here.

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:291:
ScriptStreamer::startStreaming(m_parserBlockingScript,
PendingScript::ParsingBlocking, m_document->frame()->settings(), scriptState,
m_document->frame()->frameScheduler()->loadingTaskRunner());
On 2015/09/30 13:03:16, Sami wrote:
> nit: use the new accessor here?

Done.

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:307:
ScriptStreamer::startStreaming(pendingScript, PendingScript::Deferred,
m_document->frame()->settings(), scriptState,
m_document->frame()->frameScheduler()->loadingTaskRunner());
On 2015/09/30 13:03:16, Sami wrote:
> Ditto.

Done.

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/AssociatedURLLoader.cpp (right):

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/AssociatedURLLoader.cpp:388: // TODO(alexclarke):
Maybe support this one day if it proves worthwhile.
On 2015/09/30 13:03:16, Sami wrote:
> Based on a quick look it seems like this is used for things like plugins,
> favicons and manifests, so probably not that urgent.

Acknowledged.

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebURLLoader.h (right):

https://codereview.chromium.org/1366883002/diff/270001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebURLLoader.h:83: // Sets the task runner
for which any loading tasks should be posted on.
On 2015/09/30 13:03:16, Sami wrote:
> nit: takes ownership?

Done.

[alex clarke (OOO till 29th), 2015-09-30 13:40:22]

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

[alex clarke (OOO till 29th), 2015-09-30 13:40:23]

+sandersd@ Please review changes in media/blink/mock_weburlloader.h

[commit-bot: I haz the power, 2015-09-30 13:52:02]

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

[commit-bot: I haz the power, 2015-09-30 13:52:03]

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

[alex clarke (OOO till 29th), 2015-09-30 14:22:26]

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

[commit-bot: I haz the power, 2015-09-30 14:23:11]

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

[commit-bot: I haz the power, 2015-09-30 16:05:23]

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

[commit-bot: I haz the power, 2015-09-30 16:05:24]

Dry run: This issue passed the CQ dry run.

[sandersd (OOO until July 31), 2015-09-30 17:50:09]

On 2015/09/30 16:05:24, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

LGTM for media/blink.

[alex clarke (OOO till 29th), 2015-10-02 10:29:22]

Hi Jochen, 

Gentle ping.  I know this patch is on the large side, it might be possible to
split some of it off into a separate patch if you prefer.

[jochen (gone - plz use gerrit), 2015-10-02 13:10:52]

lgtm

[alex clarke (OOO till 29th), 2015-10-02 13:13:23]

Thanks all!

[alex clarke (OOO till 29th), 2015-10-02 13:13:34]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-10-02 13:13:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1366883002/#ps310001
(title: "Fix compile")

[commit-bot: I haz the power, 2015-10-02 13:14:06]

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

[commit-bot: I haz the power, 2015-10-02 14:58:31]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2015-10-02 14:59:33]

Patchset 17 (id:??) landed as
https://crrev.com/bcc4ce71b07ea71a1056c3227c5bc381c52256fd
Cr-Commit-Position: refs/heads/master@{#352036}

[dmurph, 2015-10-02 17:06:31]

On 2015/10/02 at 14:59:33, commit-bot wrote:
> Patchset 17 (id:??) landed as
https://crrev.com/bcc4ce71b07ea71a1056c3227c5bc381c52256fd
> Cr-Commit-Position: refs/heads/master@{#352036}

This causes failusre in ASAN bot, use after free,

see crbug.com/538660

[dmurph, 2015-10-02 17:07:37]

A revert of this CL (patchset #17 id:310001) has been created in
https://codereview.chromium.org/1379543003/ by dmurph@chromium.org.

The reason for reverting is: BUG=538660

HTTP parsing use-after-free issues..

[alex clarke (OOO till 29th), 2015-10-07 15:43:51]

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

[alex clarke (OOO till 29th), 2015-10-07 15:44:48]

I fixed a UAF in BackgroundHtmlPArser caused by an incorrect lifetime assumption
about the loading task runner.

PTAL

[commit-bot: I haz the power, 2015-10-07 15:45:00]

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

[Sami, 2015-10-07 15:57:52]

lgtm.

[commit-bot: I haz the power, 2015-10-07 17:00:52]

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

[commit-bot: I haz the power, 2015-10-07 17:00:53]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2015-10-07 17:03:29]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-10-07 17:03:29]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1366883002/#ps330001
(title: "Fix UAF in BackgroundHTMLParser")

[commit-bot: I haz the power, 2015-10-07 17:05:34]

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

[commit-bot: I haz the power, 2015-10-07 17:16:03]

Committed patchset #18 (id:330001)

[commit-bot: I haz the power, 2015-10-07 17:16:56]

Patchset 18 (id:??) landed as
https://crrev.com/f3080498facd16066076e7d459ba7cfda11e582c
Cr-Commit-Position: refs/heads/master@{#352866}

[dcheng, 2015-11-26 19:15:22]

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

[dcheng, 2015-11-26 19:15:24]

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

https://codereview.chromium.org/1366883002/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1075:
m_frame->updateFrameSecurityOrigin();
Why was this line added? Why isn't Document::updateSecurityOrigin() good enough?
After all, that's where the SecurityOrigin actually gets set.

[alex clarke (OOO till 29th), 2015-11-27 10:18:01]

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

https://codereview.chromium.org/1366883002/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1075:
m_frame->updateFrameSecurityOrigin();
On 2015/11/26 19:15:24, dcheng wrote:
> Why was this line added? Why isn't Document::updateSecurityOrigin() good
enough?
> After all, that's where the SecurityOrigin actually gets set.

This line was added before the one in Document::updateSecurityOrigin.  It's
entirely possible it's not actually needed now.  

We noticed there where situations where queues didn't have the right origin in
tracing which was fixed by adding code in Document::updateSecurityOrigin, it
didn't occur to us to try and remove this line.