[DevTools] Show stack trace for exceptions in dedicated workers

[DevTools] Show stack trace for exceptions in dedicated workers

BUG=431274
R=vsevik@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187386

[kozy, 2014-11-20 14:12:17]

Vsevolod, ptal!

[vsevik, 2014-11-24 09:51:34]

The code looks fine, the test failure here seems to be an actual bug though.

We should also add a test for this.

Could you also file the bugs for adding better support for this for
service/shared worker? (you might want to check how it currently works before)

https://codereview.chromium.org/743153002/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/743153002/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.cpp:65: static unsigned long
s_workerExceptionUniqueIdentifier = 0;
Let's put these into an anonymous namespace.

[kozy, 2014-12-01 13:08:19]

Patchset #3 (id:40001) has been deleted

[kozy, 2014-12-01 13:11:29]

On 2014/11/24 09:51:34, vsevik wrote:
> We should also add a test for this.
Done.

> Could you also file the bugs for adding better support for this for
> service/shared worker? (you might want to check how it currently works before)
Ok, I am going to check it.

[kozy, 2014-12-01 13:11:55]

https://codereview.chromium.org/743153002/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/743153002/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.cpp:65: static unsigned long
s_workerExceptionUniqueIdentifier = 0;
On 2014/11/24 09:51:34, vsevik wrote:
> Let's put these into an anonymous namespace.

Done.

[vsevik, 2014-12-01 15:06:03]

lgtm

https://codereview.chromium.org/743153002/diff/60001/LayoutTests/inspector/co...
File LayoutTests/inspector/console/worker-exception-message-contains-stack.html
(right):

https://codereview.chromium.org/743153002/diff/60001/LayoutTests/inspector/co...
LayoutTests/inspector/console/worker-exception-message-contains-stack.html:31:
InspectorTest.expandConsoleMessages(step3);
I'd like to see both cases tested here: when the error is and is not handled in
the main thread.
Also, please make sure this test actually tests that the message was printed on
the worker thread (or stack trace presence already proves that? I am not sure)

https://codereview.chromium.org/743153002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/743153002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.cpp:350:
addMessageToWorkerConsole(consoleMessage.release());
Let's call addConsoleMessage instead.

[kozy, 2014-12-01 16:17:05]

https://codereview.chromium.org/743153002/diff/60001/LayoutTests/inspector/co...
File LayoutTests/inspector/console/worker-exception-message-contains-stack.html
(right):

https://codereview.chromium.org/743153002/diff/60001/LayoutTests/inspector/co...
LayoutTests/inspector/console/worker-exception-message-contains-stack.html:31:
InspectorTest.expandConsoleMessages(step3);
On 2014/12/01 15:06:02, vsevik wrote:
> I'd like to see both cases tested here: when the error is and is not handled
in
> the main thread.
> Also, please make sure this test actually tests that the message was printed
on
> the worker thread (or stack trace presence already proves that? I am not sure)

Done.
And yes, stack trace with correct urls already proves correct message thread.

https://codereview.chromium.org/743153002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/743153002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.cpp:350:
addMessageToWorkerConsole(consoleMessage.release());
On 2014/12/01 15:06:03, vsevik wrote:
> Let's call addConsoleMessage instead.

Done.

[kozy, 2014-12-02 12:12:59]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2014-12-02 12:13:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/743153002/100001

[commit-bot: I haz the power, 2014-12-02 12:22:00]

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

[commit-bot: I haz the power, 2014-12-02 12:22:01]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[kozy, 2014-12-02 12:47:57]

Vsevolod, PTAL.

Removed one line from LayoutTests/fast/workers/worker-onerror-04-expected.txt. I
suppose in this test case we shouldn't print console message about error in
worker's script and can use only onerror handler for reporting.

[vsevik, 2014-12-02 14:58:59]

https://codereview.chromium.org/743153002/diff/100001/LayoutTests/fast/worker...
File LayoutTests/fast/workers/worker-onerror-04-expected.txt (left):

https://codereview.chromium.org/743153002/diff/100001/LayoutTests/fast/worker...
LayoutTests/fast/workers/worker-onerror-04-expected.txt:1: CONSOLE ERROR: line
7: Uncaught ReferenceError: foo is not defined
Can you explain this? Is this even right?

[kozy, 2014-12-17 14:37:01]

On 2014/12/02 14:58:59, vsevik wrote:
>
https://codereview.chromium.org/743153002/diff/100001/LayoutTests/fast/worker...
> File LayoutTests/fast/workers/worker-onerror-04-expected.txt (left):
> 
>
https://codereview.chromium.org/743153002/diff/100001/LayoutTests/fast/worker...
> LayoutTests/fast/workers/worker-onerror-04-expected.txt:1: CONSOLE ERROR: line
> 7: Uncaught ReferenceError: foo is not defined
> Can you explain this? Is this even right?

Our test runner has special object for printing results:
WebKitTestResultPrinter. This class prints messages from embedder if its state
is DURING_TEST. But because we send our message to embedder throw postTask to
main thread, test has already finished when we print this message. It's problem
with our test runner and test check other details of worker implementation.

[kozy, 2014-12-17 14:38:26]

kozyatinskiy@chromium.org changed reviewers:
+ pfeldman@chromium.org

[kozy, 2014-12-17 14:40:10]

Pavel,ptal. I need owner review for Source/web/* files.

[pfeldman, 2014-12-17 14:40:45]

lgtm

[kozy, 2014-12-17 15:53:40]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2014-12-17 15:54:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/743153002/120001

[commit-bot: I haz the power, 2014-12-17 17:06:56]

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