Adding field to tell if gin::IsolateHolder needs lock to be accessed.

Adding field to tell if gin::IsolateHolder needs lock to be accessed.

V8 dump provider needs to access the isolate to get memory statistics.
When proxy_resolver causes the isolate to switch between threads, the
dump provider cannot access it from the correct thread. This CL adds a
field to IsolateHolder to tell the dump provider if it needs to use a
lock to access the isolate.

BUG=476013
Committed: https://crrev.com/cf20763e247e6efa09bd9f8ff22d460e93161806
Cr-Commit-Position: refs/heads/master@{#326794}

[ssid, 2015-04-23 15:18:29]

ssid@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[ssid, 2015-04-23 15:18:30]

PTAL.

[Primiano Tucci (use gerrit), 2015-04-23 15:52:39]

primiano@chromium.org changed reviewers:
+ jochen@chromium.org, primiano@chromium.org
- rmcilroy@chromium.org

[Primiano Tucci (use gerrit), 2015-04-23 15:52:40]

Jochen, WDYT here?

https://codereview.chromium.org/1100743004/diff/1/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/1100743004/diff/1/gin/isolate_holder.cc#newco...
gin/isolate_holder.cc:90: bool IsolateHolder::UseLock() {
At this point, since you introduced an enum, I'd just return the enum

https://codereview.chromium.org/1100743004/diff/1/gin/public/isolate_holder.h
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/1100743004/diff/1/gin/public/isolate_holder.h...
gin/public/isolate_holder.h:32: kLockToAccess,
I'd probably do:

enum AccessMode {
  kSingleThread,
  kUseLocker
}

https://codereview.chromium.org/1100743004/diff/1/gin/public/isolate_holder.h...
gin/public/isolate_holder.h:63: bool UseLock();
UseLock is a bit confusing as a name as it is not clear whether you are telling
the isolate to use a lock, or you are asking that.
What about: DoesUseV8Locker()?

Actually I'd defer to jochen for the name.

[rmcilroy, 2015-04-23 15:52:51]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org
- jochen@chromium.org, primiano@chromium.org

[rmcilroy, 2015-04-23 15:52:52]

lgtm once typo is fixed.

https://codereview.chromium.org/1100743004/diff/1/gin/public/isolate_holder.h
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/1100743004/diff/1/gin/public/isolate_holder.h...
gin/public/isolate_holder.h:30: // Stores wheather the client uses v8::Locker to
access the isolate.
/s/wheather/whether

[rmcilroy, 2015-04-23 15:53:42]

On 2015/04/23 15:52:52, rmcilroy wrote:
> lgtm once typo is fixed.

+Primiano's comments.

[ssid, 2015-04-23 16:24:57]

made changes.

https://codereview.chromium.org/1100743004/diff/1/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/1100743004/diff/1/gin/isolate_holder.cc#newco...
gin/isolate_holder.cc:90: bool IsolateHolder::UseLock() {
On 2015/04/23 15:52:40, Primiano Tucci wrote:
> At this point, since you introduced an enum, I'd just return the enum

Done.

https://codereview.chromium.org/1100743004/diff/1/gin/public/isolate_holder.h
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/1100743004/diff/1/gin/public/isolate_holder.h...
gin/public/isolate_holder.h:30: // Stores wheather the client uses v8::Locker to
access the isolate.
On 2015/04/23 15:52:51, rmcilroy wrote:
> /s/wheather/whether

Done.

https://codereview.chromium.org/1100743004/diff/1/gin/public/isolate_holder.h...
gin/public/isolate_holder.h:32: kLockToAccess,
On 2015/04/23 15:52:40, Primiano Tucci wrote:
> I'd probably do:
> 
> enum AccessMode {
>   kSingleThread,
>   kUseLocker
> }

Done.

https://codereview.chromium.org/1100743004/diff/1/gin/public/isolate_holder.h...
gin/public/isolate_holder.h:63: bool UseLock();
On 2015/04/23 15:52:40, Primiano Tucci wrote:
> UseLock is a bit confusing as a name as it is not clear whether you are
telling
> the isolate to use a lock, or you are asking that.
> What about: DoesUseV8Locker()?
> 
> Actually I'd defer to jochen for the name.

Done.

[ssid, 2015-04-23 16:26:58]

ssid@chromium.org changed reviewers:
+ jochen@chromium.org, rsleevi@chromium.org

[ssid, 2015-04-23 16:26:58]

+rsleevi@ for 1 line change in net/proxy

+jochen@ for the rest.

[rmcilroy, 2015-04-23 17:01:27]

https://codereview.chromium.org/1100743004/diff/20001/gin/public/isolate_hold...
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/1100743004/diff/20001/gin/public/isolate_hold...
gin/public/isolate_holder.h:63: AccessMode does_use_v8_locker() { return
access_mode_; }
Should be access_mode() if it is a getter. (otherwise it will need to be
DoesUseV8Locker, but I think access_mode() would be fine here).

[Primiano Tucci (use gerrit), 2015-04-23 17:20:32]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-04-23 17:20:32]

https://codereview.chromium.org/1100743004/diff/20001/gin/public/isolate_hold...
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/1100743004/diff/20001/gin/public/isolate_hold...
gin/public/isolate_holder.h:63: AccessMode does_use_v8_locker() { return
access_mode_; }
On 2015/04/23 17:01:27, rmcilroy wrote:
> Should be access_mode() if it is a getter. (otherwise it will need to be
> DoesUseV8Locker, but I think access_mode() would be fine here).

+1.
Also please make it a const (i.e. AccessMode access_mode() const { return ... })

[Ryan Sleevi, 2015-04-23 20:32:15]

//net LGTM % style issue

https://codereview.chromium.org/1100743004/diff/40001/gin/public/isolate_hold...
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/1100743004/diff/40001/gin/public/isolate_hold...
gin/public/isolate_holder.h:37: IsolateHolder(AccessMode access_mode);
explicit

[jochen (gone - plz use gerrit), 2015-04-24 14:07:53]

lgtm

[ssid, 2015-04-24 14:38:50]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-04-24 14:38:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1100743004/#ps60001
(title: "Adding explicit.")

[commit-bot: I haz the power, 2015-04-24 14:39:27]

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

[commit-bot: I haz the power, 2015-04-24 14:44:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-24 14:46:28]

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