DevTools: log worker thread ids on the main thread when Timeline recording is enabled

DevTools: log worker thread ids on the main thread when Timeline recording is enabled

DevTools uses TracingStartedInWorker trace event to figure out which worker threads correspond to the inspected page. The events were written on the worker thread which is racy. Current implementation always writes the event on the main thread. Since Tracing.start/end commands are handled on the main thread this eliminates racy behavior.

BUG=401895, 405429
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181820

[yurys, 2014-09-09 13:40:36]

yurys@chromium.org changed reviewers:
+ alph@chromium.org, caseq@chromium.org, loislo@chromium.org,
sergeyv@chromium.org

[loislo, 2014-09-09 13:59:44]

https://codereview.chromium.org/552323002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/552323002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorTraceEvents.cpp:363:
value->setDouble("workerThreadId", workerThread->platformThreadId());
do we have a guaranty that workerThread object is alive at the time when we do
the serialization?

https://codereview.chromium.org/552323002/diff/1/Source/core/workers/WorkerIn...
File Source/core/workers/WorkerInspectorProxy.cpp (right):

https://codereview.chromium.org/552323002/diff/1/Source/core/workers/WorkerIn...
Source/core/workers/WorkerInspectorProxy.cpp:94:
TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("devtools.timeline"),
"TracingStartedInWorker", "data",
InspectorTracingStartedInWorkerEvent::data(sessionId, m_workerThread));
I think with this schema the event should have different name.

https://codereview.chromium.org/552323002/diff/1/Source/core/workers/WorkerTh...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/552323002/diff/1/Source/core/workers/WorkerTh...
Source/core/workers/WorkerThread.cpp:241: if (!m_thread)
Looks like it could be converted into ASSERT

[yurys, 2014-09-09 14:04:28]

https://codereview.chromium.org/552323002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/552323002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorTraceEvents.cpp:363:
value->setDouble("workerThreadId", workerThread->platformThreadId());
On 2014/09/09 13:59:44, loislo wrote:
> do we have a guaranty that workerThread object is alive at the time when we do
> the serialization?

We don't need the thread to be alive there as we read thread id synchronously
when the event is written.

[loislo, 2014-09-09 14:14:08]

https://codereview.chromium.org/552323002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/552323002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorTraceEvents.cpp:363:
value->setDouble("workerThreadId", workerThread->platformThreadId());
On 2014/09/09 14:04:27, yurys wrote:
> On 2014/09/09 13:59:44, loislo wrote:
> > do we have a guaranty that workerThread object is alive at the time when we
do
> > the serialization?
> 
> We don't need the thread to be alive there as we read thread id synchronously
> when the event is written.
ok

[yurys, 2014-09-09 14:46:34]

https://codereview.chromium.org/552323002/diff/1/Source/core/workers/WorkerIn...
File Source/core/workers/WorkerInspectorProxy.cpp (right):

https://codereview.chromium.org/552323002/diff/1/Source/core/workers/WorkerIn...
Source/core/workers/WorkerInspectorProxy.cpp:94:
TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("devtools.timeline"),
"TracingStartedInWorker", "data",
InspectorTracingStartedInWorkerEvent::data(sessionId, m_workerThread));
On 2014/09/09 13:59:44, loislo wrote:
> I think with this schema the event should have different name.

Done. Renamed to TracingSessionIdForWorker

[loislo, 2014-09-09 14:47:00]

lgtm

[yurys, 2014-09-09 15:08:37]

https://codereview.chromium.org/552323002/diff/1/Source/core/workers/WorkerTh...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/552323002/diff/1/Source/core/workers/WorkerTh...
Source/core/workers/WorkerThread.cpp:241: if (!m_thread)
On 2014/09/09 13:59:44, loislo wrote:
> Looks like it could be converted into ASSERT

In theory WorkerThread::start can be called asynchronously after WorkerThread
constructor so I'd rather leave this check. It might make sense to merge
WorkerThread::start into the constructor.

[yurys, 2014-09-09 15:09:40]

Waiting for https://codereview.chromium.org/515243002/ to land as this CL
depends on it.

[yurys, 2014-09-11 12:14:43]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-09-11 12:15:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/552323002/20001

[commit-bot: I haz the power, 2014-09-11 12:15:37]

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

[commit-bot: I haz the power, 2014-09-11 12:15:39]

Failed to apply patch for Source/devtools/front_end/sdk/TracingModel.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/devtools/front_end/sdk/TracingModel.js
  |diff --git a/Source/devtools/front_end/sdk/TracingModel.js
b/Source/devtools/front_end/sdk/TracingModel.js
  |index
f05b1aa643a242467eb29d5f246f14e38128965b..2616132101281c389393881c8be90d9b2c47d77d
100644
  |--- a/Source/devtools/front_end/sdk/TracingModel.js
  |+++ b/Source/devtools/front_end/sdk/TracingModel.js
  --------------------------
  No file to patch.  Skipping patch.
  5 out of 5 hunks ignored

Patch:       Source/devtools/front_end/sdk/TracingModel.js
Index: Source/devtools/front_end/sdk/TracingModel.js
diff --git a/Source/devtools/front_end/sdk/TracingModel.js
b/Source/devtools/front_end/sdk/TracingModel.js
index
f05b1aa643a242467eb29d5f246f14e38128965b..2616132101281c389393881c8be90d9b2c47d77d
100644
--- a/Source/devtools/front_end/sdk/TracingModel.js
+++ b/Source/devtools/front_end/sdk/TracingModel.js
@@ -76,7 +76,7 @@ WebInspector.TracingModel.FrameLifecycleEventCategory =
"cc,devtools";
 
 WebInspector.TracingModel.DevToolsMetadataEvent = {
     TracingStartedInPage: "TracingStartedInPage",
-    TracingStartedInWorker: "TracingStartedInWorker",
+    TracingSessionIdForWorker: "TracingSessionIdForWorker",
 };
 
 /**
@@ -260,7 +260,7 @@ WebInspector.TracingModel.prototype = {
                 event.category ===
WebInspector.TracingModel.DevToolsMetadataEventCategory) {
                 this._devtoolsPageMetadataEvents.push(event);
             }
-            if (event && event.name ===
WebInspector.TracingModel.DevToolsMetadataEvent.TracingStartedInWorker &&
+            if (event && event.name ===
WebInspector.TracingModel.DevToolsMetadataEvent.TracingSessionIdForWorker &&
                 event.category ===
WebInspector.TracingModel.DevToolsMetadataEventCategory) {
                 this._devtoolsWorkerMetadataEvents.push(event);
             }
@@ -295,7 +295,11 @@ WebInspector.TracingModel.prototype = {
         var mismatchingIds = {};
         function checkSessionId(event)
         {
-            var id = event.args["sessionId"];
+            var args = event.args;
+            // FIXME: put sessionId into args["data"] for TracingStartedInPage
event.
+            if (args["data"])
+                args = args["data"];
+            var id = args["sessionId"];
             if (id === sessionId)
                 return true;
             mismatchingIds[id] = true;
@@ -697,6 +701,7 @@ WebInspector.TracingModel.Thread = function(process, id)
     this._setName("Thread " + id);
     this._events = [];
     this._asyncEvents = [];
+    this._id = id;
 
     this._stack = [];
 }
@@ -748,6 +753,14 @@ WebInspector.TracingModel.Thread.prototype = {
     },
 
     /**
+     * @return {number}
+     */
+    id: function()
+    {
+        return this._id;
+    },
+
+    /**
      * @return {!WebInspector.TracingModel.Process}
      */
     process: function()

[yurys, 2014-09-11 12:23:30]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-09-11 12:24:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/552323002/40001

[commit-bot: I haz the power, 2014-09-11 13:27:10]

Committed patchset #3 (id:40001) as 181820