Delay posting Message event on MediaKeySession if no event handlers

Delay posting "message" event on MediaKeySession if no event handlers.

A recent CL to Promises changed when JavaScript is run when a promise
is resolved. In the past the JS ran immediately -- now it is run
using a microtask. For EME code, the "message" event is generated right
after creating the session. It relies on the JS running and binding
an event handler. With the recent CL, it is possible for the event to
be generated before the handler is bound, so the event is lost. This
change delays posting the "message" event if there are no event handlers
registered.

BUG=402766, 403121
TEST=Disabled EME browser_test passes

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

[jrummell, 2014-08-12 21:51:14]

PTAL. This is causing the browser_tests to be flaky.

[ddorwin, 2014-08-12 22:31:45]

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

https://codereview.chromium.org/465053002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:478:
m_asyncEventQueue->enqueueEvent(action->event().release());
Won't ".release()" this fail to compile with Oilpan enabled (event will be a
RawPtr)?

https://codereview.chromium.org/465053002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:503: // this message to
provide time for the JavaScript to run.
Is this guaranteed to be allow sufficient time for the JS to run?

This will also affect the (rare) case where there is no message handler during
normal operation. It's unfortunate that the behavior is different, but delaying
all messages seems unnecessary. You might at least note this here.

[jrummell, 2014-08-12 23:12:44]

Updated.

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

https://codereview.chromium.org/465053002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:478:
m_asyncEventQueue->enqueueEvent(action->event().release());
On 2014/08/12 22:31:45, ddorwin wrote:
> Won't ".release()" this fail to compile with Oilpan enabled (event will be a
> RawPtr)?

With Oilpan there is a RawPtr class that has all the methods that RefPtr has, so
it's not an issue. When it actually gets converted to T* all the methods will
have to go away.

https://codereview.chromium.org/465053002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:503: // this message to
provide time for the JavaScript to run.
On 2014/08/12 22:31:45, ddorwin wrote:
> Is this guaranteed to be allow sufficient time for the JS to run?
> 
For testing I added DCHECK(hasEventListeners()) in actionTimerFired() and it
succeeded. However, that doesn't guarantee that this will be enough time with
slower/faster/more/less processors, but it does give time for this to happen.

> This will also affect the (rare) case where there is no message handler during
> normal operation. It's unfortunate that the behavior is different, but
delaying
> all messages seems unnecessary. You might at least note this here.

Added comment. However, if they have no handler, it's just that the event will
get dropped a little later.

[ddorwin, 2014-08-12 23:23:09]

Any idea why this is only affecting the one test in the BUG instead of all of
them? Is it a timing issue?

This is fine to fix the current issue, but we should file a BUG to make sure we
have the correct behavior 100% of the time. We should consider making your test
DCHECK permanent by adding a member that is set when the .then handler runs. Can
we do something when the micro task completes?

[jrummell, 2014-08-12 23:51:17]

There is now a second test that fails enough to get disabled (see linked bug).
Opened crbug.com/403121 to investigate if there is a better way to handle this
case in the future.

[ddorwin, 2014-08-12 23:56:11]

LGTM to solve the current test failures. We'll track a permanent and guaranteed
fix in crbug.com/403121.

[xhwang, 2014-08-13 00:02:13]

lgtm

[jrummell, 2014-08-13 00:02:32]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-08-13 00:03:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jrummell@chromium.org/465053002/20001

[commit-bot: I haz the power, 2014-08-13 01:04:46]

Change committed as 180128