Use Persistent to retain pointers on unittests

Use Persistent to retain pointers on unittests

This is a speculative fix for use-after-poison crashes on unittests.

BUG=607994
Committed: https://crrev.com/97ee4076649fa2160880d9e070430b743766b128
Cr-Commit-Position: refs/heads/master@{#390889}

[yhirano, 2016-05-02 03:42:21]

Description was changed from

==========
Use Persistent to retain pointers on unittests

BUG=607994
==========

to

==========
Use Persistent to retain pointers on unittests

This is a speculative fix for use-after-poison crashes on unittests.

BUG=607994
==========

[yhirano, 2016-05-02 03:42:22]

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

[haraken, 2016-05-02 03:46:41]

LGTM to try although I don't think this is related.

[yhirano1, 2016-05-02 03:51:14]

The CQ bit was checked by yhirano@google.com

[commit-bot: I haz the power, 2016-05-02 03:51:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1940763002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1940763002/1

[commit-bot: I haz the power, 2016-05-02 04:06:18]

Description was changed from

==========
Use Persistent to retain pointers on unittests

This is a speculative fix for use-after-poison crashes on unittests.

BUG=607994
==========

to

==========
Use Persistent to retain pointers on unittests

This is a speculative fix for use-after-poison crashes on unittests.

BUG=607994
==========

[commit-bot: I haz the power, 2016-05-02 04:06:18]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-02 04:07:26]

Description was changed from

==========
Use Persistent to retain pointers on unittests

This is a speculative fix for use-after-poison crashes on unittests.

BUG=607994
==========

to

==========
Use Persistent to retain pointers on unittests

This is a speculative fix for use-after-poison crashes on unittests.

BUG=607994

Committed: https://crrev.com/97ee4076649fa2160880d9e070430b743766b128
Cr-Commit-Position: refs/heads/master@{#390889}
==========

[commit-bot: I haz the power, 2016-05-02 04:07:27]

Patchset 1 (id:??) landed as
https://crrev.com/97ee4076649fa2160880d9e070430b743766b128
Cr-Commit-Position: refs/heads/master@{#390889}

[sof, 2016-05-02 07:38:31]

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

[sof, 2016-05-02 07:38:31]

sorry about the overhead... Cc: me on associated bug?

[sof, 2016-05-02 08:14:26]

On 2016/05/02 07:38:31, sof wrote:
> sorry about the overhead... Cc: me on associated bug?

thanks; it sounds as if you're working on this further, but eagerly finalize
MockClient?

[yhirano, 2016-05-06 05:40:57]

Sorry for the late response. 

On 2016/05/02 08:14:26, sof wrote:
> On 2016/05/02 07:38:31, sof wrote:
> > sorry about the overhead... Cc: me on associated bug?
> 
> thanks; it sounds as if you're working on this further, but eagerly finalize
> MockClient?

To call ~StrictMock<MockClient> timely, right?

[sof, 2016-05-06 06:46:03]

On 2016/05/06 05:40:57, yhirano wrote:
> Sorry for the late response. 
> 
> On 2016/05/02 08:14:26, sof wrote:
> > On 2016/05/02 07:38:31, sof wrote:
> > > sorry about the overhead... Cc: me on associated bug?
> > 
> > thanks; it sounds as if you're working on this further, but eagerly finalize
> > MockClient?
> 
> To call ~StrictMock<MockClient> timely, right?

Hmm, I'm not sure who releases who here :) but given that a MockClient is
accessed after it has been deemed garbage, eagerly releasing it seemed like a
cure. Looking closer, I don't think it would make a difference.

But, I now notice that ReadableStreamDataConsumerHandle::ReadingContext doesn't
keep a traced Member to its WebDataConsumerHandle::Client, but a bare pointer.
That's not too nice & something we try to avoid, as it could well be a heap
pointer, so if the lifetime of that client pointer could be clarified, that'd be
good.

[sof, 2016-05-09 05:26:37]

On 2016/05/06 06:46:03, sof wrote:
> On 2016/05/06 05:40:57, yhirano wrote:
> > Sorry for the late response. 
> > 
> > On 2016/05/02 08:14:26, sof wrote:
> > > On 2016/05/02 07:38:31, sof wrote:
> > > > sorry about the overhead... Cc: me on associated bug?
> > > 
> > > thanks; it sounds as if you're working on this further, but eagerly
finalize
> > > MockClient?
> > 
> > To call ~StrictMock<MockClient> timely, right?
> 
> Hmm, I'm not sure who releases who here :) but given that a MockClient is
> accessed after it has been deemed garbage, eagerly releasing it seemed like a
> cure. Looking closer, I don't think it would make a difference.
> 
> But, I now notice that ReadableStreamDataConsumerHandle::ReadingContext
doesn't
> keep a traced Member to its WebDataConsumerHandle::Client, but a bare pointer.
> That's not too nice & something we try to avoid, as it could well be a heap
> pointer, so if the lifetime of that client pointer could be clarified, that'd
be
> good.

If not already, could you create a bug on this bare Client pointer, please? We
should try to avoid having untraced to-heap pointers in the codebase.

[yhirano, 2016-05-09 05:29:20]

On 2016/05/09 05:26:37, sof wrote:
> On 2016/05/06 06:46:03, sof wrote:
> > On 2016/05/06 05:40:57, yhirano wrote:
> > > Sorry for the late response. 
> > > 
> > > On 2016/05/02 08:14:26, sof wrote:
> > > > On 2016/05/02 07:38:31, sof wrote:
> > > > > sorry about the overhead... Cc: me on associated bug?
> > > > 
> > > > thanks; it sounds as if you're working on this further, but eagerly
> finalize
> > > > MockClient?
> > > 
> > > To call ~StrictMock<MockClient> timely, right?
> > 
> > Hmm, I'm not sure who releases who here :) but given that a MockClient is
> > accessed after it has been deemed garbage, eagerly releasing it seemed like
a
> > cure. Looking closer, I don't think it would make a difference.
> > 
> > But, I now notice that ReadableStreamDataConsumerHandle::ReadingContext
> doesn't
> > keep a traced Member to its WebDataConsumerHandle::Client, but a bare
pointer.
> > That's not too nice & something we try to avoid, as it could well be a heap
> > pointer, so if the lifetime of that client pointer could be clarified,
that'd
> be
> > good.
> 
> If not already, could you create a bug on this bare Client pointer, please? We
> should try to avoid having untraced to-heap pointers in the codebase.

I'll send a CL linked to the same crbug shortly.

[yhirano1, 2016-05-09 05:33:03]

On 2016/05/09 05:29:20, yhirano wrote:
> On 2016/05/09 05:26:37, sof wrote:
> > On 2016/05/06 06:46:03, sof wrote:
> > > On 2016/05/06 05:40:57, yhirano wrote:
> > > > Sorry for the late response. 
> > > > 
> > > > On 2016/05/02 08:14:26, sof wrote:
> > > > > On 2016/05/02 07:38:31, sof wrote:
> > > > > > sorry about the overhead... Cc: me on associated bug?
> > > > > 
> > > > > thanks; it sounds as if you're working on this further, but eagerly
> > finalize
> > > > > MockClient?
> > > > 
> > > > To call ~StrictMock<MockClient> timely, right?
> > > 
> > > Hmm, I'm not sure who releases who here :) but given that a MockClient is
> > > accessed after it has been deemed garbage, eagerly releasing it seemed
like
> a
> > > cure. Looking closer, I don't think it would make a difference.
> > > 
> > > But, I now notice that ReadableStreamDataConsumerHandle::ReadingContext
> > doesn't
> > > keep a traced Member to its WebDataConsumerHandle::Client, but a bare
> pointer.
> > > That's not too nice & something we try to avoid, as it could well be a
heap
> > > pointer, so if the lifetime of that client pointer could be clarified,
> that'd
> > be
> > > good.
> > 
> > If not already, could you create a bug on this bare Client pointer, please?
We
> > should try to avoid having untraced to-heap pointers in the codebase.
> 
> I'll send a CL linked to the same crbug shortly.

Ah, I misunderstood, sorry. I'll create a bug...