Oilpan: fix build after r182644.

Oilpan: fix build after r182644.

TBR=oilpan-reviews, ddorwin@chromium.org
BUG=
NOTRY=true

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

[haraken, 2014-09-25 03:35:27]

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

[haraken, 2014-09-25 03:35:27]

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

https://codereview.chromium.org/599093006/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:66:
RawPtrWillBePersistent<HTMLMediaElement> m_element;

Are you sure that the persistent handle doesn't cause leaks?
RawPtrWillBePersistent is very likely to cause a memory leak.

In the beginning, I want to understand whether the m_element should be a RawPtr
or a RefPtr in non-oilpan builds. If it should be a RefPtr, using a
RefPtrWillBePersistent makes sense. If it should be a RawPtr (i.e., using a
RefPtr causes a leak), that implies that using a Persistent causes a leak as
well.

https://codereview.chromium.org/599093006/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:67:
RawPtrWillBePersistent<MediaKeys> m_newMediaKeys;

MediaKeys are on-heap in both oilpan and non-oilpan, so it's wrong to use a
RawPtr in non-oilpan. I think Persistent<MediaKeys> is correct.

[DanielChow, 2014-09-25 03:48:32]

danielchow2011@gmail.com changed reviewers:
+ danielchow2011@gmail.com

[DanielChow, 2014-09-25 03:48:32]

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

https://codereview.chromium.org/599093006/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:66:
RawPtrWillBePersistent<HTMLMediaElement> m_element;
On 2014/09/25 03:35:27, haraken wrote:
> 
> Are you sure that the persistent handle doesn't cause leaks?
> RawPtrWillBePersistent is very likely to cause a memory leak.
> 
> In the beginning, I want to understand whether the m_element should be a
RawPtr
> or a RefPtr in non-oilpan builds. If it should be a RefPtr, using a
> RefPtrWillBePersistent makes sense. If it should be a RawPtr (i.e., using a
> RefPtr causes a leak), that implies that using a Persistent causes a leak as
> well.

I am not sure that the persistent handle doesn't cause leaks. I am running test
to checkout it.

https://codereview.chromium.org/599093006/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:67:
RawPtrWillBePersistent<MediaKeys> m_newMediaKeys;
On 2014/09/25 03:35:27, haraken wrote:
> 
> MediaKeys are on-heap in both oilpan and non-oilpan, so it's wrong to use a
> RawPtr in non-oilpan. I think Persistent<MediaKeys> is correct.

Done.

[sof, 2014-09-25 05:21:23]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2014-09-25 05:21:24]

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

https://codereview.chromium.org/599093006/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:66:
RawPtrWillBePersistent<HTMLMediaElement> m_element;
On 2014/09/25 03:48:32, DanielChow wrote:
> On 2014/09/25 03:35:27, haraken wrote:
> > 
> > Are you sure that the persistent handle doesn't cause leaks?
> > RawPtrWillBePersistent is very likely to cause a memory leak.
> > 
> > In the beginning, I want to understand whether the m_element should be a
> RawPtr
> > or a RefPtr in non-oilpan builds. If it should be a RefPtr, using a
> > RefPtrWillBePersistent makes sense. If it should be a RawPtr (i.e., using a
> > RefPtr causes a leak), that implies that using a Persistent causes a leak as
> > well.
> 
> I am not sure that the persistent handle doesn't cause leaks. I am running
test
> to checkout it.

Definitely worth checking, run them with --enable-leak-detection.

If there's a known upper bound of how long the promise will stay unfulfilled,
then a Persistent would be acceptable, I think. But clear the m_element
Persistent when done with it & at the earliest possible time.

So, is it known how long these Promises stay unfulfilled?

[haraken, 2014-09-25 05:45:02]

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

https://codereview.chromium.org/599093006/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:66:
RawPtrWillBePersistent<HTMLMediaElement> m_element;
On 2014/09/25 05:21:24, sof wrote:
> On 2014/09/25 03:48:32, DanielChow wrote:
> > On 2014/09/25 03:35:27, haraken wrote:
> > > 
> > > Are you sure that the persistent handle doesn't cause leaks?
> > > RawPtrWillBePersistent is very likely to cause a memory leak.
> > > 
> > > In the beginning, I want to understand whether the m_element should be a
> > RawPtr
> > > or a RefPtr in non-oilpan builds. If it should be a RefPtr, using a
> > > RefPtrWillBePersistent makes sense. If it should be a RawPtr (i.e., using
a
> > > RefPtr causes a leak), that implies that using a Persistent causes a leak
as
> > > well.
> > 
> > I am not sure that the persistent handle doesn't cause leaks. I am running
> test
> > to checkout it.
> 
> Definitely worth checking, run them with --enable-leak-detection.
> 
> If there's a known upper bound of how long the promise will stay unfulfilled,
> then a Persistent would be acceptable, I think. But clear the m_element
> Persistent when done with it & at the earliest possible time.
> 
> So, is it known how long these Promises stay unfulfilled?

If that's the case, I think we should use a RefPtr in non-oilpan builds.

[DanielChow, 2014-09-25 06:07:08]

On 2014/09/25 05:21:24, sof wrote:
>
https://codereview.chromium.org/599093006/diff/1/Source/modules/encryptedmedi...
> File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right):
> 
>
https://codereview.chromium.org/599093006/diff/1/Source/modules/encryptedmedi...
> Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:66:
> RawPtrWillBePersistent<HTMLMediaElement> m_element;
> On 2014/09/25 03:48:32, DanielChow wrote:
> > On 2014/09/25 03:35:27, haraken wrote:
> > > 
> > > Are you sure that the persistent handle doesn't cause leaks?
> > > RawPtrWillBePersistent is very likely to cause a memory leak.
> > > 
> > > In the beginning, I want to understand whether the m_element should be a
> > RawPtr
> > > or a RefPtr in non-oilpan builds. If it should be a RefPtr, using a
> > > RefPtrWillBePersistent makes sense. If it should be a RawPtr (i.e., using
a
> > > RefPtr causes a leak), that implies that using a Persistent causes a leak
as
> > > well.
> > 
> > I am not sure that the persistent handle doesn't cause leaks. I am running
> test
> > to checkout it.
> 
> Definitely worth checking, run them with --enable-leak-detection.
> 
> If there's a known upper bound of how long the promise will stay unfulfilled,
> then a Persistent would be acceptable, I think. But clear the m_element
> Persistent when done with it & at the earliest possible time.
> 
> So, is it known how long these Promises stay unfulfilled?

I have finished run-webkit-tests for LayoutTests\media with asan=1 and lsan=1,
and have run with --enable-leak-detection, there is no leaks. 
and I can not get into touch with the owner of the original patch:
https://codereview.chromium.org/423633002/#ps1
So, I am not sure how long the promise will stay unfulfilled for now.  
Should we invoke the release of Persistent at the end of
SetMediaKeysHandler::finish() and SetMediaKeysHandler::reportSetFailed ?

[sof, 2014-09-25 07:08:52]

On 2014/09/25 06:07:08, DanielChow wrote:
> On 2014/09/25 05:21:24, sof wrote:
> >
>
https://codereview.chromium.org/599093006/diff/1/Source/modules/encryptedmedi...
> > File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/599093006/diff/1/Source/modules/encryptedmedi...
> > Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:66:
> > RawPtrWillBePersistent<HTMLMediaElement> m_element;
> > On 2014/09/25 03:48:32, DanielChow wrote:
> > > On 2014/09/25 03:35:27, haraken wrote:
> > > > 
> > > > Are you sure that the persistent handle doesn't cause leaks?
> > > > RawPtrWillBePersistent is very likely to cause a memory leak.
> > > > 
> > > > In the beginning, I want to understand whether the m_element should be a
> > > RawPtr
> > > > or a RefPtr in non-oilpan builds. If it should be a RefPtr, using a
> > > > RefPtrWillBePersistent makes sense. If it should be a RawPtr (i.e.,
using
> a
> > > > RefPtr causes a leak), that implies that using a Persistent causes a
leak
> as
> > > > well.
> > > 
> > > I am not sure that the persistent handle doesn't cause leaks. I am running
> > test
> > > to checkout it.
> > 
> > Definitely worth checking, run them with --enable-leak-detection.
> > 
> > If there's a known upper bound of how long the promise will stay
unfulfilled,
> > then a Persistent would be acceptable, I think. But clear the m_element
> > Persistent when done with it & at the earliest possible time.
> > 
> > So, is it known how long these Promises stay unfulfilled?
> 
> I have finished run-webkit-tests for LayoutTests\media with asan=1 and lsan=1,
> and have run with --enable-leak-detection, there is no leaks. 
> and I can not get into touch with the owner of the original patch:
> https://codereview.chromium.org/423633002/#ps1
> So, I am not sure how long the promise will stay unfulfilled for now.  
> Should we invoke the release of Persistent at the end of
> SetMediaKeysHandler::finish() and SetMediaKeysHandler::reportSetFailed ?

The resolver should fall away when being fulfilled, so I don't think we need to
do anything special.

I'd suggest we change m_element to

  RefPtrWillBePersistent<HTMLMediaElement> m_element;

for now, but leave behind a FIXME to verify that the Promise lifetime is
'guaranteed' to short. There's a call to the embedder to handle step 3.3.3,
which is what makes this unclear.

[haraken, 2014-09-25 07:33:51]

On 2014/09/25 07:08:52, sof wrote:
> On 2014/09/25 06:07:08, DanielChow wrote:
> > On 2014/09/25 05:21:24, sof wrote:
> > >
> >
>
https://codereview.chromium.org/599093006/diff/1/Source/modules/encryptedmedi...
> > > File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/599093006/diff/1/Source/modules/encryptedmedi...
> > > Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:66:
> > > RawPtrWillBePersistent<HTMLMediaElement> m_element;
> > > On 2014/09/25 03:48:32, DanielChow wrote:
> > > > On 2014/09/25 03:35:27, haraken wrote:
> > > > > 
> > > > > Are you sure that the persistent handle doesn't cause leaks?
> > > > > RawPtrWillBePersistent is very likely to cause a memory leak.
> > > > > 
> > > > > In the beginning, I want to understand whether the m_element should be
a
> > > > RawPtr
> > > > > or a RefPtr in non-oilpan builds. If it should be a RefPtr, using a
> > > > > RefPtrWillBePersistent makes sense. If it should be a RawPtr (i.e.,
> using
> > a
> > > > > RefPtr causes a leak), that implies that using a Persistent causes a
> leak
> > as
> > > > > well.
> > > > 
> > > > I am not sure that the persistent handle doesn't cause leaks. I am
running
> > > test
> > > > to checkout it.
> > > 
> > > Definitely worth checking, run them with --enable-leak-detection.
> > > 
> > > If there's a known upper bound of how long the promise will stay
> unfulfilled,
> > > then a Persistent would be acceptable, I think. But clear the m_element
> > > Persistent when done with it & at the earliest possible time.
> > > 
> > > So, is it known how long these Promises stay unfulfilled?
> > 
> > I have finished run-webkit-tests for LayoutTests\media with asan=1 and
lsan=1,
> > and have run with --enable-leak-detection, there is no leaks. 
> > and I can not get into touch with the owner of the original patch:
> > https://codereview.chromium.org/423633002/#ps1
> > So, I am not sure how long the promise will stay unfulfilled for now.  
> > Should we invoke the release of Persistent at the end of
> > SetMediaKeysHandler::finish() and SetMediaKeysHandler::reportSetFailed ?
> 
> The resolver should fall away when being fulfilled, so I don't think we need
to
> do anything special.
> 
> I'd suggest we change m_element to
> 
>   RefPtrWillBePersistent<HTMLMediaElement> m_element;

Agreed with this idea.

[DanielChow, 2014-09-25 07:36:30]

Done, please have another look.

[haraken, 2014-09-25 07:37:38]

haraken@chromium.org changed reviewers:
+ yhirano@chromium.org

[haraken, 2014-09-25 07:37:39]

+yhirano

LGTM.

[sof, 2014-09-25 07:37:43]

sigbjornf@opera.com changed reviewers:
- yhirano@chromium.org

[sof, 2014-09-25 07:37:44]

thanks, looks good to me. will let others chime in.

[Daniel Chow, 2014-09-25 07:52:57]

The CQ bit was checked by zhishun.zhou@samsung.com

[commit-bot: I haz the power, 2014-09-25 07:53:20]

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

[commit-bot: I haz the power, 2014-09-25 07:54:04]

Committed patchset #3 (id:40001) as 182654

[yhirano, 2014-09-25 08:09:48]

I have a question. Currently SetMediaKeyHandler is AsyncInitializer only for
executing some operations asynchronous. Do you have a plan to change the
behavior?
If it won't change, making the operation a subclass of ScriptFunction (let us
call it AsyncOperation here) and creating a function that returns
 ScriptPromise.resolve().then(AsyncOperation::create(...))
looks simpler to me. Inheriting ScriptPromiseResolver is complex and I would
avoid that if possible.
What do you think?

[Daniel Chow, 2014-09-25 08:49:10]

zhishun.zhou@samsung.com changed reviewers:
+ jrummell@chromium.org

[Daniel Chow, 2014-09-25 08:49:11]

+jrummell

[jrummell, 2014-09-25 20:49:20]

> I have a question. Currently SetMediaKeyHandler is AsyncInitializer only for
> executing some operations asynchronous. Do you have a plan to change the
> behavior?

It will always be async. Clearing the existing value (which is not yet
supported on the Chromium side) has to wait until any existing decryption
work stops using the MediaKeys object (decryption runs on a different thread).

> If it won't change, making the operation a subclass of ScriptFunction (let us
> call it AsyncOperation here) and creating a function that returns
> ScriptPromise.resolve().then(AsyncOperation::create(...))
> looks simpler to me.

Won't ScriptPromise.resolve().then(...) resolve the promise in JavaScript when
AsyncOperation::create(...) finishes? We need the promise resolved only when
the AsyncOperation is all done.

> Inheriting ScriptPromiseResolver is complex and I would avoid that if
possible.
> What do you think?

Agreed. If there is a simpler way to do this then I don't mind changing it.

[yhirano1, 2014-09-26 09:56:53]

yhirano@google.com changed reviewers:
+ yhirano@chromium.org

[yhirano1, 2014-09-26 10:12:57]

On 2014/09/25 20:49:20, jrummell wrote:
> > I have a question. Currently SetMediaKeyHandler is AsyncInitializer only for
> > executing some operations asynchronous. Do you have a plan to change the
> > behavior?
> 
> It will always be async. Clearing the existing value (which is not yet
> supported on the Chromium side) has to wait until any existing decryption
> work stops using the MediaKeys object (decryption runs on a different thread).
> 
> > If it won't change, making the operation a subclass of ScriptFunction (let
us
> > call it AsyncOperation here) and creating a function that returns
> > ScriptPromise.resolve().then(AsyncOperation::create(...))
> > looks simpler to me.
> 
> Won't ScriptPromise.resolve().then(...) resolve the promise in JavaScript when
> AsyncOperation::create(...) finishes? We need the promise resolved only when
> the AsyncOperation is all done.
> 
Sorry, I overlooked 3.3.1-3 case. I don't have a good idea now.

> > Inheriting ScriptPromiseResolver is complex and I would avoid that if
> possible.
> > What do you think?
> 
> Agreed. If there is a simpler way to do this then I don't mind changing it.