Move encryptedmedia module to oilpan

Move encryptedmedia module to oilpan

This CL also changes the internal field where a persistent
handle is stored (for some complicated reason explained below).
Previously the persistent handle was stored in the first index
of custom internal fields. However, this causes an index
conflict in the following scenario:

- EventTarget stores its hidden value array in the first index
of its custom internal fields.
- MediaKeySession stores the persistent handle in the first index
of its custom internal fields.
- MediaKeySession inherits from EventTarget.
- This situation confuses an EventTarget object because the first index
of its custom internal fields stores different things depending
on subclass types.

The easiest way to avoid the confusion is to store the persistent
handle into the last index of all internal fields. This CL does that.

Note: We're planning to remove the internal field for the persistent handle
by sharing one internal field between the persistent handle (if the object
is in oilpan) and a C++ pointer of the DOM object (if the object is not in oilpan).

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

[haraken, 2014-02-06 12:08:40]

https://codereview.chromium.org/139173012/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/139173012/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:58:
REFCOUNTED_EVENT_TARGET(MediaKeySession);

This doesn't compile with the following error:

../../third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.h:58:5:
error: member 'deref' found in multiple base classes of different types
    REFCOUNTED_EVENT_TARGET(MediaKeySession);
    ^
../../third_party/WebKit/Source/core/events/EventTarget.h:243:44: note: expanded
from macro 'REFCOUNTED_EVENT_TARGET'
#define REFCOUNTED_EVENT_TARGET(className)
DEFINE_EVENT_TARGET_REFCOUNTING(RefCounted<className>)
                                           ^
../../third_party/WebKit/Source/core/events/EventTarget.h:237:54: note: expanded
from macro 'DEFINE_EVENT_TARGET_REFCOUNTING'
    virtual void derefEventTarget() OVERRIDE FINAL { deref(); } \
                                                     ^
../../third_party/WebKit/Source/heap/Heap.h:883:10: note: member found by
ambiguous name lookup
    void deref()
         ^
../../third_party/WebKit/Source/core/events/EventTarget.h:102:10: note: member
found by ambiguous name lookup
    void deref() { derefEventTarget(); }
         ^
In file included from gen/blink/bindings/V8GeneratedBindings04.cpp:54:
gen/blink/bindings/V8MediaKeySession.cpp:241:34: error: member 'deref' found in
multiple base classes of different types
    fromInternalPointer(object)->deref();
                                 ^
../../third_party/WebKit/Source/heap/Heap.h:883:10: note: member found by
ambiguous name lookup
    void deref()
         ^
../../third_party/WebKit/Source/core/events/EventTarget.h:102:10: note: member
found by ambiguous name lookup
    void deref() { derefEventTarget(); }


I wonder why RefCounted<MediaKeySession> doesn't cause the ambiguous error but
RefCountedWillBeGarbageCollectedFinalized<MediaKeySession> causes the ambiguous
error.

Would you give me advice? :)

[Mads Ager (chromium), 2014-02-06 12:28:46]

https://codereview.chromium.org/139173012/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/139173012/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:55: : public
RefCountedWillBeGarbageCollectedFinalized<MediaKeySession>, public
ScriptWrappable, public EventTargetWithInlineData, public
ContextLifecycleObserver
I don't think you can move this to GarbageCollectedFinalized before you have
moved all event targets to our heap. EventTargets are held in RefPtrs and I
guess the REFCOUNTED_EVENT_TARGET macro has something to do with that.

You need to use RefCountedWillBeRefCountedGarbageCollected until all event
targets are moved.

[haraken, 2014-02-06 12:36:46]

On 2014/02/06 12:28:46, Mads Ager (chromium) wrote:
>
https://codereview.chromium.org/139173012/diff/1/Source/modules/encryptedmedi...
> File Source/modules/encryptedmedia/MediaKeySession.h (right):
> 
>
https://codereview.chromium.org/139173012/diff/1/Source/modules/encryptedmedi...
> Source/modules/encryptedmedia/MediaKeySession.h:55: : public
> RefCountedWillBeGarbageCollectedFinalized<MediaKeySession>, public
> ScriptWrappable, public EventTargetWithInlineData, public
> ContextLifecycleObserver
> I don't think you can move this to GarbageCollectedFinalized before you have
> moved all event targets to our heap. EventTargets are held in RefPtrs and I
> guess the REFCOUNTED_EVENT_TARGET macro has something to do with that.
> 
> You need to use RefCountedWillBeRefCountedGarbageCollected until all event
> targets are moved.

Thanks mads. I used RefCountedWillBeRefCountedGarbageCollected but am still
hitting the same ambiguous error.

Looking.

[Mads Ager (chromium), 2014-02-06 12:45:23]

On 2014/02/06 12:36:46, haraken wrote:
> On 2014/02/06 12:28:46, Mads Ager (chromium) wrote:
> >
>
https://codereview.chromium.org/139173012/diff/1/Source/modules/encryptedmedi...
> > File Source/modules/encryptedmedia/MediaKeySession.h (right):
> > 
> >
>
https://codereview.chromium.org/139173012/diff/1/Source/modules/encryptedmedi...
> > Source/modules/encryptedmedia/MediaKeySession.h:55: : public
> > RefCountedWillBeGarbageCollectedFinalized<MediaKeySession>, public
> > ScriptWrappable, public EventTargetWithInlineData, public
> > ContextLifecycleObserver
> > I don't think you can move this to GarbageCollectedFinalized before you have
> > moved all event targets to our heap. EventTargets are held in RefPtrs and I
> > guess the REFCOUNTED_EVENT_TARGET macro has something to do with that.
> > 
> > You need to use RefCountedWillBeRefCountedGarbageCollected until all event
> > targets are moved.
> 
> Thanks mads. I used RefCountedWillBeRefCountedGarbageCollected but am still
> hitting the same ambiguous error.
> 
> Looking.

Here is the problem: #define REFCOUNTED_EVENT_TARGET(className)
DEFINE_EVENT_TARGET_REFCOUNTING(RefCounted<className>)

That should be RefCountedGarbageCollected with the oilpan flag.

[Mads Ager (chromium), 2014-02-06 12:47:43]

Maybe you can use
DEFINE_EVENT_TARGET_REFCOUNTING(RefCountedWIllBeRefCountedGarbageCollected<MediaKeySession>)
instead?


On Thu, Feb 6, 2014 at 1:45 PM, <ager@chromium.org> wrote:

> On 2014/02/06 12:36:46, haraken wrote:
>
>> On 2014/02/06 12:28:46, Mads Ager (chromium) wrote:
>> >
>>
>
> https://codereview.chromium.org/139173012/diff/1/Source/
> modules/encryptedmedia/MediaKeySession.h
>
>> > File Source/modules/encryptedmedia/MediaKeySession.h (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/139173012/diff/1/Source/
> modules/encryptedmedia/MediaKeySession.h#newcode55
>
>> > Source/modules/encryptedmedia/MediaKeySession.h:55: : public
>> > RefCountedWillBeGarbageCollectedFinalized<MediaKeySession>, public
>> > ScriptWrappable, public EventTargetWithInlineData, public
>> > ContextLifecycleObserver
>> > I don't think you can move this to GarbageCollectedFinalized before you
>> have
>> > moved all event targets to our heap. EventTargets are held in RefPtrs
>> and I
>> > guess the REFCOUNTED_EVENT_TARGET macro has something to do with that.
>> >
>> > You need to use RefCountedWillBeRefCountedGarbageCollected until all
>> event
>> > targets are moved.
>>
>
>  Thanks mads. I used RefCountedWillBeRefCountedGarbageCollected but am
>> still
>> hitting the same ambiguous error.
>>
>
>  Looking.
>>
>
> Here is the problem: #define REFCOUNTED_EVENT_TARGET(className)
> DEFINE_EVENT_TARGET_REFCOUNTING(RefCounted<className>)
>
> That should be RefCountedGarbageCollected with the oilpan flag.
>
> https://codereview.chromium.org/139173012/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[haraken, 2014-02-06 13:14:58]

On 2014/02/06 12:47:43, Mads Ager (chromium) wrote:
> Maybe you can use
>
DEFINE_EVENT_TARGET_REFCOUNTING(RefCountedWIllBeRefCountedGarbageCollected<MediaKeySession>)
> instead?

Thanks Mads! It worked!

I'm still seeing a couple of test failures, so I'll upload a CL after fixing the
failures.

[haraken, 2014-02-07 04:25:00]

Now all tests pass. PTAL.

https://codereview.chromium.org/139173012/diff/210001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/139173012/diff/210001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.pm:1074: # Persistent handle is stored
in the last internal field.

Please look at the CL description if you wonder why we need to change the
internal field where the persistent handle is stored.

[tkent, 2014-02-07 05:06:59]

lgtm

[haraken, 2014-02-07 05:07:29]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-02-07 05:07:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/139173012/210001

[commit-bot: I haz the power, 2014-02-07 05:07:54]

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

[commit-bot: I haz the power, 2014-02-07 05:07:55]

Failed to apply patch for Source/bindings/scripts/code_generator_v8.pm:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/scripts/code_generator_v8.pm
  Hunk #1 FAILED at 1066.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/bindings/scripts/code_generator_v8.pm.rej

Patch:       Source/bindings/scripts/code_generator_v8.pm
Index: Source/bindings/scripts/code_generator_v8.pm
diff --git a/Source/bindings/scripts/code_generator_v8.pm
b/Source/bindings/scripts/code_generator_v8.pm
index
b5ef7bf42f6c4bd5d149069cb65a78f8fa7e64f4..3155754c513519a2bf2ff566769231990cf17d46
100644
--- a/Source/bindings/scripts/code_generator_v8.pm
+++ b/Source/bindings/scripts/code_generator_v8.pm
@@ -1066,16 +1066,18 @@ sub GetInternalFields
     my $interface = shift;
 
     my @customInternalFields = ();
-    # If we have persistentHandleIndex, it should be at the first index of the
custom
-    # internal fileds.
-    if (IsWillBeGarbageCollectedType($interface->name)) {
-        push(@customInternalFields, "persistentHandleIndex");
-    }
     # Event listeners on DOM nodes are explicitly supported in the GC
controller.
     if (!InheritsInterface($interface, "Node") &&
         InheritsInterface($interface, "EventTarget")) {
         push(@customInternalFields, "eventListenerCacheIndex");
     }
+    # Persistent handle is stored in the last internal field.
+    # FIXME: Remove this internal field. Since we need either of a persistent
handle
+    # (if the object is in oilpan) or a C++ pointer to the DOM object (if the
object is not in oilpan),
+    # we can share the internal field between the two cases.
+    if (IsWillBeGarbageCollectedType($interface->name)) {
+        push(@customInternalFields, "persistentHandleIndex");
+    }
     return @customInternalFields;
 }

[haraken, 2014-02-07 05:08:07]

The CQ bit was unchecked by haraken@chromium.org

[haraken, 2014-02-07 05:21:55]

https://codereview.chromium.org/139173012/diff/210001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/139173012/diff/210001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.h:56: class MediaKeys : public
RefCountedWillBeGarbageCollected<MediaKeys>, public ScriptWrappable {

I noticed that this should be RefCountedWillBeGarbageCollectedFinalized, since
this object has OwnPtr and thus needs a destructor. Will fix and land.

[haraken, 2014-02-07 05:55:13]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-02-07 05:55:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/139173012/330001

[commit-bot: I haz the power, 2014-02-07 07:15:46]

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

[commit-bot: I haz the power, 2014-02-07 07:15:47]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[Mads Ager (chromium), 2014-02-07 07:19:24]

LGTM2

https://codereview.chromium.org/139173012/diff/330001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/139173012/diff/330001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.h:88:
RefPtrWillBePersistent<MediaKeySession> session;
I think this is fine. My guess would be that this is not very performance
critical and that we will not have many of these. The alternative would be to
make this a Member and define a trace method in InitializeNewSessionData which
will trace the session. When tracing MediaKeys would would then trace all the
pending new session data objects as well.

[haraken, 2014-02-07 07:22:53]

Thanks for review!

https://codereview.chromium.org/139173012/diff/330001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/139173012/diff/330001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.h:88:
RefPtrWillBePersistent<MediaKeySession> session;
On 2014/02/07 07:19:25, Mads Ager (chromium) wrote:
> I think this is fine. My guess would be that this is not very performance
> critical and that we will not have many of these. The alternative would be to
> make this a Member and define a trace method in InitializeNewSessionData which
> will trace the session. When tracing MediaKeys would would then trace all the
> pending new session data objects as well.

I'll keep the current CL since this wouldn't be performance critical, but your
point completely makes sense.

[haraken, 2014-02-07 07:23:09]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-02-07 07:23:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/139173012/330001

[zerny-chromium, 2014-02-07 08:30:07]

lgtm2

https://codereview.chromium.org/139173012/diff/330001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/139173012/diff/330001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.h:88:
RefPtrWillBePersistent<MediaKeySession> session;
I agree with Mads that this should be traced and
m_pendingInitializeNewSessionData traced from MediaKeys. We have support for
tracing non-pointer structures in collections so it should be a matter of making
sure tracing a Deque is supported.

[Mads Ager (chromium), 2014-02-07 08:52:34]

https://codereview.chromium.org/139173012/diff/330001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.h (right):

https://codereview.chromium.org/139173012/diff/330001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.h:88:
RefPtrWillBePersistent<MediaKeySession> session;
On 2014/02/07 08:30:07, zerny-chromium wrote:
> I agree with Mads that this should be traced and
> m_pendingInitializeNewSessionData traced from MediaKeys. We have support for
> tracing non-pointer structures in collections so it should be a matter of
making
> sure tracing a Deque is supported.

It looks like that code was not moved from the branch. I will do that once Erik
change to fix OffHeapCollectionTraceTrait is in.

[commit-bot: I haz the power, 2014-02-07 09:25:35]

Change committed as 166698

[haraken, 2014-02-07 09:32:35]

On 2014/02/07 08:52:34, Mads Ager (chromium) wrote:
>
https://codereview.chromium.org/139173012/diff/330001/Source/modules/encrypte...
> File Source/modules/encryptedmedia/MediaKeys.h (right):
> 
>
https://codereview.chromium.org/139173012/diff/330001/Source/modules/encrypte...
> Source/modules/encryptedmedia/MediaKeys.h:88:
> RefPtrWillBePersistent<MediaKeySession> session;
> On 2014/02/07 08:30:07, zerny-chromium wrote:
> > I agree with Mads that this should be traced and
> > m_pendingInitializeNewSessionData traced from MediaKeys. We have support for
> > tracing non-pointer structures in collections so it should be a matter of
> making
> > sure tracing a Deque is supported.
> 
> It looks like that code was not moved from the branch. I will do that once
Erik
> change to fix OffHeapCollectionTraceTrait is in.

Sounds reasonable. Once it's in, I'll make a change to
m_pendingInitializeNewSessionData.

[Nils Barth (inactive), 2014-02-10 07:16:07]

Python side at:
IDL compiler: sync Python to r166698 (and add test case)
https://codereview.chromium.org/154133003/