Move InspectorTask handling from MainThreadTaskRunner to Document

Move InspectorTask handling from MainThreadTaskRunner to Document

MainThreadTaskRunner handles the task suspension, cancellation, and
instrumentation of Inspector for a posted task. However, a task posted
via MainThreadTaskRunner::postInspectorTask doesn't need the task
suspension and instrumentation.

This CL moves InspectorTask handling from MainThreadTaskRunner to
Document for smaller MainThreadTaskRunner.

BUG=667310
Committed: https://crrev.com/1f1dcb17059558c87c9e6f8ee45888de1f71c2d3
Cr-Commit-Position: refs/heads/master@{#435889}

[tzik, 2016-11-28 07:23:10]

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

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

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

[tzik, 2016-11-28 08:08:16]

Description was changed from

==========
Move InspectorTask handling from MainThreadTaskRunner to Document

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

to

==========
Move InspectorTask handling from MainThreadTaskRunner to Document

MainThreadTaskRunner handles the task suspension, cancellation, and
instrumentation of Inspector for a posted task. However, a task posted
via MainThreadTaskRunner::postInspectorTask doesn't need the task
suspension and instrumentation.

This CL moves InspectorTask handling from MainThreadTaskRunner to
Document for smaller MainThreadTaskRunner.
==========

[tzik, 2016-11-28 08:08:51]

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

[tzik, 2016-11-28 08:08:52]

PTAL

[commit-bot: I haz the power, 2016-11-28 08:19:36]

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

[commit-bot: I haz the power, 2016-11-28 08:19:37]

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

[tzik, 2016-11-28 08:30:37]

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

[commit-bot: I haz the power, 2016-11-28 08:30:46]

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-11-28 10:15:28]

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

[commit-bot: I haz the power, 2016-11-28 10:15:29]

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

[haraken, 2016-11-28 11:00:43]

https://codereview.chromium.org/2534803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2534803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:5714:
wrapCrossThreadWeakPersistent(this),

Does this need to be cross-thread?

If this really needs to be cross-thread, it means that a non-main thread is
accessing a Document object. Document is a main-thread's thing, so it should be
problematic...

[tzik, 2016-11-29 01:52:38]

https://codereview.chromium.org/2534803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2534803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:5714:
wrapCrossThreadWeakPersistent(this),
On 2016/11/28 11:00:43, haraken wrote:
> 
> Does this need to be cross-thread?
> 
> If this really needs to be cross-thread, it means that a non-main thread is
> accessing a Document object. Document is a main-thread's thing, so it should
be
> problematic...
> 

Yes, this needs to be cross-thread. In addition, there are only 4 caller of this
function, and all of them are cross-thread.

[haraken, 2016-11-29 02:00:45]

On 2016/11/29 01:52:38, tzik wrote:
>
https://codereview.chromium.org/2534803002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2534803002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:5714:
> wrapCrossThreadWeakPersistent(this),
> On 2016/11/28 11:00:43, haraken wrote:
> > 
> > Does this need to be cross-thread?
> > 
> > If this really needs to be cross-thread, it means that a non-main thread is
> > accessing a Document object. Document is a main-thread's thing, so it should
> be
> > problematic...
> > 
> 
> Yes, this needs to be cross-thread. In addition, there are only 4 caller of
this
> function, and all of them are cross-thread.

Why can they access Document safely? Conceptually Document is an
only-main-thread thing. Maybe is it safe because it's somehow guaranteed that
the Document is alive while the cross-thread callers are accessing the Document?

Is there any option to replace the callers with ParentFrameTaskRunner?

[tzik, 2016-11-30 08:16:30]

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

[commit-bot: I haz the power, 2016-11-30 08:17:23]

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

[tzik, 2016-11-30 08:44:29]

On 2016/11/29 02:00:45, haraken wrote:
> On 2016/11/29 01:52:38, tzik wrote:
> >
>
https://codereview.chromium.org/2534803002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/dom/Document.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2534803002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/dom/Document.cpp:5714:
> > wrapCrossThreadWeakPersistent(this),
> > On 2016/11/28 11:00:43, haraken wrote:
> > > 
> > > Does this need to be cross-thread?
> > > 
> > > If this really needs to be cross-thread, it means that a non-main thread
is
> > > accessing a Document object. Document is a main-thread's thing, so it
should
> > be
> > > problematic...
> > > 
> > 
> > Yes, this needs to be cross-thread. In addition, there are only 4 caller of
> this
> > function, and all of them are cross-thread.
> 
> Why can they access Document safely? Conceptually Document is an
> only-main-thread thing. Maybe is it safe because it's somehow guaranteed that
> the Document is alive while the cross-thread callers are accessing the
Document?
> 
> Is there any option to replace the callers with ParentFrameTaskRunner?

The safety is guaranteed by a strong reference to Document they have.

Yeah, we can use ParentFrameTaskRunner to avoid using Document here. I
originally planned to move there after this CL, but it seems cleaner to do it in
this CL.

[haraken, 2016-11-30 08:55:25]

Mostly LG.

Do you know why these inspector tasks need to be unthrottled? Unthrottled tasks
should be limited to things that really need to be unthrottled for some special
reason.

[tzik, 2016-11-30 09:42:43]

On 2016/11/30 08:55:25, haraken wrote:
> Mostly LG.
> 
> Do you know why these inspector tasks need to be unthrottled? Unthrottled
tasks
> should be limited to things that really need to be unthrottled for some
special
> reason.

These tasks need to run even when the page is suspended. Since the inspector
suspends the page while it is paused on a JS break point, and it posts a task as
an inspector task to run a function specified in the Inspector console.

[haraken, 2016-11-30 09:44:00]

On 2016/11/30 09:42:43, tzik wrote:
> On 2016/11/30 08:55:25, haraken wrote:
> > Mostly LG.
> > 
> > Do you know why these inspector tasks need to be unthrottled? Unthrottled
> tasks
> > should be limited to things that really need to be unthrottled for some
> special
> > reason.
> 
> These tasks need to run even when the page is suspended. Since the inspector
> suspends the page while it is paused on a JS break point, and it posts a task
as
> an inspector task to run a function specified in the Inspector console.

ok, LGTM

[tzik, 2016-11-30 10:37:28]

The CQ bit was unchecked by tzik@chromium.org

[tzik, 2016-11-30 10:37:33]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2016-11-30 10:37:49]

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-11-30 11:05:04]

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

[commit-bot: I haz the power, 2016-11-30 11:05:06]

Failed to apply patch for
third_party/WebKit/Source/core/workers/ThreadedWorkletObjectProxy.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/core/workers/ThreadedWorkletObjectProxy.cpp:40
  error: third_party/WebKit/Source/core/workers/ThreadedWorkletObjectProxy.cpp:
patch does not apply

Patch:      
third_party/WebKit/Source/core/workers/ThreadedWorkletObjectProxy.cpp
Index: third_party/WebKit/Source/core/workers/ThreadedWorkletObjectProxy.cpp
diff --git
a/third_party/WebKit/Source/core/workers/ThreadedWorkletObjectProxy.cpp
b/third_party/WebKit/Source/core/workers/ThreadedWorkletObjectProxy.cpp
index
f8fc183a13648a5b9d3bb91d3c9df58aea68b991..a64511feb6f08280cb9aa16b36b38b9e1202905d
100644
--- a/third_party/WebKit/Source/core/workers/ThreadedWorkletObjectProxy.cpp
+++ b/third_party/WebKit/Source/core/workers/ThreadedWorkletObjectProxy.cpp
@@ -10,6 +10,7 @@
 #include "core/dom/ExecutionContext.h"
 #include "core/dom/ExecutionContextTask.h"
 #include "core/inspector/ConsoleMessage.h"
+#include "core/workers/ParentFrameTaskRunners.h"
 #include "core/workers/ThreadedWorkletMessagingProxy.h"
 #include "wtf/Functional.h"
 #include "wtf/PtrUtil.h"
@@ -40,14 +41,12 @@ void ThreadedWorkletObjectProxy::reportConsoleMessage(
 void ThreadedWorkletObjectProxy::postMessageToPageInspector(
     const String& message) {
   DCHECK(getExecutionContext()->isDocument());
-  // TODO(nhiroki): Replace this with getParentFrameTaskRunners().
-  // (https://crbug.com/667310)
-  toDocument(getExecutionContext())
-      ->postInspectorTask(
-          BLINK_FROM_HERE,
-          createCrossThreadTask(
-              &ThreadedWorkletMessagingProxy::postMessageToPageInspector,
-              crossThreadUnretained(m_messagingProxy), message));
+  getParentFrameTaskRunners()
+      ->get(TaskType::Unthrottled)
+      ->postTask(BLINK_FROM_HERE,
+                 crossThreadBind(
+                    
&ThreadedWorkletMessagingProxy::postMessageToPageInspector,
+                     crossThreadUnretained(m_messagingProxy), message));
 }
 
 ParentFrameTaskRunners*

[nhiroki, 2016-11-30 14:46:36]

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

[nhiroki, 2016-11-30 14:46:36]

drive-by comments

https://codereview.chromium.org/2534803002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
(right):

https://codereview.chromium.org/2534803002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:144:
return;
We might want to replace this with DCHECK because we haven't supported nested
workers yet and a parent execution context should always be a document.

https://codereview.chromium.org/2534803002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:146:
->get(TaskType::Unthrottled)
Can you add a comment about why we prefer Unthrottled here and elsewhere?

[tzik, 2016-12-01 07:04:12]

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

[commit-bot: I haz the power, 2016-12-01 07:04:24]

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

[tzik, 2016-12-01 07:12:38]

https://codereview.chromium.org/2534803002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
(right):

https://codereview.chromium.org/2534803002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:144:
return;
On 2016/11/30 14:46:36, nhiroki (OOO until Dec 2) wrote:
> We might want to replace this with DCHECK because we haven't supported nested
> workers yet and a parent execution context should always be a document.

Done.

https://codereview.chromium.org/2534803002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:146:
->get(TaskType::Unthrottled)
On 2016/11/30 14:46:36, nhiroki (OOO until Dec 2) wrote:
> Can you add a comment about why we prefer Unthrottled here and elsewhere?

Done.

[commit-bot: I haz the power, 2016-12-01 08:30:27]

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

[commit-bot: I haz the power, 2016-12-01 08:30:28]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2016-12-01 09:10:32]

Thank you. LGTM

[tzik, 2016-12-01 11:27:58]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-12-01 11:27:59]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2534803002/#ps60001
(title: "+DCHECK. +comment")

[commit-bot: I haz the power, 2016-12-01 11:28:14]

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-01 11:34:00]

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

[commit-bot: I haz the power, 2016-12-01 11:34:01]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tzik, 2016-12-02 02:58:01]

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

[commit-bot: I haz the power, 2016-12-02 02:58:37]

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

[nhiroki, 2016-12-02 03:50:34]

Description was changed from

==========
Move InspectorTask handling from MainThreadTaskRunner to Document

MainThreadTaskRunner handles the task suspension, cancellation, and
instrumentation of Inspector for a posted task. However, a task posted
via MainThreadTaskRunner::postInspectorTask doesn't need the task
suspension and instrumentation.

This CL moves InspectorTask handling from MainThreadTaskRunner to
Document for smaller MainThreadTaskRunner.
==========

to

==========
Move InspectorTask handling from MainThreadTaskRunner to Document

MainThreadTaskRunner handles the task suspension, cancellation, and
instrumentation of Inspector for a posted task. However, a task posted
via MainThreadTaskRunner::postInspectorTask doesn't need the task
suspension and instrumentation.

This CL moves InspectorTask handling from MainThreadTaskRunner to
Document for smaller MainThreadTaskRunner.

BUG=667310
==========

[tzik, 2016-12-02 03:51:12]

The CQ bit was unchecked by tzik@chromium.org

[tzik, 2016-12-02 03:51:17]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-12-02 03:51:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2534803002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-02 03:51: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-12-02 06:19:49]

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

[commit-bot: I haz the power, 2016-12-02 06:19:51]

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

[tzik, 2016-12-02 07:16:53]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2016-12-02 07:17:13]

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-02 08:52:19]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480663013766600,
"parent_rev": "c9f134b0a1c103f16d729768fafee9ae9910e642", "commit_rev":
"c4bbd41dcbf836ec1035c6338e2d8eff320356a3"}

[commit-bot: I haz the power, 2016-12-02 08:53:31]

Description was changed from

==========
Move InspectorTask handling from MainThreadTaskRunner to Document

MainThreadTaskRunner handles the task suspension, cancellation, and
instrumentation of Inspector for a posted task. However, a task posted
via MainThreadTaskRunner::postInspectorTask doesn't need the task
suspension and instrumentation.

This CL moves InspectorTask handling from MainThreadTaskRunner to
Document for smaller MainThreadTaskRunner.

BUG=667310
==========

to

==========
Move InspectorTask handling from MainThreadTaskRunner to Document

MainThreadTaskRunner handles the task suspension, cancellation, and
instrumentation of Inspector for a posted task. However, a task posted
via MainThreadTaskRunner::postInspectorTask doesn't need the task
suspension and instrumentation.

This CL moves InspectorTask handling from MainThreadTaskRunner to
Document for smaller MainThreadTaskRunner.

BUG=667310
==========

[commit-bot: I haz the power, 2016-12-02 08:53:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-02 08:55:30]

Description was changed from

==========
Move InspectorTask handling from MainThreadTaskRunner to Document

MainThreadTaskRunner handles the task suspension, cancellation, and
instrumentation of Inspector for a posted task. However, a task posted
via MainThreadTaskRunner::postInspectorTask doesn't need the task
suspension and instrumentation.

This CL moves InspectorTask handling from MainThreadTaskRunner to
Document for smaller MainThreadTaskRunner.

BUG=667310
==========

to

==========
Move InspectorTask handling from MainThreadTaskRunner to Document

MainThreadTaskRunner handles the task suspension, cancellation, and
instrumentation of Inspector for a posted task. However, a task posted
via MainThreadTaskRunner::postInspectorTask doesn't need the task
suspension and instrumentation.

This CL moves InspectorTask handling from MainThreadTaskRunner to
Document for smaller MainThreadTaskRunner.

BUG=667310

Committed: https://crrev.com/1f1dcb17059558c87c9e6f8ee45888de1f71c2d3
Cr-Commit-Position: refs/heads/master@{#435889}
==========

[commit-bot: I haz the power, 2016-12-02 08:55:31]

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