[inspector] entered into context before getPossibleBreakpoints call

[inspector] entered into context before getPossibleBreakpoints call

getPossibleBreakpoints implementation can enforce function compilation which potentially can produce syntax error, we need to have a context to correctly report this error.

BUG=chromium:715334
R=dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2851853002
Cr-Commit-Position: refs/heads/master@{#45003}
Committed: https://chromium.googlesource.com/v8/v8/+/f79c3b51426b5b68f044593979a674106a5432c6

[kozy, 2017-04-28 19:40:08]

kozyatinskiy@chromium.org changed reviewers:
+ jgruber@chromium.org, yangguo@chromium.org

[kozy, 2017-04-28 19:40:09]

please take a look.

I'm not sure about this fix since it looks too lazy to me to throw compilation
errors afterwards (one script is reported twice as good one and as script with
syntax error), should preparser catch this errors?

[dgozman, 2017-04-28 20:57:23]

I don't really like entering some context when looking for breakpoints. What if
I enter the wrong one? Will the script get compiled with wrong context and/or
global? That's very error prone. Can we do something else?

https://codereview.chromium.org/2851853002/diff/1/src/inspector/v8-debugger-a...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2851853002/diff/1/src/inspector/v8-debugger-a...
src/inspector/v8-debugger-agent-impl.cc:464: if (!inspected) return
Response::InternalError();
I don't like this return. Sometimes we don't have a context for a script - we
should handle it gracefully.

https://codereview.chromium.org/2851853002/diff/1/src/inspector/v8-debugger-a...
src/inspector/v8-debugger-agent-impl.cc:471: v8::TryCatch tryCatch(m_isolate);
Put all scopes inside of {} block?

[kozy, 2017-04-28 21:17:10]

Dmitry, please take another look.

https://codereview.chromium.org/2851853002/diff/1/src/inspector/v8-debugger-a...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2851853002/diff/1/src/inspector/v8-debugger-a...
src/inspector/v8-debugger-agent-impl.cc:464: if (!inspected) return
Response::InternalError();
On 2017/04/28 20:57:23, dgozman wrote:
> I don't like this return. Sometimes we don't have a context for a script - we
> should handle it gracefully.

I think I can use debuggerContext to fix a crash and later think about this
within debugger context removing work - we currently enter debugger context when
setting a breakpoint for the same reason.

https://codereview.chromium.org/2851853002/diff/1/src/inspector/v8-debugger-a...
src/inspector/v8-debugger-agent-impl.cc:471: v8::TryCatch tryCatch(m_isolate);
On 2017/04/28 20:57:23, dgozman wrote:
> Put all scopes inside of {} block?

Done.

[dgozman, 2017-05-01 19:06:08]

lgtm

[kozy, 2017-05-01 19:11:50]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2017-05-01 19:11:54]

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, 2017-05-01 19:44:14]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493665910416680,
"parent_rev": "fc5079d34bd91f824aacd6d18c487e9ef59349f9", "commit_rev":
"f79c3b51426b5b68f044593979a674106a5432c6"}

[commit-bot: I haz the power, 2017-05-01 19:44:23]

Description was changed from

==========
[inspector] entered into context before getPossibleBreakpoints call

getPossibleBreakpoints implementation can enforce function compilation which
potentially can produce syntax error, we need to have a context to correctly
report this error.

BUG=chromium:715334
R=dgozman@chromium.org
==========

to

==========
[inspector] entered into context before getPossibleBreakpoints call

getPossibleBreakpoints implementation can enforce function compilation which
potentially can produce syntax error, we need to have a context to correctly
report this error.

BUG=chromium:715334
R=dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2851853002
Cr-Commit-Position: refs/heads/master@{#45003}
Committed:
https://chromium.googlesource.com/v8/v8/+/f79c3b51426b5b68f044593979a674106a5...
==========

[commit-bot: I haz the power, 2017-05-01 19:44:23]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/f79c3b51426b5b68f044593979a674106a5...

[jgruber, 2017-05-02 07:00:24]

https://codereview.chromium.org/2851853002/diff/20001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2851853002/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:466: it->second->getPossibleBreakpoints(
For my understanding, why do we neither check the return value nor whether
tryCatch has caught?

[kozy, 2017-05-04 15:23:41]

https://codereview.chromium.org/2851853002/diff/20001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2851853002/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:466: it->second->getPossibleBreakpoints(
On 2017/05/02 07:00:23, jgruber wrote:
> For my understanding, why do we neither check the return value nor whether
> tryCatch has caught?

Reason of linked issue is lazy compilation of script with syntax error - there
is no breakable locations in script with syntax error so reporting empty array
instead of InternalError sounds good to me.

[jgruber, 2017-05-04 15:26:39]

https://codereview.chromium.org/2851853002/diff/20001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2851853002/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:466: it->second->getPossibleBreakpoints(
On 2017/05/04 15:23:41, kozy wrote:
> On 2017/05/02 07:00:23, jgruber wrote:
> > For my understanding, why do we neither check the return value nor whether
> > tryCatch has caught?
> 
> Reason of linked issue is lazy compilation of script with syntax error - there
> is no breakable locations in script with syntax error so reporting empty array
> instead of InternalError sounds good to me.

I see, thanks for the explanation.