Oilpan: improve ScriptPromiseProperty's argument types.

Oilpan: improve ScriptPromiseProperty<>'s argument types.

The ScriptPromiseProperty<> template will instantiate three fields holding
references to objects. In order to ensure lifetime safety, supply better
transition types where it is used.

R=ager@chromium.org,haraken@chromium.org
BUG=

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

[sof, 2014-07-16 09:59:41]

Please take a look.

An alternative way to handle the tracing from ScriptPromiseProperty; too much
type hackery?

[Mads Ager (chromium), 2014-07-16 10:25:03]

Yeah, I did consider this. It is clever and it will definitely work, but I do
find it to be a bit too much type hackery and it also makes the trace methods
less stylized. I personally prefer the #if ENABLE(OILPAN) usages for now. The
plugin should find it if we use them wrong and I don't see a case where the
ifdef in the trace method will not be enough.

[sof, 2014-07-16 10:29:33]

On 2014/07/16 10:25:03, Mads Ager (chromium) wrote:
> Yeah, I did consider this. It is clever and it will definitely work, but I do
> find it to be a bit too much type hackery and it also makes the trace methods
> less stylized. I personally prefer the #if ENABLE(OILPAN) usages for now. The
> plugin should find it if we use them wrong and I don't see a case where the
> ifdef in the trace method will not be enough.

ok; please notice that the use of ScriptPromiseProperty in *Test is currently
unsafe with the tracing that is now performed. m_rejected is an uninitialized
pointer.

[Mads Ager (chromium), 2014-07-16 10:31:34]

On 2014/07/16 10:29:33, sof wrote:
> On 2014/07/16 10:25:03, Mads Ager (chromium) wrote:
> > Yeah, I did consider this. It is clever and it will definitely work, but I
do
> > find it to be a bit too much type hackery and it also makes the trace
methods
> > less stylized. I personally prefer the #if ENABLE(OILPAN) usages for now.
The
> > plugin should find it if we use them wrong and I don't see a case where the
> > ifdef in the trace method will not be enough.
> 
> ok; please notice that the use of ScriptPromiseProperty in *Test is currently
> unsafe with the tracing that is now performed. m_rejected is an uninitialized
> pointer.

I think that is actually OK? Uninitialized pointers in heap allocated objects
are always 0. We only give back zero-filled memory?

[Mads Ager (chromium), 2014-07-16 10:32:16]

On 2014/07/16 10:31:34, Mads Ager (chromium) wrote:
> On 2014/07/16 10:29:33, sof wrote:
> > On 2014/07/16 10:25:03, Mads Ager (chromium) wrote:
> > > Yeah, I did consider this. It is clever and it will definitely work, but I
> do
> > > find it to be a bit too much type hackery and it also makes the trace
> methods
> > > less stylized. I personally prefer the #if ENABLE(OILPAN) usages for now.
> The
> > > plugin should find it if we use them wrong and I don't see a case where
the
> > > ifdef in the trace method will not be enough.
> > 
> > ok; please notice that the use of ScriptPromiseProperty in *Test is
currently
> > unsafe with the tracing that is now performed. m_rejected is an
uninitialized
> > pointer.
> 
> I think that is actually OK? Uninitialized pointers in heap allocated objects
> are always 0. We only give back zero-filled memory?

But, yeah, it should use RawPtrWillBeMember. Can we land that part of this
change? :)

[sof, 2014-07-16 10:34:47]

On 2014/07/16 10:32:16, Mads Ager (chromium) wrote:
> On 2014/07/16 10:31:34, Mads Ager (chromium) wrote:
> > On 2014/07/16 10:29:33, sof wrote:
> > > On 2014/07/16 10:25:03, Mads Ager (chromium) wrote:
> > > > Yeah, I did consider this. It is clever and it will definitely work, but
I
> > do
> > > > find it to be a bit too much type hackery and it also makes the trace
> > methods
> > > > less stylized. I personally prefer the #if ENABLE(OILPAN) usages for
now.
> > The
> > > > plugin should find it if we use them wrong and I don't see a case where
> the
> > > > ifdef in the trace method will not be enough.
> > > 
> > > ok; please notice that the use of ScriptPromiseProperty in *Test is
> currently
> > > unsafe with the tracing that is now performed. m_rejected is an
> uninitialized
> > > pointer.
> > 
> > I think that is actually OK? Uninitialized pointers in heap allocated
objects
> > are always 0. We only give back zero-filled memory?
> 
> But, yeah, it should use RawPtrWillBeMember. Can we land that part of this
> change? :)

m_rejected isn't initialized and a bare pointer. RawPtrWillBeMember<> wouldn't
be ideal either; RefPtrWillBeMember<>.

[Mads Ager (chromium), 2014-07-16 10:40:55]

On 2014/07/16 10:34:47, sof wrote:
> On 2014/07/16 10:32:16, Mads Ager (chromium) wrote:
> > On 2014/07/16 10:31:34, Mads Ager (chromium) wrote:
> > > On 2014/07/16 10:29:33, sof wrote:
> > > > On 2014/07/16 10:25:03, Mads Ager (chromium) wrote:
> > > > > Yeah, I did consider this. It is clever and it will definitely work,
but
> I
> > > do
> > > > > find it to be a bit too much type hackery and it also makes the trace
> > > methods
> > > > > less stylized. I personally prefer the #if ENABLE(OILPAN) usages for
> now.
> > > The
> > > > > plugin should find it if we use them wrong and I don't see a case
where
> > the
> > > > > ifdef in the trace method will not be enough.
> > > > 
> > > > ok; please notice that the use of ScriptPromiseProperty in *Test is
> > currently
> > > > unsafe with the tracing that is now performed. m_rejected is an
> > uninitialized
> > > > pointer.
> > > 
> > > I think that is actually OK? Uninitialized pointers in heap allocated
> objects
> > > are always 0. We only give back zero-filled memory?
> > 
> > But, yeah, it should use RawPtrWillBeMember. Can we land that part of this
> > change? :)
> 
> m_rejected isn't initialized and a bare pointer. RawPtrWillBeMember<> wouldn't
> be ideal either; RefPtrWillBeMember<>.

Yes, RefPtrWillBeMember makes sense here. We should update
ServiceWorkerContainer.h as well. It has the following typedef:

    typedef ScriptPromiseProperty<ServiceWorkerContainer*,
RefPtrWillBeMember<ServiceWorker>, RefPtrWillBeMember<ServiceWorker> >
ReadyProperty;


The ServiceWorkerContainer* should be RawPtrWillBeMember<ServiceWorkerContainer>
I think?

[sof, 2014-07-16 10:46:07]

On 2014/07/16 10:40:55, Mads Ager (chromium) wrote:
> On 2014/07/16 10:34:47, sof wrote:
> > On 2014/07/16 10:32:16, Mads Ager (chromium) wrote:
> > > On 2014/07/16 10:31:34, Mads Ager (chromium) wrote:
> > > > On 2014/07/16 10:29:33, sof wrote:
> > > > > On 2014/07/16 10:25:03, Mads Ager (chromium) wrote:
> > > > > > Yeah, I did consider this. It is clever and it will definitely work,
> but
> > I
> > > > do
> > > > > > find it to be a bit too much type hackery and it also makes the
trace
> > > > methods
> > > > > > less stylized. I personally prefer the #if ENABLE(OILPAN) usages for
> > now.
> > > > The
> > > > > > plugin should find it if we use them wrong and I don't see a case
> where
> > > the
> > > > > > ifdef in the trace method will not be enough.
> > > > > 
> > > > > ok; please notice that the use of ScriptPromiseProperty in *Test is
> > > currently
> > > > > unsafe with the tracing that is now performed. m_rejected is an
> > > uninitialized
> > > > > pointer.
> > > > 
> > > > I think that is actually OK? Uninitialized pointers in heap allocated
> > objects
> > > > are always 0. We only give back zero-filled memory?
> > > 
> > > But, yeah, it should use RawPtrWillBeMember. Can we land that part of this
> > > change? :)
> > 
> > m_rejected isn't initialized and a bare pointer. RawPtrWillBeMember<>
wouldn't
> > be ideal either; RefPtrWillBeMember<>.
> 
> Yes, RefPtrWillBeMember makes sense here. We should update
> ServiceWorkerContainer.h as well. It has the following typedef:
> 
>     typedef ScriptPromiseProperty<ServiceWorkerContainer*,
> RefPtrWillBeMember<ServiceWorker>, RefPtrWillBeMember<ServiceWorker> >
> ReadyProperty;
> 
> 
> The ServiceWorkerContainer* should be
RawPtrWillBeMember<ServiceWorkerContainer>
> I think?

That would be a better type; let's make this CL about ScriptPromiseProperty uses
only.

[sof, 2014-07-16 11:01:04]

On 2014/07/16 10:46:07, sof wrote:
> On 2014/07/16 10:40:55, Mads Ager (chromium) wrote:
> > On 2014/07/16 10:34:47, sof wrote:
> > > On 2014/07/16 10:32:16, Mads Ager (chromium) wrote:
> > > > On 2014/07/16 10:31:34, Mads Ager (chromium) wrote:
> > > > > On 2014/07/16 10:29:33, sof wrote:
> > > > > > On 2014/07/16 10:25:03, Mads Ager (chromium) wrote:
> > > > > > > Yeah, I did consider this. It is clever and it will definitely
work,
> > but
> > > I
> > > > > do
> > > > > > > find it to be a bit too much type hackery and it also makes the
> trace
> > > > > methods
> > > > > > > less stylized. I personally prefer the #if ENABLE(OILPAN) usages
for
> > > now.
> > > > > The
> > > > > > > plugin should find it if we use them wrong and I don't see a case
> > where
> > > > the
> > > > > > > ifdef in the trace method will not be enough.
> > > > > > 
> > > > > > ok; please notice that the use of ScriptPromiseProperty in *Test is
> > > > currently
> > > > > > unsafe with the tracing that is now performed. m_rejected is an
> > > > uninitialized
> > > > > > pointer.
> > > > > 
> > > > > I think that is actually OK? Uninitialized pointers in heap allocated
> > > objects
> > > > > are always 0. We only give back zero-filled memory?
> > > > 
> > > > But, yeah, it should use RawPtrWillBeMember. Can we land that part of
this
> > > > change? :)
> > > 
> > > m_rejected isn't initialized and a bare pointer. RawPtrWillBeMember<>
> wouldn't
> > > be ideal either; RefPtrWillBeMember<>.
> > 
> > Yes, RefPtrWillBeMember makes sense here. We should update
> > ServiceWorkerContainer.h as well. It has the following typedef:
> > 
> >     typedef ScriptPromiseProperty<ServiceWorkerContainer*,
> > RefPtrWillBeMember<ServiceWorker>, RefPtrWillBeMember<ServiceWorker> >
> > ReadyProperty;
> > 
> > 
> > The ServiceWorkerContainer* should be
> RawPtrWillBeMember<ServiceWorkerContainer>
> > I think?
> 
> That would be a better type; let's make this CL about ScriptPromiseProperty
uses
> only.

Done; tried to make the description more accurate.

[Mads Ager (chromium), 2014-07-16 11:10:43]

https://codereview.chromium.org/393773004/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.h (right):

https://codereview.chromium.org/393773004/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.h:90: typedef
ScriptPromiseProperty<RefPtrWillBeMember<ServiceWorkerContainer>,
RefPtrWillBeMember<ServiceWorker>, RefPtrWillBeMember<ServiceWorker> >
ReadyProperty;
It looks to me like this will leak through the tiny ref cycle:

ServiceWorkerContainer::m_ready -> ScriptProperty::m_host
      ^
      |                                      |
       --------------------------------------

It looks like m_ready is always set, so this needs to be RawPtrWillBeMember.

[haraken, 2014-07-16 11:17:50]

+dominicc

[sof, 2014-07-16 11:30:04]

https://codereview.chromium.org/393773004/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.h (right):

https://codereview.chromium.org/393773004/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.h:90: typedef
ScriptPromiseProperty<RefPtrWillBeMember<ServiceWorkerContainer>,
RefPtrWillBeMember<ServiceWorker>, RefPtrWillBeMember<ServiceWorker> >
ReadyProperty;
On 2014/07/16 11:10:43, Mads Ager (chromium) wrote:
> It looks to me like this will leak through the tiny ref cycle:
> 
> ServiceWorkerContainer::m_ready -> ScriptProperty::m_host
>       ^
>       |                                      |
>        --------------------------------------
> 
> It looks like m_ready is always set, so this needs to be RawPtrWillBeMember.

Ouch, a ref pointing straight back. Thanks much, fixed.

[Mads Ager (chromium), 2014-07-16 11:33:16]

LGTM

[sof, 2014-07-16 11:35:31]

On 2014/07/16 11:33:16, Mads Ager (chromium) wrote:
> LGTM

Thanks, will wait for bindings/ owner and dominicc's feedback.

[haraken, 2014-07-16 11:38:17]

LGTM

[sof, 2014-07-16 11:51:33]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-07-16 11:51:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/393773004/60001

[commit-bot: I haz the power, 2014-07-16 13:11:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17258)

[commit-bot: I haz the power, 2014-07-16 13:48:48]

Change committed as 178278