[Blink-Workers]Set verbosity of the external exception handler.

[Blink-Workers]Set verbosity of the external exception handler.

In case of workers, by default exceptions that are 
caught by an external exception handler are not 
reported. Call SetVerbose with true to report them.

BUG=590219

[sivag, 2016-06-24 08:24:49]

Description was changed from

==========
Set verbosity/CaptureMessage of the external exception handler.


BUG=590219
==========

to

==========
Set verbosity/CaptureMessage of the external exception handler.


BUG=590219
==========

[sivag, 2016-06-24 08:24:49]

siva.gunturi@samsung.com changed reviewers:
+ nhiroki@chromium.org

[sivag, 2016-06-24 08:25:31]

The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 08:25:44]

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

[commit-bot: I haz the power, 2016-06-24 09:02:37]

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

[commit-bot: I haz the power, 2016-06-24 09:02:38]

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

[sivag, 2016-06-24 10:02:32]

Description was changed from

==========
Set verbosity/CaptureMessage of the external exception handler.


BUG=590219
==========

to

==========
Set verbosity of the external exception handler.

For workers by default, exceptions that are caught 
by an external exception handler are not reported.
Call SetVerbose with true to report them.

BUG=590219
==========

[sivag, 2016-06-24 10:03:03]

siva.gunturi@samsung.com changed reviewers:
+ vivekg@chromium.org

[sivag, 2016-06-24 10:03:59]

Description was changed from

==========
Set verbosity of the external exception handler.

For workers by default, exceptions that are caught 
by an external exception handler are not reported.
Call SetVerbose with true to report them.

BUG=590219
==========

to

==========
Set verbosity of the external exception handler.

In case of workers, by default exceptions that are 
caught by an external exception handler are not 
reported. Call SetVerbose with true to report them.

BUG=590219
==========

[sivag, 2016-06-24 10:05:18]

Description was changed from

==========
Set verbosity of the external exception handler.

In case of workers, by default exceptions that are 
caught by an external exception handler are not 
reported. Call SetVerbose with true to report them.

BUG=590219
==========

to

==========
[Blink-Workers]Set verbosity of the external exception handler.

In case of workers, by default exceptions that are 
caught by an external exception handler are not 
reported. Call SetVerbose with true to report them.

BUG=590219
==========

[sivag, 2016-06-24 11:34:43]

Few layout tests are failing after this change, i am trying to fix those.

[nhiroki, 2016-06-28 07:34:38]

Sorry for my delayed reply and thank you for working on this!

This change looks good to me. I'm not familiar with bindings/V8 API, so please
ask an expert on them to review this.

[haraken, 2016-06-28 08:00:24]

This change makes sense. We're setting the verbose flag on the main thread
already. LGTM.

[sivag, 2016-07-07 07:34:26]

siva.gunturi@samsung.com changed reviewers:
+ mkwst@chromium.org, ojan@chromium.org

[sivag, 2016-07-07 07:35:57]

By enabling verbose mode, few of the worker tests needs to be changed. I just
modified a few tests here. Ptal and share your views.

[nhiroki, 2016-07-20 01:57:46]

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

[commit-bot: I haz the power, 2016-07-20 01:58:34]

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-07-20 03:20:45]

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

[commit-bot: I haz the power, 2016-07-20 03:20:45]

Dry run: 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_...)

[Mike West, 2016-07-20 07:26:51]

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

[Mike West, 2016-07-20 07:26:52]

The test failures look relevant.

[sivag, 2016-07-20 07:30:25]

On 2016/07/20 07:26:52, Mike West wrote:
> The test failures look relevant.

hi mike,

In the patch i enabled the verbose flag
so few tests are failing. In above patch i fixed few of the failing tests.
can you let me know if the changes look fine?

[sivag, 2016-07-22 12:26:09]

The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 12:26:22]

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

[sivag, 2016-07-22 12:37:08]

siva.gunturi@samsung.com changed reviewers:
+ kinuko@chromium.org

[commit-bot: I haz the power, 2016-07-22 15:02:55]

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

[commit-bot: I haz the power, 2016-07-22 15:02:56]

Dry run: This issue passed the CQ dry run.

[sivag, 2016-07-22 15:39:30]

ptal Layouttests updated.

[sivag, 2016-07-25 06:31:46]

@ojan, mike

can you ptal.. layouttests?

[nhiroki, 2016-07-25 06:35:12]

I'll take a look again.

[nhiroki, 2016-07-25 08:30:34]

https://codereview.chromium.org/2090953006/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/workers/worker-onerror-05.html (left):

https://codereview.chromium.org/2090953006/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/workers/worker-onerror-05.html:21: return
false;
After SetVerbose(true), an error handler on Worker context propagates an error
to a handler on Window context (according to the spec[1], this would be correct
behavior). This results in 2 error handler calls on Window context and 1 error
handler call on Worker. This mismatches with a test expectation.

Probably we should prevent the firing of Window's event handler by returning
true here.

[1] https://html.spec.whatwg.org/multipage/workers.html#runtime-script-errors-2
"If the event is not canceled, the user agent must act as if the uncaught
runtime script error had occurred in the global scope that the Worker object is
in, thus repeating the entire runtime script error reporting process one level
up."

[sivag, 2016-07-25 11:27:30]

The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 11:27:42]

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-07-25 13:17:29]

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

[commit-bot: I haz the power, 2016-07-25 13:17:30]

Dry run: 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_...)

[sivag, 2016-07-25 13:34:04]

ptal..

https://codereview.chromium.org/2090953006/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/workers/worker-onerror-05.html (left):

https://codereview.chromium.org/2090953006/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/workers/worker-onerror-05.html:21: return
false;
On 2016/07/25 08:30:34, nhiroki (slow) wrote:
> After SetVerbose(true), an error handler on Worker context propagates an error
> to a handler on Window context (according to the spec[1], this would be
correct
> behavior). This results in 2 error handler calls on Window context and 1 error
> handler call on Worker. This mismatches with a test expectation.
> 
> Probably we should prevent the firing of Window's event handler by returning
> true here.
> 
> [1]
https://html.spec.whatwg.org/multipage/workers.html#runtime-script-errors-2
> "If the event is not canceled, the user agent must act as if the uncaught
> runtime script error had occurred in the global scope that the Worker object
is
> in, thus repeating the entire runtime script error reporting process one level
> up."

Done.

[nhiroki, 2016-07-25 15:09:30]

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

[commit-bot: I haz the power, 2016-07-25 15:09:45]

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

[nhiroki, 2016-07-25 15:11:00]

The CQ bit was unchecked by nhiroki@chromium.org

[ojan, 2016-07-25 21:54:45]

ojan@chromium.org changed reviewers:
- ojan@chromium.org

[ojan, 2016-07-25 21:54:46]

I'm probably not a good reviewer for this change. Removing myself. I think the
current list of reviewers is good and I'm not needed. :)

[nhiroki, 2016-07-26 01:59:12]

I'm a bit confused about SetVerbose(true). At first, I assumed that it's
necessary to print more specific error messages on initial worker script
evaluation as issue 590219 mentions. However, according to the CL description
and a header comment of the function[1], it's used for reporting an exception
already caught by an external exception handler and not intended for making
error message verbose. This would be the reason why these tests are broken.

Is there another way to refine error messages without such a side-effect? As far
as I can see, when SetVerbose(true) is enabled, message seems to be tweaked in
MessageHandler::ReportMessage()[2]. We could do the similar thing even when the
verbose flag is not set.

[1]
https://cs.chromium.org/chromium/src/v8/include/v8.h?sq=package:chromium&dr=C...
[2]
https://cs.chromium.org/chromium/src/v8/src/messages.cc?sq=package:chromium&d...

https://codereview.chromium.org/2090953006/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/workers/worker-onerror-05.html (left):

https://codereview.chromium.org/2090953006/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/workers/worker-onerror-05.html:21: return
false;
On 2016/07/25 13:34:03, sivag wrote:
> On 2016/07/25 08:30:34, nhiroki (slow) wrote:
> > After SetVerbose(true), an error handler on Worker context propagates an
error
> > to a handler on Window context (according to the spec[1], this would be
> correct
> > behavior). This results in 2 error handler calls on Window context and 1
error
> > handler call on Worker. This mismatches with a test expectation.
> > 
> > Probably we should prevent the firing of Window's event handler by returning
> > true here.
> > 
> > [1]
> https://html.spec.whatwg.org/multipage/workers.html#runtime-script-errors-2
> > "If the event is not canceled, the user agent must act as if the uncaught
> > runtime script error had occurred in the global scope that the Worker object
> is
> > in, thus repeating the entire runtime script error reporting process one
level
> > up."
> 
> Done.

Hmmm... sorry, I read the spec[1] again and realized I misunderstood the problem
and spec. I thought 'onerror' on Window context is always called regardless of
whether 'onerror' on Worker context is canceled. It was not correct. 'onerror'
on Window context must be called only when 'onerror' on Worker context is not
canceled like the original 'worker-onerror-05.html'.

Therefore, returning false here is correct...

https://codereview.chromium.org/2090953006/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/workers/worker-constructor.html
(right):

https://codereview.chromium.org/2090953006/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/workers/worker-constructor.html:16:
"testEmptyScriptUrl",
Why did you move this test case?

[haraken, 2016-07-26 11:44:16]

On 2016/07/26 01:59:12, nhiroki (slow) wrote:
> I'm a bit confused about SetVerbose(true). At first, I assumed that it's
> necessary to print more specific error messages on initial worker script
> evaluation as issue 590219 mentions. However, according to the CL description
> and a header comment of the function[1], it's used for reporting an exception
> already caught by an external exception handler and not intended for making
> error message verbose. This would be the reason why these tests are broken.
> 
> Is there another way to refine error messages without such a side-effect? As
far
> as I can see, when SetVerbose(true) is enabled, message seems to be tweaked in
> MessageHandler::ReportMessage()[2]. We could do the similar thing even when
the
> verbose flag is not set.
> 
> [1]
>
https://cs.chromium.org/chromium/src/v8/include/v8.h?sq=package:chromium&dr=C...
> [2]
>
https://cs.chromium.org/chromium/src/v8/src/messages.cc?sq=package:chromium&d...

Maybe are you looking for ExecutionContext::reportException()? That's a common
way to report exceptions associated with ErrorEvents.

See here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

[sivag, 2016-07-27 13:14:40]

On 2016/07/26 01:59:12, nhiroki wrote:
> I'm a bit confused about SetVerbose(true). At first, I assumed that it's
> necessary to print more specific error messages on initial worker script
> evaluation as issue 590219 mentions. However, according to the CL description
> and a header comment of the function[1], it's used for reporting an exception
> already caught by an external exception handler and not intended for making
> error message verbose. This would be the reason why these tests are broken.
> 
> Is there another way to refine error messages without such a side-effect? As
far
> as I can see, when SetVerbose(true) is enabled, message seems to be tweaked in
> MessageHandler::ReportMessage()[2]. We could do the similar thing even when
the
> verbose flag is not set.
> 
> [1]
>
https://cs.chromium.org/chromium/src/v8/include/v8.h?sq=package:chromium&dr=C...
> [2]
>
https://cs.chromium.org/chromium/src/v8/src/messages.cc?sq=package:chromium&d...
> 
>
https://codereview.chromium.org/2090953006/diff/80001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/fast/workers/worker-onerror-05.html
(left):
> 
>
https://codereview.chromium.org/2090953006/diff/80001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/fast/workers/worker-onerror-05.html:21: return
> false;
> On 2016/07/25 13:34:03, sivag wrote:
> > On 2016/07/25 08:30:34, nhiroki (slow) wrote:
> > > After SetVerbose(true), an error handler on Worker context propagates an
> error
> > > to a handler on Window context (according to the spec[1], this would be
> > correct
> > > behavior). This results in 2 error handler calls on Window context and 1
> error
> > > handler call on Worker. This mismatches with a test expectation.
> > > 
> > > Probably we should prevent the firing of Window's event handler by
returning
> > > true here.
> > > 
> > > [1]
> > https://html.spec.whatwg.org/multipage/workers.html#runtime-script-errors-2
> > > "If the event is not canceled, the user agent must act as if the uncaught
> > > runtime script error had occurred in the global scope that the Worker
object
> > is
> > > in, thus repeating the entire runtime script error reporting process one
> level
> > > up."
> > 
> > Done.
> 
> Hmmm... sorry, I read the spec[1] again and realized I misunderstood the
problem
> and spec. I thought 'onerror' on Window context is always called regardless of
> whether 'onerror' on Worker context is canceled. It was not correct. 'onerror'
> on Window context must be called only when 'onerror' on Worker context is not
> canceled like the original 'worker-onerror-05.html'.
> 
> Therefore, returning false here is correct...
> 
>
https://codereview.chromium.org/2090953006/diff/100001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/fast/workers/worker-constructor.html
> (right):
> 
>
https://codereview.chromium.org/2090953006/diff/100001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/fast/workers/worker-constructor.html:16:
> "testEmptyScriptUrl",
> Why did you move this test case?

As far as i can see from the code, the DOMException is thrown to
console only , when we enable the verbose in trycatch, before we 
compile and run the script in worker context.

[sivag, 2016-07-27 13:16:06]

On 2016/07/26 11:44:16, haraken wrote:
> On 2016/07/26 01:59:12, nhiroki (slow) wrote:
> > I'm a bit confused about SetVerbose(true). At first, I assumed that it's
> > necessary to print more specific error messages on initial worker script
> > evaluation as issue 590219 mentions. However, according to the CL
description
> > and a header comment of the function[1], it's used for reporting an
exception
> > already caught by an external exception handler and not intended for making
> > error message verbose. This would be the reason why these tests are broken.
> > 
> > Is there another way to refine error messages without such a side-effect? As
> far
> > as I can see, when SetVerbose(true) is enabled, message seems to be tweaked
in
> > MessageHandler::ReportMessage()[2]. We could do the similar thing even when
> the
> > verbose flag is not set.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/v8/include/v8.h?sq=package:chromium&dr=C...
> > [2]
> >
>
https://cs.chromium.org/chromium/src/v8/src/messages.cc?sq=package:chromium&d...
> 
> Maybe are you looking for ExecutionContext::reportException()? That's a common
> way to report exceptions associated with ErrorEvents.
> 
> See here:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

we are already reportingException here
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
except the the errorMessage is not having the exception info.

[sivag, 2016-07-27 13:20:52]

https://codereview.chromium.org/2090953006/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/workers/worker-constructor.html
(right):

https://codereview.chromium.org/2090953006/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/workers/worker-constructor.html:16:
"testEmptyScriptUrl",
On 2016/07/26 01:59:11, nhiroki wrote:
> Why did you move this test case?

In these set of tests only 2 tests throw worker.onerror testNotExistentScriptUrl
and testEmptyScriptUrl", but here when verbose is enabled  and the order of call
is 

testEmptyScriptUrl
...
testNotExistentScriptUrl

in both cases testEmptyScriptUrl functions worker.onerror is getting
printed.changing the order as above works fine.Initially i thought we are
executing the next test case in onerror by calling runNextTest(); which might be
causing this, but i tried calling this after the try catch for all test cases
still the same issue exists.

I am trying to figure out why this happens.

[sivag, 2016-07-29 09:28:50]

siva.gunturi@samsung.com changed reviewers:
- vivekg@chromium.org

[sivag, 2016-07-29 09:29:32]

https://codereview.chromium.org/2090953006/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/workers/worker-constructor.html
(right):

https://codereview.chromium.org/2090953006/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/workers/worker-constructor.html:16:
"testEmptyScriptUrl",
On 2016/07/26 01:59:11, nhiroki wrote:
> Why did you move this test case?

After enabling verbose the order of test is getting effected.
When test [1] new Worker("") gets called before test [2] new
Worker("emptyscript.js")
in both cases test[1] onError is getting called.

If i make test [1] to  new Worker("invalid.js") , with out changing the order
the respective onerror functions of [1][2] get called.

Any pointers, how should we proceed further here?