Avoid using stale InspectedContext pointers

Avoid using stale InspectedContext pointers

BUG=657568
TEST=Manually, see bug report

Committed: https://crrev.com/cb2a39d36762da3a800d9b5e734319d1141070b9
Cr-Commit-Position: refs/heads/master@{#40605}

[robwu, 2016-10-19 19:30:50]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 19:30:53]

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-10-19 19:55:36]

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

[commit-bot: I haz the power, 2016-10-19 19:55:36]

Dry run: Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/26895)

[robwu, 2016-10-19 22:00:36]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 22:00:38]

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

[robwu, 2016-10-19 22:37:30]

rob@robwu.nl changed reviewers:
+ kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-10-19 22:57:00]

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

[commit-bot: I haz the power, 2016-10-19 22:57:01]

Dry run: This issue passed the CQ dry run.

[kozy, 2016-10-20 00:16:59]

lgtm

[kozy, 2016-10-20 00:20:55]

I added blink bot to check that our layout tests in Chrome will pass with this
change, please wait until it finishes.

[robwu, 2016-10-20 23:07:11]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 23:07:19]

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

[robwu, 2016-10-20 23:08:45]

I added unit tests, please take another look.

I generated the test expectation as follows:

./out.gn/x64.release/inspector-test test/inspector/protocol-test.js
test/inspector/console/destroy-context-during-log.js >
test/inspector/console/destroy-context-during-log-expected.txt

[robwu, 2016-10-20 23:12:28]

rob@robwu.nl changed reviewers:
+ jochen@chromium.org

[robwu, 2016-10-20 23:12:28]

+jochen for rubberstamp review of AUTHORS.

[commit-bot: I haz the power, 2016-10-20 23:45:19]

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

[commit-bot: I haz the power, 2016-10-20 23:45:20]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-10-21 11:01:18]

lgtm

[kozy, 2016-10-21 15:06:53]

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
File test/inspector/console/destroy-context-during-log-expected.txt (right):

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
test/inspector/console/destroy-context-during-log-expected.txt:4: Attaching
context.
Please don't dump dettaching/attaching context. It can be root of flakiness
because here you dump text output from different threads.

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
File test/inspector/console/destroy-context-during-log.js (right):

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
test/inspector/console/destroy-context-during-log.js:32: expression,
Looks suspicious, expression: expression?

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
test/inspector/console/destroy-context-during-log.js:33: silent: true,
You don't really need it and it'll hide exception in expression if it happens.

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
test/inspector/console/destroy-context-during-log.js:34: returnByValue: false,
it's false by default, you can just pass { expression: expression }

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/inspecto...
File test/inspector/inspector-test.cc (right):

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/inspecto...
test/inspector/inspector-test.cc:244: v8::Local<v8::Context> context =
isolate->GetEnteredContext();
GetCurrentContext()

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/inspecto...
test/inspector/inspector-test.cc:252: inspector->contextDestroyed(context);
Do you really need this call? Otherwise it looks like Reaatach, not attach.

[robwu, 2016-10-21 20:32:08]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 20:32:19]

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

[robwu, 2016-10-21 20:32:45]

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
File test/inspector/console/destroy-context-during-log-expected.txt (right):

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
test/inspector/console/destroy-context-during-log-expected.txt:4: Attaching
context.
On 2016/10/21 15:06:53, kozyatinskiy wrote:
> Please don't dump dettaching/attaching context. It can be root of flakiness
> because here you dump text output from different threads.

Done.

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
File test/inspector/console/destroy-context-during-log.js (right):

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
test/inspector/console/destroy-context-during-log.js:32: expression,
On 2016/10/21 15:06:53, kozyatinskiy wrote:
> Looks suspicious, expression: expression?

Nope, this is ES6 syntax. It is a shorthand for expression: expression.
I've changed it to "expression: expression" in case you require ES5 syntax (the
"const" at the top of this file is also ES6 syntax), but please let me know if
you would or would not accept such modern JS syntax in future patches, if any.

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
test/inspector/console/destroy-context-during-log.js:33: silent: true,
On 2016/10/21 15:06:53, kozyatinskiy wrote:
> You don't really need it and it'll hide exception in expression if it happens.

Done.

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
test/inspector/console/destroy-context-during-log.js:34: returnByValue: false,
On 2016/10/21 15:06:53, kozyatinskiy wrote:
> it's false by default, you can just pass { expression: expression }

Done.

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/inspecto...
File test/inspector/inspector-test.cc (right):

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/inspecto...
test/inspector/inspector-test.cc:244: v8::Local<v8::Context> context =
isolate->GetEnteredContext();
On 2016/10/21 15:06:53, kozyatinskiy wrote:
> GetCurrentContext()

Done.

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/inspecto...
test/inspector/inspector-test.cc:252: inspector->contextDestroyed(context);
On 2016/10/21 15:06:53, kozyatinskiy wrote:
> Do you really need this call? Otherwise it looks like Reaatach, not attach.

Oops. Removed.

[kozy, 2016-10-21 20:41:26]

lgtm

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
File test/inspector/console/destroy-context-during-log.js (right):

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
test/inspector/console/destroy-context-during-log.js:32: expression,
On 2016/10/21 20:32:45, robwu wrote:
> On 2016/10/21 15:06:53, kozyatinskiy wrote:
> > Looks suspicious, expression: expression?
> 
> Nope, this is ES6 syntax. It is a shorthand for expression: expression.
> I've changed it to "expression: expression" in case you require ES5 syntax
(the
> "const" at the top of this file is also ES6 syntax), but please let me know if
> you would or would not accept such modern JS syntax in future patches, if any.

Thanks,
For object literals we prefer ES5 syntax without shorthands. Scope variables are
okey. As well as one line arrow functions and template literals for multiline
strings.

[robwu, 2016-10-21 20:47:37]

Thanks for the reviews.

Are the new tests automatically run on a try bot, or should I add a
CQ_INCLUDE_TRYBOTS line to the commit description?

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
File test/inspector/console/destroy-context-during-log.js (right):

https://codereview.chromium.org/2432163004/diff/40001/test/inspector/console/...
test/inspector/console/destroy-context-during-log.js:32: expression,
On 2016/10/21 20:41:26, kozyatinskiy wrote:
> On 2016/10/21 20:32:45, robwu wrote:
> > On 2016/10/21 15:06:53, kozyatinskiy wrote:
> > > Looks suspicious, expression: expression?
> > 
> > Nope, this is ES6 syntax. It is a shorthand for expression: expression.
> > I've changed it to "expression: expression" in case you require ES5 syntax
> (the
> > "const" at the top of this file is also ES6 syntax), but please let me know
if
> > you would or would not accept such modern JS syntax in future patches, if
any.
> 
> Thanks,
> For object literals we prefer ES5 syntax without shorthands. Scope variables
are
> okey. As well as one line arrow functions and template literals for multiline
> strings.

Is this tribal knowledge, or is there a public coding style document or e.g. a
set of rules that I can add to my .eslintrc or .jshintrc?

[commit-bot: I haz the power, 2016-10-21 21:03:40]

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

[commit-bot: I haz the power, 2016-10-21 21:03:40]

Dry run: This issue passed the CQ dry run.

[kozy, 2016-10-25 15:14:24]

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

[commit-bot: I haz the power, 2016-10-25 15:14:34]

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

[kozy, 2016-10-25 15:23:05]

On 2016/10/21 20:47:37, robwu wrote:
> Thanks for the reviews.
> 
> Are the new tests automatically run on a try bot, or should I add a
> CQ_INCLUDE_TRYBOTS line to the commit description?
> 

Tests were enabled around 8 hours ago, I rerun CQ dry run to check your CL with
all inspector tests.

> Is this tribal knowledge, or is there a public coding style document or e.g. a
> set of rules that I can add to my .eslintrc or .jshintrc?

Unfortunately, it's kind of tribal knowledge, I think that allowed ES2016
features:
- one line arrow functions (we use () => instead of _ =>),
- for of loops,
- template literals,
- Map, Set.

[commit-bot: I haz the power, 2016-10-25 15:48:51]

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

[commit-bot: I haz the power, 2016-10-25 15:48:52]

Dry run: This issue passed the CQ dry run.

[robwu, 2016-10-26 19:54:38]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-10-26 19:54:38]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2432163004/#ps60001
(title: "Fix nits")

[commit-bot: I haz the power, 2016-10-26 19:54:43]

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-10-26 20:27:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-17 22:14:46]

Description was changed from

==========
Avoid using stale InspectedContext pointers

BUG=657568
TEST=Manually, see bug report
==========

to

==========
Avoid using stale InspectedContext pointers

BUG=657568
TEST=Manually, see bug report

Committed: https://crrev.com/cb2a39d36762da3a800d9b5e734319d1141070b9
Cr-Commit-Position: refs/heads/master@{#40605}
==========

[commit-bot: I haz the power, 2016-11-17 22:14:48]

Patchset 4 (id:??) landed as
https://crrev.com/cb2a39d36762da3a800d9b5e734319d1141070b9
Cr-Commit-Position: refs/heads/master@{#40605}