Get rid of usages of ScriptWrappable::wrapper().

Get rid of usages of ScriptWrappable::wrapper().

This is needed for getting rid of Persistent handle copying, as we're going to
remove the copy constructor (see bug for more information).

If we need to pass back a Persistent by reference in the V8 API, we can normally
just pass back a Persistent we hold. ScriptWrappable is problematic, because it
does not hold a Persistent, but instead stores the raw pointer. Added
UnsafePersistent to make the conversion from the raw pointer to a Persistent
explicit.

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

[marja, 2013-04-26 13:38:51]

dcarney, ptal

http://knowyourmeme.com/photos/234765-i-have-no-idea-what-im-doing

https://codereview.chromium.org/14443007/diff/1/Source/bindings/v8/V8GCContro...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/14443007/diff/1/Source/bindings/v8/V8GCContro...
Source/bindings/v8/V8GCController.cpp:147: void gcTree(v8::Isolate* isolate,
Node* startNode)
Needed to move this function to be able to be ScriptWrappable's friend...

https://codereview.chromium.org/14443007/diff/1/Source/bindings/v8/V8GCContro...
Source/bindings/v8/V8GCController.cpp:193: v8::UniqueId
id(reinterpret_cast<intptr_t>((*nodeIterator)->rawWrapper()));
Here are the parts which have changed. (rawWrapper())

[haraken, 2013-04-26 13:42:22]

https://codereview.chromium.org/14443007/diff/1/Source/bindings/v8/ScriptWrap...
File Source/bindings/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/14443007/diff/1/Source/bindings/v8/ScriptWrap...
Source/bindings/v8/ScriptWrappable.h:157: friend class MinorGCWrapperVisitor; 
// For calling rawWrapper().
Is this the only function we need to make a friend? If there are many other
functions we need to make friends, we might want to consider another solution.

https://codereview.chromium.org/14443007/diff/1/Source/bindings/v8/ScriptWrap...
Source/bindings/v8/ScriptWrappable.h:162: return NULL;
NULL => 0

https://codereview.chromium.org/14443007/diff/1/Source/bindings/v8/V8GCContro...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/14443007/diff/1/Source/bindings/v8/V8GCContro...
Source/bindings/v8/V8GCController.cpp:196: v8::Persistent<v8::Object>* wrapper =
reinterpret_cast<v8::Persistent<v8::Object>*>(rawWrapper);
How many places do you think we have to use this kind of hard-to-understand
programming pattern?

[marja, 2013-04-26 13:48:17]

... and I forgot to say that this patch applies on top of 
https://codereview.chromium.org/13975005/ .

https://codereview.chromium.org/14443007/diff/1/Source/bindings/v8/ScriptWrap...
File Source/bindings/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/14443007/diff/1/Source/bindings/v8/ScriptWrap...
Source/bindings/v8/ScriptWrappable.h:157: friend class MinorGCWrapperVisitor; 
// For calling rawWrapper().
On 2013/04/26 13:42:23, haraken wrote:
> Is this the only function we need to make a friend? If there are many other
> functions we need to make friends, we might want to consider another solution.

Afaics, the only other use of wrapper() is in
DOMDataStore::holderContainsWrapper.

https://codereview.chromium.org/14443007/diff/1/Source/bindings/v8/ScriptWrap...
Source/bindings/v8/ScriptWrappable.h:162: return NULL;
On 2013/04/26 13:42:23, haraken wrote:
> NULL => 0

Done.

https://codereview.chromium.org/14443007/diff/1/Source/bindings/v8/V8GCContro...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/14443007/diff/1/Source/bindings/v8/V8GCContro...
Source/bindings/v8/V8GCController.cpp:196: v8::Persistent<v8::Object>* wrapper =
reinterpret_cast<v8::Persistent<v8::Object>*>(rawWrapper);
On 2013/04/26 13:42:23, haraken wrote:
> How many places do you think we have to use this kind of hard-to-understand
> programming pattern?

Hard to estimate at this point; we need to do something which doesn't require
copying Handles, this is one option...

[haraken, 2013-04-26 13:51:12]

> On 2013/04/26 13:42:23, haraken wrote:
> > How many places do you think we have to use this kind of hard-to-understand
> > programming pattern?
> 
> Hard to estimate at this point; we need to do something which doesn't require
> copying Handles, this is one option...

Personally I think that we should first implement PassHandle before messing up
the code base with hard-to-understand handle usages.

Dan: WDYT?

[dcarney, 2013-04-26 14:05:59]

On 2013/04/26 13:51:12, haraken wrote:
> > On 2013/04/26 13:42:23, haraken wrote:
> > > How many places do you think we have to use this kind of
hard-to-understand
> > > programming pattern?
> > 
> > Hard to estimate at this point; we need to do something which doesn't
require
> > copying Handles, this is one option...
> 
> Personally I think that we should first implement PassHandle before messing up
> the code base with hard-to-understand handle usages.
> 
> Dan: WDYT?

We are not going to implement PassHandle for Persistent.  I think rawWrapper
should have a name which makes it more clear that we're returning a persistent.

There should only be a handful of places where we need to do this kind of thing,
and the fact that an v8::Object* is identical to a v8::Handle means the its a
good candidate to pass around.

[haraken, 2013-04-26 14:14:17]

> We are not going to implement PassHandle for Persistent.  I think rawWrapper
> should have a name which makes it more clear that we're returning a
persistent.
> 
> There should only be a handful of places where we need to do this kind of
thing,
> and the fact that an v8::Object* is identical to a v8::Handle means the its a
> good candidate to pass around.

Hmm, but then we have to guarantee that GC never happens while we're holding
v8::Object*s. GC can change the address of the objects. Is my understanding
correct?

[dcarney, 2013-04-26 14:15:28]

> Hmm, but then we have to guarantee that GC never happens while we're holding
> v8::Object*s. GC can change the address of the objects. Is my understanding
> correct?

no, a v8::internal::Object* can be relocated, but not a v8::Object*

[haraken, 2013-04-26 14:19:02]

> no, a v8::internal::Object* can be relocated, but not a v8::Object*

Ah, got it. Then, as long as a persistent handle for a v8::Object* exists, it is
safe to pass the v8::Object* around. Is this correct?

[dcarney, 2013-04-26 14:26:13]

On 2013/04/26 14:19:02, haraken wrote:
> > no, a v8::internal::Object* can be relocated, but not a v8::Object*
> 
> Ah, got it. Then, as long as a persistent handle for a v8::Object* exists, it
is
> safe to pass the v8::Object* around. Is this correct?

it's certainly safe during the gc epilogue and prologue.  this is why it's
private

[haraken, 2013-04-26 14:30:14]

Understood.

However, I still think that the programming pattern is too hard to understand.
If we use the pattern here and there, I'm afraid that V8 binding will become a
place where others cannot touch...

Although I don't have a good idea at this point, I'm thinking about an
abstraction mechanism for this kind of handle usages.

[marja, 2013-04-26 14:33:24]

I think a fundamental problem is that Handle<T> is T*,
internal::Handle<internal::T> is T**, and still they are the same thing.
(Because T is one indirection level more than internal::T.) This is incredibly
hard to grasp without somebody explaining it to you. I'm afraid abstracting this
further will make it more obscure; it's pretty maximally abstract as is. ;)

[abarth-chromium, 2013-04-26 22:13:20]

It would be nice if the description contained more information about this CL. 
Why are we making this change?  What benefit does it have for the project?

[dcarney, 2013-04-29 10:33:51]

lgtm, but let haraken sign off on it first

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/Unsafe...
File Source/bindings/v8/UnsafePersistent.h (right):

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/Unsafe...
Source/bindings/v8/UnsafePersistent.h:55: T* value() const
is this needed?

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/V8GCCo...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/V8GCCo...
Source/bindings/v8/V8GCController.cpp:199:
unsafeWrapper.makePersistentHandle(&wrapper);
maybe make one line?

[haraken, 2013-04-29 10:42:17]

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/Unsafe...
File Source/bindings/v8/UnsafePersistent.h (right):

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/Unsafe...
Source/bindings/v8/UnsafePersistent.h:41: // the UnsafePersistent is alive or 2)
when there is a strong Persistent keeping
Would you add an ASSERT() that checks that GC has never happened during the
lifetime of the UnsafePersistent? You can add a flag in V8GCController (e.g.
V8GCController::gc_observed_flag_).

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/V8GCCo...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/V8GCCo...
Source/bindings/v8/V8GCController.cpp:200:
wrapper.MarkPartiallyDependent(isolate);
Would it be possible to define UnsafePersistent::operator->() and write this
line like:

  unsafeWrapper->MarkPartiallyDependent(isolate);

? Then you can remove the redundant 'wrapper'.

[marja, 2013-04-29 12:02:07]

Thanks for comments!

abarth: Could you have a look at the new version, haraken told me to wait for
your review.

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/Unsafe...
File Source/bindings/v8/UnsafePersistent.h (right):

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/Unsafe...
Source/bindings/v8/UnsafePersistent.h:41: // the UnsafePersistent is alive or 2)
when there is a strong Persistent keeping
On 2013/04/29 10:42:17, haraken wrote:
> Would you add an ASSERT() that checks that GC has never happened during the
> lifetime of the UnsafePersistent? You can add a flag in V8GCController (e.g.
> V8GCController::gc_observed_flag_).

Can't be a bool, because we wouldn't know when to reset it (and there might be
multiple UnsafePersistents around...), but added a counter.

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/Unsafe...
Source/bindings/v8/UnsafePersistent.h:55: T* value() const
On 2013/04/29 10:33:51, dcarney wrote:
> is this needed?

This is used for constructing the UniqueId.

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/V8GCCo...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/V8GCCo...
Source/bindings/v8/V8GCController.cpp:199:
unsafeWrapper.makePersistentHandle(&wrapper);
On 2013/04/29 10:33:51, dcarney wrote:
> maybe make one line?

Done.

https://codereview.chromium.org/14443007/diff/16001/Source/bindings/v8/V8GCCo...
Source/bindings/v8/V8GCController.cpp:200:
wrapper.MarkPartiallyDependent(isolate);
On 2013/04/29 10:42:17, haraken wrote:
> Would it be possible to define UnsafePersistent::operator->() and write this
> line like:
> 
>   unsafeWrapper->MarkPartiallyDependent(isolate);
> 
> ? Then you can remove the redundant 'wrapper'.

Offline discussion -> the wrapper is still needed for SetObjectGroupId.

The underlying problem is that ScriptWrappable doesn't hold a wrapper; if it
did, we could pass it by reference through this layer to the V8 API. So it holds
this raw pointer and we need to somehow create a Persistent handle; thus
UnsafePersistent.

But I streamlined this so that we don't assign unsafeWrapper  (above) into its
own variable.

[abarth-chromium, 2013-04-29 17:47:33]

LGTM

This CL is kind of crazy, but it looks like it's moving us in the right
direction.

https://codereview.chromium.org/14443007/diff/31001/Source/bindings/v8/Unsafe...
File Source/bindings/v8/UnsafePersistent.h (right):

https://codereview.chromium.org/14443007/diff/31001/Source/bindings/v8/Unsafe...
Source/bindings/v8/UnsafePersistent.h:44: // TODO: assert that GC doesn't happen
during the lifetime of UnsafePersistent.
TODO -> FIXME

https://codereview.chromium.org/14443007/diff/31001/Source/bindings/v8/Unsafe...
Source/bindings/v8/UnsafePersistent.h:45: template<typename T> class
UnsafePersistent {
I probably would have called this class UnsafeHandle, but this name is fine too.

https://codereview.chromium.org/14443007/diff/31001/Source/bindings/v8/Unsafe...
Source/bindings/v8/UnsafePersistent.h:51: void
makePersistentHandle(v8::Persistent<T>* handle) const
copyTo ?

https://codereview.chromium.org/14443007/diff/31001/Source/bindings/v8/Unsafe...
Source/bindings/v8/UnsafePersistent.h:53: T** rawValue =
reinterpret_cast<T**>(handle);
Wow, that's scary stuff.

https://codereview.chromium.org/14443007/diff/31001/Source/bindings/v8/V8GCCo...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/14443007/diff/31001/Source/bindings/v8/V8GCCo...
Source/bindings/v8/V8GCController.cpp:164: if
(!node->isV8CollectableDuringMinorGC() || (node->hasTagName(HTMLNames::imgTag)
&& static_cast<    HTMLImageElement*>(node)->hasPendingActivity())) {
Looks like you've got some extra spaces between the < and the HTMLImageElement

[marja, 2013-04-30 08:55:50]

Thanks for review!

https://codereview.chromium.org/14443007/diff/31001/Source/bindings/v8/Unsafe...
File Source/bindings/v8/UnsafePersistent.h (right):

https://codereview.chromium.org/14443007/diff/31001/Source/bindings/v8/Unsafe...
Source/bindings/v8/UnsafePersistent.h:44: // TODO: assert that GC doesn't happen
during the lifetime of UnsafePersistent.
On 2013/04/29 17:47:33, abarth wrote:
> TODO -> FIXME

Done.

https://codereview.chromium.org/14443007/diff/31001/Source/bindings/v8/Unsafe...
Source/bindings/v8/UnsafePersistent.h:45: template<typename T> class
UnsafePersistent {
On 2013/04/29 17:47:33, abarth wrote:
> I probably would have called this class UnsafeHandle, but this name is fine
too.

After the future renaming, Local will be called Handle, so calling this
UnsafeHandle would be confusing.

https://codereview.chromium.org/14443007/diff/31001/Source/bindings/v8/Unsafe...
Source/bindings/v8/UnsafePersistent.h:51: void
makePersistentHandle(v8::Persistent<T>* handle) const
On 2013/04/29 17:47:33, abarth wrote:
> copyTo ?

Done.

https://codereview.chromium.org/14443007/diff/31001/Source/bindings/v8/V8GCCo...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/14443007/diff/31001/Source/bindings/v8/V8GCCo...
Source/bindings/v8/V8GCController.cpp:164: if
(!node->isV8CollectableDuringMinorGC() || (node->hasTagName(HTMLNames::imgTag)
&& static_cast<    HTMLImageElement*>(node)->hasPendingActivity())) {
On 2013/04/29 17:47:33, abarth wrote:
> Looks like you've got some extra spaces between the < and the HTMLImageElement

Oops. Done.

[commit-bot: I haz the power, 2013-05-02 09:47:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/14443007/40001

[commit-bot: I haz the power, 2013-05-02 10:12:17]

Change committed as 149585