Oilpan: Enable Oilpan by default in modules/encryptedmedia

Oilpan: Enable Oilpan by default in modules/encryptedmedia

This CL changes generated bindings so that both toV8(RawPtr<X>) and toV8(X*) are generated. Otherwise, if someone uses RawPtr<X> instead of X* in the code base, it can be bound to unexpected toV8() and a wrong wrapper can be returned to V8. For more details, see my comment in the patch set 3.

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

[haraken, 2014-04-25 09:47:32]

PTAL

https://codereview.chromium.org/257503003/diff/40001/Source/bindings/template...
File Source/bindings/templates/interface.h (left):

https://codereview.chromium.org/257503003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface.h:257: {% if gc_type !=
'GarbageCollectedObject' %}

It took me a long time to notice that I need to remove this line... If we have
the if branch, several encryptedmedia tests fail with strange errors.

Details:

- If we don't have the if branch, both toV8(MediaKeySession*) and
toV8(RawPtr<MediaKeySession>) are generated. If we have the if branch, only
toV8(MediaKeySession*) is generated.

- MediaKeySession is on-heap, so this CL replaces all RawPtr<MediaKeySession>
with MediaKeySession*, but the generated binding still uses
RawPtr<MediaKeySession> as follows:

  RawPtr<MediaKeySession> result = impl->create(...);
  return toV8(result.release());

The problem is that result.release() returns RawPtr<MediaKeySession> and that
MediaKeySession inherits from EventTarget which is still off-heap.

- As a result, toV8(result.release()) is bound to toV8(RawPtr<EventTarget>)
instead of toV8(MediaKeySession*). Then we end up returning a unexpected wrapper
to JS side.

I think there are two solutions to address this issue:

(a) Generate both toV8(RawPtr<X>) and toV8(X*) even if X is on-heap by default.

(b) Completely remove RawPtr<X> from the generated binding.


I think (a) is better for the following reasons:

- (b) will introduce a mess to the IDL compiler.

- Even if we completely remove RawPtr<X> from the generated binding, it's
possible that someone uses RawPtr<X> instead of X* in the code base. It won't
cause any compile issue and leads to the bug described above. To avoid this kind
of errors, it would be better to support both toV8(RawPtr<X>) and toV8(X*) for
safety.

Therefore this CL takes the approach (a).

[Mads Ager (chromium), 2014-04-28 11:02:31]

LGTM

https://codereview.chromium.org/257503003/diff/40001/Source/bindings/template...
File Source/bindings/templates/interface.h (left):

https://codereview.chromium.org/257503003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface.h:257: {% if gc_type !=
'GarbageCollectedObject' %}
On 2014/04/25 09:47:32, haraken wrote:
> 
> It took me a long time to notice that I need to remove this line... If we have
> the if branch, several encryptedmedia tests fail with strange errors.
> 
> Details:
> 
> - If we don't have the if branch, both toV8(MediaKeySession*) and
> toV8(RawPtr<MediaKeySession>) are generated. If we have the if branch, only
> toV8(MediaKeySession*) is generated.
> 
> - MediaKeySession is on-heap, so this CL replaces all RawPtr<MediaKeySession>
> with MediaKeySession*, but the generated binding still uses
> RawPtr<MediaKeySession> as follows:
> 
>   RawPtr<MediaKeySession> result = impl->create(...);
>   return toV8(result.release());
> 
> The problem is that result.release() returns RawPtr<MediaKeySession> and that
> MediaKeySession inherits from EventTarget which is still off-heap.
> 
> - As a result, toV8(result.release()) is bound to toV8(RawPtr<EventTarget>)
> instead of toV8(MediaKeySession*). Then we end up returning a unexpected
wrapper
> to JS side.
> 
> I think there are two solutions to address this issue:
> 
> (a) Generate both toV8(RawPtr<X>) and toV8(X*) even if X is on-heap by
default.
> 
> (b) Completely remove RawPtr<X> from the generated binding.
> 
> 
> I think (a) is better for the following reasons:
> 
> - (b) will introduce a mess to the IDL compiler.
> 
> - Even if we completely remove RawPtr<X> from the generated binding, it's
> possible that someone uses RawPtr<X> instead of X* in the code base. It won't
> cause any compile issue and leads to the bug described above. To avoid this
kind
> of errors, it would be better to support both toV8(RawPtr<X>) and toV8(X*) for
> safety.
> 
> Therefore this CL takes the approach (a).

I think this is OK for now. How much would (b) complicate the code generator. We
should move away from using RawPtr as much as possible when going to the actual
types...

[haraken, 2014-04-28 11:06:17]

On 2014/04/28 11:02:31, Mads Ager (chromium) wrote:
> LGTM
> 
>
https://codereview.chromium.org/257503003/diff/40001/Source/bindings/template...
> File Source/bindings/templates/interface.h (left):
> 
>
https://codereview.chromium.org/257503003/diff/40001/Source/bindings/template...
> Source/bindings/templates/interface.h:257: {% if gc_type !=
> 'GarbageCollectedObject' %}
> On 2014/04/25 09:47:32, haraken wrote:
> > 
> > It took me a long time to notice that I need to remove this line... If we
have
> > the if branch, several encryptedmedia tests fail with strange errors.
> > 
> > Details:
> > 
> > - If we don't have the if branch, both toV8(MediaKeySession*) and
> > toV8(RawPtr<MediaKeySession>) are generated. If we have the if branch, only
> > toV8(MediaKeySession*) is generated.
> > 
> > - MediaKeySession is on-heap, so this CL replaces all
RawPtr<MediaKeySession>
> > with MediaKeySession*, but the generated binding still uses
> > RawPtr<MediaKeySession> as follows:
> > 
> >   RawPtr<MediaKeySession> result = impl->create(...);
> >   return toV8(result.release());
> > 
> > The problem is that result.release() returns RawPtr<MediaKeySession> and
that
> > MediaKeySession inherits from EventTarget which is still off-heap.
> > 
> > - As a result, toV8(result.release()) is bound to toV8(RawPtr<EventTarget>)
> > instead of toV8(MediaKeySession*). Then we end up returning a unexpected
> wrapper
> > to JS side.
> > 
> > I think there are two solutions to address this issue:
> > 
> > (a) Generate both toV8(RawPtr<X>) and toV8(X*) even if X is on-heap by
> default.
> > 
> > (b) Completely remove RawPtr<X> from the generated binding.
> > 
> > 
> > I think (a) is better for the following reasons:
> > 
> > - (b) will introduce a mess to the IDL compiler.
> > 
> > - Even if we completely remove RawPtr<X> from the generated binding, it's
> > possible that someone uses RawPtr<X> instead of X* in the code base. It
won't
> > cause any compile issue and leads to the bug described above. To avoid this
> kind
> > of errors, it would be better to support both toV8(RawPtr<X>) and toV8(X*)
for
> > safety.
> > 
> > Therefore this CL takes the approach (a).
> 
> I think this is OK for now. How much would (b) complicate the code generator.
We
> should move away from using RawPtr as much as possible when going to the
actual
> types...

(b) is doable and the complexity would be acceptable.

However, I'm concerned that not having the RawPtr<> version will lead to
unexpected errors if we forget to replace RawPtr<T> with T*, so I think (a) is
safer at the moment. Of course we should drop the RawPtr<> version in the near
future.

[haraken, 2014-04-28 11:06:37]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-04-28 11:06:52]

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

[commit-bot: I haz the power, 2014-04-28 11:07:04]

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

[commit-bot: I haz the power, 2014-04-28 11:07:05]

Failed to apply patch for Source/modules/encryptedmedia/MediaKeySession.idl:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/encryptedmedia/MediaKeySession.idl
  Hunk #1 FAILED at 26.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/modules/encryptedmedia/MediaKeySession.idl.rej

Patch:       Source/modules/encryptedmedia/MediaKeySession.idl
Index: Source/modules/encryptedmedia/MediaKeySession.idl
diff --git a/Source/modules/encryptedmedia/MediaKeySession.idl
b/Source/modules/encryptedmedia/MediaKeySession.idl
index
37d26ec6e12cf894e3e8e7493e79a79aa1ea9534..50b163f6719421aca87da49a8b14e4d35d14ff46
100644
--- a/Source/modules/encryptedmedia/MediaKeySession.idl
+++ b/Source/modules/encryptedmedia/MediaKeySession.idl
@@ -26,7 +26,7 @@
 [
     ActiveDOMObject,
     RuntimeEnabled=EncryptedMedia,
-    WillBeGarbageCollected,
+    GarbageCollected,
     TypeChecking=Interface|Nullable|String
 ] interface MediaKeySession : EventTarget {
     // error state

[haraken, 2014-04-28 11:11:31]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-04-28 11:11:47]

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

[commit-bot: I haz the power, 2014-04-28 11:55:26]

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

[commit-bot: I haz the power, 2014-04-28 11:55:27]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[haraken, 2014-04-28 12:01:03]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-04-28 12:01:22]

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

[commit-bot: I haz the power, 2014-04-28 12:45:46]

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

[commit-bot: I haz the power, 2014-04-28 12:45:47]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[haraken, 2014-05-12 06:36:23]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-12 06:36:28]

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

[commit-bot: I haz the power, 2014-05-12 06:36:38]

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

[commit-bot: I haz the power, 2014-05-12 06:36:38]

Failed to apply patch for
Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h
  Hunk #1 FAILED at 75.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h.rej

Patch:       Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h
Index: Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h
diff --git a/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h
b/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h
index
c41125dbd0cc4dea1051358ebae495577779f291..380da83ac0c0e1d6be8b0f9c9b565cb8004c44b8
100644
--- a/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h
+++ b/Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h
@@ -75,7 +75,7 @@ private:
 
     EmeMode m_emeMode;
 
-    RefPtrWillBePersistent<MediaKeys> m_mediaKeys;
+    Persistent<MediaKeys> m_mediaKeys;
 };
 
 }

[haraken, 2014-05-12 07:20:40]

I didn't notice that this CL hasn't yet landed.

Landing this CL now, since it's important to land the bindings/ part in order to
wrap an EventTarget object into a correct type. (Actually I don't quite
understand why we were able to enable oilpan by default for MediaSource without
landing the bindings/ part of this CL.)

[haraken, 2014-05-12 07:20:45]

The CQ bit was checked by haraken@chromium.org

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

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

[commit-bot: I haz the power, 2014-05-12 08:29:42]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/7192)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6948)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6434)

[commit-bot: I haz the power, 2014-05-12 08:58:42]

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

[commit-bot: I haz the power, 2014-05-12 08:58:42]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[haraken, 2014-05-12 09:32:37]

tkent-san: Would you take a look at platform/?

I guess we need to land this CL (the bindings/ part) to make module/mediasource/
(already landed by keishi@) workable.

[haraken, 2014-05-13 08:02:40]

tkent-san@: ping? Would you take a look at platform/ ?

[tkent, 2014-05-13 08:10:46]

lgtm

[commit-bot: I haz the power, 2014-05-13 08:50:13]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6544)

[commit-bot: I haz the power, 2014-05-13 09:30:09]

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

[commit-bot: I haz the power, 2014-05-13 09:30:10]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6552)

[haraken, 2014-05-13 09:41:34]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-13 09:41:39]

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

[commit-bot: I haz the power, 2014-05-13 10:03:43]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6552)

[tkent, 2014-05-13 10:41:33]

On 2014/05/13 10:03:43, I haz the power (commit-bot) wrote:
> FYI, CQ is re-trying this CL (attempt #1).
> Please consider checking whether the failures are real,
> and report flakes to mailto:chrome-troopers@google.com.
> The failing builders are:
>   mac_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6552)

The failures look real.
> failures:
> media/encrypted-media/encrypted-media-lifetime-mediakeys.html
> media/encrypted-media/encrypted-media-lifetime-mediakeysession-reference.html

[commit-bot: I haz the power, 2014-05-13 10:45:43]

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

[commit-bot: I haz the power, 2014-05-13 10:45:44]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6557)

[haraken, 2014-05-13 10:46:14]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-13 10:46:22]

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

[commit-bot: I haz the power, 2014-05-13 11:25:03]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6562)

[commit-bot: I haz the power, 2014-05-13 12:02:37]

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

[commit-bot: I haz the power, 2014-05-13 12:02:38]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6564)

[haraken, 2014-05-13 12:19:53]

It looks like encrypted-media-lifetime-mediakeys.html is failing only on the
mac_rel bot. I'm taking a look.

[haraken, 2014-05-27 09:30:27]

On 2014/05/13 12:19:53, haraken wrote:
> It looks like encrypted-media-lifetime-mediakeys.html is failing only on the
> mac_rel bot. I'm taking a look.

I can't reproduce the error locally, so trying the mac_rel_bot again. It's
possible that the failure is related to conservative scanning and rewriting the
test with asyncGC() might fix the issue.

[sof, 2014-05-27 09:46:58]

On 2014/05/27 09:30:27, haraken wrote:
> On 2014/05/13 12:19:53, haraken wrote:
> > It looks like encrypted-media-lifetime-mediakeys.html is failing only on the
> > mac_rel bot. I'm taking a look.
> 
> I can't reproduce the error locally, so trying the mac_rel_bot again. It's
> possible that the failure is related to conservative scanning and rewriting
the
> test with asyncGC() might fix the issue.

Interesting; the MediaKeySession's weak reference to MediaKeys ought not keep
that object alive, but it appears to be still.

[Mads Ager (chromium), 2014-05-27 09:50:58]

On Tue, May 27, 2014 at 11:46 AM, <sigbjornf@opera.com> wrote:

> On 2014/05/27 09:30:27, haraken wrote:
>
>> On 2014/05/13 12:19:53, haraken wrote:
>> > It looks like encrypted-media-lifetime-mediakeys.html is failing only
>> on the
>> > mac_rel bot. I'm taking a look.
>>
>
>  I can't reproduce the error locally, so trying the mac_rel_bot again. It's
>> possible that the failure is related to conservative scanning and
>> rewriting
>>
> the
>
>> test with asyncGC() might fix the issue.
>>
>
> Interesting; the MediaKeySession's weak reference to MediaKeys ought not
> keep
> that object alive, but it appears to be still.
>

Sounds like conservative GC keeping the object alive as Sigbjorn suggests.
If the MediaKeys object is marked conservatively it will be kept alive. Is
the test performing explicit GCs that are now asyncGC? In that case, we
need to rewrite the test to use asyncGC before testing that something is
gone.

-- M

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

[haraken, 2014-05-27 12:27:41]

When I rewrite the test using asyncGC, it starts failing in my local linux :)
Investigating.

[haraken, 2014-05-27 14:51:16]

https://codereview.chromium.org/257503003/diff/180001/LayoutTests/media/encry...
File LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html
(right):

https://codereview.chromium.org/257503003/diff/180001/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-lifetime-mediakeys.html:71:
gc();

Hmm, I rewrote the test using asyncGC(), but the result looks very strange.

- If I use the original encrypted-media-lifetime-mediakeys.html as is, only the
mac_rel_bot fails.

- This patch set rewrites most gc()s with asyncGC()s. If I use this patch set,
my local Linux Debug build passes but all try bots fail.

- If I rewrite all gc()s with asyncGC()s, my local Linux Debug build and all try
bots fail. More specifically, if I rewrite the gc() at line 71 with asyncGC(),
the assert_equals at line 87 starts to fail. The expected number of
numActiveDOMObjectsCreated() is 0 but the actual number is 1.

This result is strange because the result means that the more we use asyncGC()s,
the less objects are collected.

I'll investigate more in details tomorrow, but any ideas are appreciated.

[haraken, 2014-05-28 05:31:26]

Here is a minimized test case. Investigating.

================================================

jsTestIsAsync = true;
step1();
var mediaKeys;
var startingActiveDOMObjectCount;
function numActiveDOMObjectsCreated() {
    return window.internals.activeDOMObjectCount(document) -
startingActiveDOMObjectCount;
}

function step1() {
    startingActiveDOMObjectCount =
window.internals.activeDOMObjectCount(document);
    mediaKeys = new MediaKeys("org.w3.clearkey");
    mediaKeys.createSession("video/webm", new Uint8Array([0]));
    shouldBe('numActiveDOMObjectsCreated()', '1');
    gc();
    mediaKeys = null;
    gc();
    shouldBe('numActiveDOMObjectsCreated()', '0');  // This passes.
    finishJSTest();
}

================================================

jsTestIsAsync = true;
step1();
var mediaKeys;
var startingActiveDOMObjectCount;
function numActiveDOMObjectsCreated() {
    return window.internals.activeDOMObjectCount(document) -
startingActiveDOMObjectCount;
}

function step1() {
    startingActiveDOMObjectCount =
window.internals.activeDOMObjectCount(document);
    mediaKeys = new MediaKeys("org.w3.clearkey");
    mediaKeys.createSession("video/webm", new Uint8Array([0]));
    shouldBe('numActiveDOMObjectsCreated()', '1');
    setTimeout(step2, 0);
}

function step2() {
    mediaKeys = null;
    gc();
    shouldBe('numActiveDOMObjectsCreated()', '0');  // This fails. The actual
value is 1.
    finishJSTest();
}

[haraken, 2014-05-28 07:06:34]

OK, I finally found the cause. In short, the test is relying on being executed
in one event loop, and thus using asynGC() breaks the test.

Details:

Consider the following code.

function step1() {
    mediaKeys = new MediaKeys("org.w3.clearkey");
    mediaKeys.createSession("video/webm", new Uint8Array([0]));
    shouldBe('numActiveDOMObjectsCreated()', '1');
    mediaKeys = null;
    asyncGC(step2);
}

function step2() {
    shouldBe('numActiveDOMObjectsCreated()', '0');  // This fails. The actual
value is 1.
}

(1) mediaKeys.createSession() creates a wrapper of MediaKeySession.
(2) mediaKeys.createSession() sets a Timer which triggers an event loop A later.
(3) asyncGC() is called.
(4) Before a full GC is fired, the event loop A is fired.
MediaKeys::initializeNewSessionTimerFired() is called.
(5) MediaKeys::initializeNewSessionTimerFired() calls back chromium code. The
chromium code calls back MediaKeySession::message(). MediaKeySession::message()
pushes an event to MediaKeySession::m_asyncEventQueue.
(6) The event loop A finishes.
(7) A full GC is fired.
(8) The GC tries to collect the wrapper of MediaKeySession and calls
MediaKeySession::hasPendingActivity(). However,
MediaKeySession::hasPendingActivity() returns true because there is a pending
event in MediaKeySession::m_asyncEventQueue.
(9) Consequently, the wrapper of MediaKeySession is not collected.
(10) step2() is fired. shouldBe('numActiveDOMObjectsCreated()', '0') fails.

hmm... do you have any idea?

[Mads Ager (chromium), 2014-05-28 07:55:09]

On 2014/05/28 07:06:34, haraken wrote:
> OK, I finally found the cause. In short, the test is relying on being executed
> in one event loop, and thus using asynGC() breaks the test.
> 
> Details:
> 
> Consider the following code.
> 
> function step1() {
>     mediaKeys = new MediaKeys("org.w3.clearkey");
>     mediaKeys.createSession("video/webm", new Uint8Array([0]));
>     shouldBe('numActiveDOMObjectsCreated()', '1');
>     mediaKeys = null;
>     asyncGC(step2);
> }
> 
> function step2() {
>     shouldBe('numActiveDOMObjectsCreated()', '0');  // This fails. The actual
> value is 1.
> }
> 
> (1) mediaKeys.createSession() creates a wrapper of MediaKeySession.
> (2) mediaKeys.createSession() sets a Timer which triggers an event loop A
later.
> (3) asyncGC() is called.
> (4) Before a full GC is fired, the event loop A is fired.
> MediaKeys::initializeNewSessionTimerFired() is called.
> (5) MediaKeys::initializeNewSessionTimerFired() calls back chromium code. The
> chromium code calls back MediaKeySession::message().
MediaKeySession::message()
> pushes an event to MediaKeySession::m_asyncEventQueue.
> (6) The event loop A finishes.
> (7) A full GC is fired.
> (8) The GC tries to collect the wrapper of MediaKeySession and calls
> MediaKeySession::hasPendingActivity(). However,
> MediaKeySession::hasPendingActivity() returns true because there is a pending
> event in MediaKeySession::m_asyncEventQueue.
> (9) Consequently, the wrapper of MediaKeySession is not collected.
> (10) step2() is fired. shouldBe('numActiveDOMObjectsCreated()', '0') fails.
> 
> hmm... do you have any idea?

We cannot create a non-flaky test with oilpan that tests that things disappear
within one task in the event loop. Can we rewrite the test to deal with the
pending message before expecting the mediakeys wrapper to be gone? Either that,
or create a separate test that works like that and put this one in
OilpanExpectations with a comment on why it is flaky with oilpan and cannot be
made non-flaky.

[haraken, 2014-05-28 11:23:45]

On 2014/05/28 07:55:09, Mads Ager (chromium) wrote:
> On 2014/05/28 07:06:34, haraken wrote:
> > OK, I finally found the cause. In short, the test is relying on being
executed
> > in one event loop, and thus using asynGC() breaks the test.
> > 
> > Details:
> > 
> > Consider the following code.
> > 
> > function step1() {
> >     mediaKeys = new MediaKeys("org.w3.clearkey");
> >     mediaKeys.createSession("video/webm", new Uint8Array([0]));
> >     shouldBe('numActiveDOMObjectsCreated()', '1');
> >     mediaKeys = null;
> >     asyncGC(step2);
> > }
> > 
> > function step2() {
> >     shouldBe('numActiveDOMObjectsCreated()', '0');  // This fails. The
actual
> > value is 1.
> > }
> > 
> > (1) mediaKeys.createSession() creates a wrapper of MediaKeySession.
> > (2) mediaKeys.createSession() sets a Timer which triggers an event loop A
> later.
> > (3) asyncGC() is called.
> > (4) Before a full GC is fired, the event loop A is fired.
> > MediaKeys::initializeNewSessionTimerFired() is called.
> > (5) MediaKeys::initializeNewSessionTimerFired() calls back chromium code.
The
> > chromium code calls back MediaKeySession::message().
> MediaKeySession::message()
> > pushes an event to MediaKeySession::m_asyncEventQueue.
> > (6) The event loop A finishes.
> > (7) A full GC is fired.
> > (8) The GC tries to collect the wrapper of MediaKeySession and calls
> > MediaKeySession::hasPendingActivity(). However,
> > MediaKeySession::hasPendingActivity() returns true because there is a
pending
> > event in MediaKeySession::m_asyncEventQueue.
> > (9) Consequently, the wrapper of MediaKeySession is not collected.
> > (10) step2() is fired. shouldBe('numActiveDOMObjectsCreated()', '0') fails.
> > 
> > hmm... do you have any idea?
> 
> We cannot create a non-flaky test with oilpan that tests that things disappear
> within one task in the event loop. Can we rewrite the test to deal with the
> pending message before expecting the mediakeys wrapper to be gone? Either
that,
> or create a separate test that works like that and put this one in
> OilpanExpectations with a comment on why it is flaky with oilpan and cannot be
> made non-flaky.

Unfortunately it is hard to rewrite the test to deal with the pending message,
because the only way to clean up the message is to stop the ExecutionContext.
(This sounds problematic because it means that the wrapper of MediaKeySession is
kept alive while the Document is alive.)

https://codereview.chromium.org/257503003/diff/220001/Source/platform/ThreadT...
File Source/platform/ThreadTimers.cpp (right):

https://codereview.chromium.org/257503003/diff/220001/Source/platform/ThreadT...
Source/platform/ThreadTimers.cpp:144:
ThreadState::current()->safePoint(ThreadState::NoHeapPointersOnStack);

The underlying issue is that a full GC is not invoked _immediately_ after
finishing the timer that called asyncGC(). (A full GC is invoked after finishing
the current event loop, which can contain a bunch of timers that can affect the
result of the GC e.g., MediaKeys::initializeNewSessionTimerFired.)

To address the issue, would it be possible to invoke a full GC _immediately_
after finishing the timer that called asyncGC()? The above change is doing that
and I confirmed that this fixes the test failure.

Note 1: I've not yet verified that it's OK to call safePoint with
NoHeapPointersOnStack from here.

Note 2: Either way, I'm planning to land this CL with a [Pass Failure] mark in
TestExpectation at the moment.

[haraken, 2014-05-28 11:40:11]

In the patch set 14, I added [Pass Failure] to the TestExpectation at the
moment. PTAL.

[sof, 2014-05-28 14:59:13]

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

https://codereview.chromium.org/257503003/diff/240001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.h:78: HTMLMediaElement* m_mediaElement;
Untraced and unused; shall we just remove it here?

[haraken, 2014-05-29 00:45:44]

On 2014/05/28 14:59:13, sof wrote:
>
https://codereview.chromium.org/257503003/diff/240001/Source/modules/encrypte...
> File Source/modules/encryptedmedia/MediaKeys.h (right):
> 
>
https://codereview.chromium.org/257503003/diff/240001/Source/modules/encrypte...
> Source/modules/encryptedmedia/MediaKeys.h:78: HTMLMediaElement*
m_mediaElement;
> Untraced and unused; shall we just remove it here?

Nice catch. This is used by HTMLMediaElementEncryptedMedia, so I made it a
Member and traced.

PTAL.

[sof, 2014-05-29 08:59:47]

On 2014/05/29 00:45:44, haraken wrote:
> On 2014/05/28 14:59:13, sof wrote:
> >
>
https://codereview.chromium.org/257503003/diff/240001/Source/modules/encrypte...
> > File Source/modules/encryptedmedia/MediaKeys.h (right):
> > 
> >
>
https://codereview.chromium.org/257503003/diff/240001/Source/modules/encrypte...
> > Source/modules/encryptedmedia/MediaKeys.h:78: HTMLMediaElement*
> m_mediaElement;
> > Untraced and unused; shall we just remove it here?
> 
> Nice catch. This is used by HTMLMediaElementEncryptedMedia, so I made it a
> Member and traced.
> 
> PTAL.

lgtm.

It isn't really used in HTMLMediaElementEncryptedMedia. I will remove it all as
part of moving MediaPlayer to the heap (need to touch HMEEM anyway.)

[haraken, 2014-05-29 09:42:08]

On 2014/05/29 08:59:47, sof wrote:
> On 2014/05/29 00:45:44, haraken wrote:
> > On 2014/05/28 14:59:13, sof wrote:
> > >
> >
>
https://codereview.chromium.org/257503003/diff/240001/Source/modules/encrypte...
> > > File Source/modules/encryptedmedia/MediaKeys.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/257503003/diff/240001/Source/modules/encrypte...
> > > Source/modules/encryptedmedia/MediaKeys.h:78: HTMLMediaElement*
> > m_mediaElement;
> > > Untraced and unused; shall we just remove it here?
> > 
> > Nice catch. This is used by HTMLMediaElementEncryptedMedia, so I made it a
> > Member and traced.
> > 
> > PTAL.
> 
> lgtm.
> 
> It isn't really used in HTMLMediaElementEncryptedMedia. I will remove it all
as
> part of moving MediaPlayer to the heap (need to touch HMEEM anyway.)

oh, understood. Then I'd be happy if you could take the work :)

[haraken, 2014-05-29 09:42:14]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-29 09:43:08]

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

[commit-bot: I haz the power, 2014-05-29 09:43:27]

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

[commit-bot: I haz the power, 2014-05-29 09:43:28]

Failed to apply patch for Source/modules/encryptedmedia/MediaKeySession.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/encryptedmedia/MediaKeySession.h
  Hunk #1 FAILED at 64.
  Hunk #2 succeeded at 112 (offset 1 line).
  Hunk #3 succeeded at 127 (offset 1 line).
  1 out of 3 hunks FAILED -- saving rejects to file
Source/modules/encryptedmedia/MediaKeySession.h.rej

Patch:       Source/modules/encryptedmedia/MediaKeySession.h
Index: Source/modules/encryptedmedia/MediaKeySession.h
diff --git a/Source/modules/encryptedmedia/MediaKeySession.h
b/Source/modules/encryptedmedia/MediaKeySession.h
index
8881515e22b94db9737b737c24d08bbec9d7589d..cfdc654516f56f6cec284d9f01c00b91f4f7eb80
100644
--- a/Source/modules/encryptedmedia/MediaKeySession.h
+++ b/Source/modules/encryptedmedia/MediaKeySession.h
@@ -64,11 +64,11 @@ class MediaKeys;
 // it may outlive any JavaScript references as long as the MediaKeys object is
alive.
 // The WebContentDecryptionModuleSession has the same lifetime as this object.
 class MediaKeySession FINAL
-    : public RefCountedWillBeRefCountedGarbageCollected<MediaKeySession>,
public ActiveDOMObject, public ScriptWrappable, public EventTargetWithInlineData
+    : public RefCountedGarbageCollected<MediaKeySession>, public
ActiveDOMObject, public ScriptWrappable, public EventTargetWithInlineData
     , private blink::WebContentDecryptionModuleSession::Client {
-   
DEFINE_EVENT_TARGET_REFCOUNTING(RefCountedWillBeRefCountedGarbageCollected<MediaKeySession>);
+   
DEFINE_EVENT_TARGET_REFCOUNTING(RefCountedGarbageCollected<MediaKeySession>);
 public:
-    static PassRefPtrWillBeRawPtr<MediaKeySession> create(ExecutionContext*,
blink::WebContentDecryptionModule*, WeakPtrWillBeRawPtr<MediaKeys>);
+    static MediaKeySession* create(ExecutionContext*,
blink::WebContentDecryptionModule*, MediaKeys*);
     virtual ~MediaKeySession();
 
     const String& keySystem() const { return m_keySystem; }
@@ -111,7 +111,7 @@ private:
         PendingAction(Type, PassRefPtr<Uint8Array> data);
     };
 
-    MediaKeySession(ExecutionContext*, blink::WebContentDecryptionModule*,
WeakPtrWillBeRawPtr<MediaKeys>);
+    MediaKeySession(ExecutionContext*, blink::WebContentDecryptionModule*,
MediaKeys*);
     void actionTimerFired(Timer<MediaKeySession>*);
 
     // blink::WebContentDecryptionModuleSession::Client
@@ -126,7 +126,7 @@ private:
     OwnPtr<blink::WebContentDecryptionModuleSession> m_session;
 
     // Used to determine if MediaKeys is still active.
-    WeakPtrWillBeWeakMember<MediaKeys> m_keys;
+    WeakMember<MediaKeys> m_keys;
 
     // Is the CDM finished with this session?
     bool m_isClosed;

[haraken, 2014-05-29 10:49:18]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-29 10:51:12]

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

[commit-bot: I haz the power, 2014-05-29 12:26:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9952)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9396)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9653)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10451)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/13529)

[commit-bot: I haz the power, 2014-05-29 12:49:53]

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

[commit-bot: I haz the power, 2014-05-29 12:49:54]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[haraken, 2014-06-02 01:23:36]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-06-02 01:23:45]

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

[commit-bot: I haz the power, 2014-06-02 02:11:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6628)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-02 02:12:17]

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

[commit-bot: I haz the power, 2014-06-02 02:12:18]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[haraken, 2014-06-02 02:28:30]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-06-02 02:28:39]

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

[commit-bot: I haz the power, 2014-06-02 03:24:52]

Change committed as 175248

[haraken, 2014-06-04 01:15:11]

acolwell@: Not related to Oilpan, I have a question about lifetime of
MediaKeySession. I found that a MediaKeySession object is not collected until a
Document stops, but is this intentional?

More details:

> function step1() {
>     mediaKeys = new MediaKeys("org.w3.clearkey");
>     mediaKeys.createSession("video/webm", new Uint8Array([0]));
>     shouldBe('numActiveDOMObjectsCreated()', '1');
>     mediaKeys = null;
>     asyncGC(step2);
> }
> 
> function step2() {
>     shouldBe('numActiveDOMObjectsCreated()', '0');  // This fails. The actual
> value is 1.
> }
> 
> (1) mediaKeys.createSession() creates a wrapper of MediaKeySession.
> (2) mediaKeys.createSession() sets a Timer which triggers an event loop A
later.
> (3) asyncGC() is called.
> (4) Before a full GC is fired, the event loop A is fired.
> MediaKeys::initializeNewSessionTimerFired() is called.
> (5) MediaKeys::initializeNewSessionTimerFired() calls back chromium code. The
> chromium code calls back MediaKeySession::message().
MediaKeySession::message()
> pushes an event to MediaKeySession::m_asyncEventQueue.
> (6) The event loop A finishes.
> (7) A full GC is fired.
> (8) The GC tries to collect the wrapper of MediaKeySession and calls
> MediaKeySession::hasPendingActivity(). However,
> MediaKeySession::hasPendingActivity() returns true because there is a pending
> event in MediaKeySession::m_asyncEventQueue.
> (9) Consequently, the wrapper of MediaKeySession is not collected.
> (10) step2() is fired. shouldBe('numActiveDOMObjectsCreated()', '0') fails.

As far as I read the code, MediaKeySession::m_asyncEventQueue is not flushed
until the Document stops. This means that the MediaKeySession object doesn't die
until the Document dies. I'd like to understand if this is an intentional
behavior.

[acolwell GONE FROM CHROMIUM, 2014-06-04 15:56:33]

On 2014/06/04 01:15:11, haraken wrote:
> acolwell@: Not related to Oilpan, I have a question about lifetime of
> MediaKeySession. I found that a MediaKeySession object is not collected until
a
> Document stops, but is this intentional?
> 
> More details:
> 
> > function step1() {
> >     mediaKeys = new MediaKeys("org.w3.clearkey");
> >     mediaKeys.createSession("video/webm", new Uint8Array([0]));
> >     shouldBe('numActiveDOMObjectsCreated()', '1');
> >     mediaKeys = null;
> >     asyncGC(step2);
> > }
> > 
> > function step2() {
> >     shouldBe('numActiveDOMObjectsCreated()', '0');  // This fails. The
actual
> > value is 1.
> > }
> > 
> > (1) mediaKeys.createSession() creates a wrapper of MediaKeySession.
> > (2) mediaKeys.createSession() sets a Timer which triggers an event loop A
> later.
> > (3) asyncGC() is called.
> > (4) Before a full GC is fired, the event loop A is fired.
> > MediaKeys::initializeNewSessionTimerFired() is called.
> > (5) MediaKeys::initializeNewSessionTimerFired() calls back chromium code.
The
> > chromium code calls back MediaKeySession::message().
> MediaKeySession::message()
> > pushes an event to MediaKeySession::m_asyncEventQueue.
> > (6) The event loop A finishes.
> > (7) A full GC is fired.
> > (8) The GC tries to collect the wrapper of MediaKeySession and calls
> > MediaKeySession::hasPendingActivity(). However,
> > MediaKeySession::hasPendingActivity() returns true because there is a
pending
> > event in MediaKeySession::m_asyncEventQueue.
> > (9) Consequently, the wrapper of MediaKeySession is not collected.
> > (10) step2() is fired. shouldBe('numActiveDOMObjectsCreated()', '0') fails.
> 
> As far as I read the code, MediaKeySession::m_asyncEventQueue is not flushed
> until the Document stops. This means that the MediaKeySession object doesn't
die
> until the Document dies. I'd like to understand if this is an intentional
> behavior.

haraken@: jrummell@ and or ddorwin@ should be able to help you with this. They
understand the EME code more than I do. This behavior is a little surprising to
me because the timer inside of m_asyncEventQueue should cause the event enqueued
to get dispatched and m_asyncEventQueue->hasPendingEvents() to eventually become
false.

[jrummell, 2014-06-04 19:23:20]

My understanding is the same as acolwell@'s. The event timer fires, and
should dispatch the event. If there are no listeners available (in your
example) the event is freed.

I think what is happening in your example is that after step 6, the blink
thread has the event timer firing queued up after GC. So when GC runs, the
event is still pending, and m_asyncEventQueue still has an entry,
preventing it from being collected. If step2 does asyncGC(step3), step3()
is the test, does that work as it should give time for the event timer to
be handled?

The other problem I've seen is that MediaKeySession objects remain as long
as JavaScript has a reference to it OR (MediaKeys is around AND the session
has not received a close() event). Since I don't know the order GC checks
objects, it is possible that GC checks MediaKeySession before MediaKeys and
doesn't free it due to the second condition. Later GC frees MediaKeys. So
it requires a second GC to actually free the MediaKeySession object. Doing
2 GC calls should fix this as well.



On Wed, Jun 4, 2014 at 8:56 AM, <acolwell@chromium.org> wrote:

> On 2014/06/04 01:15:11, haraken wrote:
>
>> acolwell@: Not related to Oilpan, I have a question about lifetime of
>> MediaKeySession. I found that a MediaKeySession object is not collected
>> until
>>
> a
>
>> Document stops, but is this intentional?
>>
>
>  More details:
>>
>
>  > function step1() {
>> >     mediaKeys = new MediaKeys("org.w3.clearkey");
>> >     mediaKeys.createSession("video/webm", new Uint8Array([0]));
>> >     shouldBe('numActiveDOMObjectsCreated()', '1');
>> >     mediaKeys = null;
>> >     asyncGC(step2);
>> > }
>> >
>> > function step2() {
>> >     shouldBe('numActiveDOMObjectsCreated()', '0');  // This fails. The
>>
> actual
>
>> > value is 1.
>> > }
>> >
>> > (1) mediaKeys.createSession() creates a wrapper of MediaKeySession.
>> > (2) mediaKeys.createSession() sets a Timer which triggers an event loop
>> A
>> later.
>> > (3) asyncGC() is called.
>> > (4) Before a full GC is fired, the event loop A is fired.
>> > MediaKeys::initializeNewSessionTimerFired() is called.
>> > (5) MediaKeys::initializeNewSessionTimerFired() calls back chromium
>> code.
>>
> The
>
>> > chromium code calls back MediaKeySession::message().
>> MediaKeySession::message()
>> > pushes an event to MediaKeySession::m_asyncEventQueue.
>> > (6) The event loop A finishes.
>> > (7) A full GC is fired.
>> > (8) The GC tries to collect the wrapper of MediaKeySession and calls
>> > MediaKeySession::hasPendingActivity(). However,
>> > MediaKeySession::hasPendingActivity() returns true because there is a
>>
> pending
>
>> > event in MediaKeySession::m_asyncEventQueue.
>> > (9) Consequently, the wrapper of MediaKeySession is not collected.
>> > (10) step2() is fired. shouldBe('numActiveDOMObjectsCreated()', '0')
>> fails.
>>
>
>  As far as I read the code, MediaKeySession::m_asyncEventQueue is not
>> flushed
>> until the Document stops. This means that the MediaKeySession object
>> doesn't
>>
> die
>
>> until the Document dies. I'd like to understand if this is an intentional
>> behavior.
>>
>
> haraken@: jrummell@ and or ddorwin@ should be able to help you with this.
> They
> understand the EME code more than I do. This behavior is a little
> surprising to
> me because the timer inside of m_asyncEventQueue should cause the event
> enqueued
> to get dispatched and m_asyncEventQueue->hasPendingEvents() to eventually
> become
> false.
>
> https://codereview.chromium.org/257503003/
>

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

[haraken, 2014-06-05 02:46:58]

> My understanding is the same as acolwell@'s. The event timer fires, and
> should dispatch the event. If there are no listeners available (in your
> example) the event is freed.
> 
> I think what is happening in your example is that after step 6, the blink
> thread has the event timer firing queued up after GC. So when GC runs, the
> event is still pending, and m_asyncEventQueue still has an entry,
> preventing it from being collected. If step2 does asyncGC(step3), step3()
> is the test, does that work as it should give time for the event timer to
> be handled?

Thanks jrummell@! I tried it but unfortunately it didn't work. Specifically, the
following test didn't work.

function step1() {
  mediaKeys = new MediaKeys("org.w3.clearkey");
  mediaKeys.createSession("video/webm", new Uint8Array([0]));
  shouldBe('numActiveDOMObjectsCreated()', '1');
  mediaKeys = null;
  setTimeout(step2, 100);  // asyncGC(step2) didn't work either.
}

function step2() {
  asyncGC(step3);
}

function step3() {
  shouldBe('numActiveDOMObjectsCreated()', '0');  // This fails. The actual
value is 1.
}

As far as I read MediaKeySession.cpp, there is no call path other than
MediaKeySession::stop() that pops events from m_asyncEventQueue. So I guess that
events queued in the m_asyncEventQueue won't be popped until the
ExeuctionContext stops. (Looks like MediaKeySession::actionTimerFired() does
nothing for m_asyncEventQueue.)


> The other problem I've seen is that MediaKeySession objects remain as long
> as JavaScript has a reference to it OR (MediaKeys is around AND the session
> has not received a close() event). Since I don't know the order GC checks
> objects, it is possible that GC checks MediaKeySession before MediaKeys and
> doesn't free it due to the second condition. Later GC frees MediaKeys. So
> it requires a second GC to actually free the MediaKeySession object. Doing
> 2 GC calls should fix this as well.

You're right, but this won't be a problem, since gc() or asyncGC() (which calls
GCController.collectAll()) triggers 7 V8 GCs :)

[acolwell GONE FROM CHROMIUM, 2014-06-05 04:28:01]

On 2014/06/05 02:46:58, haraken wrote:
> > My understanding is the same as acolwell@'s. The event timer fires, and
> > should dispatch the event. If there are no listeners available (in your
> > example) the event is freed.
> > 
> > I think what is happening in your example is that after step 6, the blink
> > thread has the event timer firing queued up after GC. So when GC runs, the
> > event is still pending, and m_asyncEventQueue still has an entry,
> > preventing it from being collected. If step2 does asyncGC(step3), step3()
> > is the test, does that work as it should give time for the event timer to
> > be handled?
> 
> Thanks jrummell@! I tried it but unfortunately it didn't work. Specifically,
the
> following test didn't work.
> 
> function step1() {
>   mediaKeys = new MediaKeys("org.w3.clearkey");
>   mediaKeys.createSession("video/webm", new Uint8Array([0]));
>   shouldBe('numActiveDOMObjectsCreated()', '1');
>   mediaKeys = null;
>   setTimeout(step2, 100);  // asyncGC(step2) didn't work either.
> }
> 
> function step2() {
>   asyncGC(step3);
> }
> 
> function step3() {
>   shouldBe('numActiveDOMObjectsCreated()', '0');  // This fails. The actual
> value is 1.
> }
> 
> As far as I read MediaKeySession.cpp, there is no call path other than
> MediaKeySession::stop() that pops events from m_asyncEventQueue. So I guess
that
> events queued in the m_asyncEventQueue won't be popped until the
> ExeuctionContext stops. (Looks like MediaKeySession::actionTimerFired() does
> nothing for m_asyncEventQueue.)

haraken: m_asyncEventQueue is a GenericEventQueue.
GenericEventQueue::timerFired() is where the events get popped. The timer gets
scheduled in GenericEventQueue::enqueueEvent(). Is it possible that something
else is somehow cancelling the timer in the GenericEventQueue?