Create a TrivialURLRequestContextGetter to shim an existing URLRequestContext.

Create a TrivialURLRequestContextGetter to shim an existing URLRequestContext.

BUG=None
R=willchan@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272953

[Randy Smith (Not in Mondays), 2014-05-20 00:24:49]

Will, I'd like to play a quick game of Chromium jeopardy with you (A question in
the form of a CL.)  Is this kind of creation of an URLRequestContextGetter on
the fly when you only have access to an URLRequestContext an ok idiom?  

It's specifically needed when you're initializing on the IO thread (in my case,
in ProfileImplIOData::InitializeInternal) and you need to create something that
needs a URCGetter (because it uses URLFetcher) but you don't have a URCGetter
because you're on the IO thread.  

I figured it's done in enough places to warrant its own class.  One thing I
meant to do but forgot (but will fix if you're ok with the basic concept) is to
put a comment in saying that it's the consumers responsibility to make sure the
URC outlives the URCGetter being created.

WDYT?

[willchan no longer on Chromium, 2014-05-20 05:51:33]

Yeah, it's an OK idiom in the most general sense, but there are some other
issues to think about in each case, mostly around cancellation. How does profile
shutdown work?

Also, URLRequestContextGetter is primarily for proxying URLRequestContexts
across threads. Same with URLFetcher. Have you considered using URLRequest
directly? Perhaps the interface is too complicated for your use case?

[Randy Smith (Not in Mondays), 2014-05-20 12:23:02]

On 2014/05/20 05:51:33, willchan wrote:
> Yeah, it's an OK idiom in the most general sense, but there are some other
> issues to think about in each case, mostly around cancellation. How does
profile
> shutdown work?

So in the use case I'm aiming at (making SdchManager per-profile), and I believe
in the domain_reliability use case above, the URLRequestContext is associated
with a profile and is shut down with it.  I.e. the situation works (I imagine)
identically to if a URLRequestContext had been passed instead of a
URLRequestContextGetter.  Do you see a gotcha in that that I don't? 

My guess is that in get_server_time we don't do shutdown, we just exit, but I
haven't examined it closely to be sure.  It's just a support tool :-} :-|.

> Also, URLRequestContextGetter is primarily for proxying URLRequestContexts
> across threads. Same with URLFetcher. Have you considered using URLRequest
> directly? Perhaps the interface is too complicated for your use case?

That's basically it (that URLFetcher is the simpler interface). 
SdchDictionaryFetcher currently uses a URLFetcher, and I hadn't looked at what
would be involved in turning it into using a URLRequest.  "I just want to make
SdchManager per-profile!" and am trying to minimize the yak hair that clogs my
razor in process.

I should also call out (since I'm consulting with you :-}) that the idiom above
actually leaks the URLRequestContextGetter.  I don't think that's ideal, so I'm
going to try and tweak that model to return a
scoped_refptr<URLRequestContextGetter> instead. While I don't like reference
counting (though I probably like it more than you do :-})
URLRequestContextGetter is already reference counted, and if I'm creating a new
one and passing ownership (which is what I'm doing here) I think I need to use
those semantics.  So something else for you to object to if you wish, though I
think that gets into the question of whether or not URLRequestContextGetter
should be refcounted.

[Randy Smith (Not in Mondays), 2014-05-21 21:26:35]

Will: Now officially asking for a real review and stamp.

[willchan no longer on Chromium, 2014-05-22 22:49:01]

LGTM

I haven't looked at why URLRequestContextGetter would get leaked. I suspect
that's a problem with the user of this code, not with the code changed. Perhaps
if you showed me what you were doing with it in the SDCH code, then I'd
understand.

https://codereview.chromium.org/292063002/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_context_getter.h (right):

https://codereview.chromium.org/292063002/diff/20001/net/url_request/url_requ...
net/url_request/url_request_context_getter.h:75: };
DISALLOW_COPY_AND_ASSIGN

[Randy Smith (Not in Mondays), 2014-05-22 23:26:41]

On 2014/05/22 22:49:01, willchan (traveling 5.23-6.27) wrote:
> LGTM

Thanks!

> 
> I haven't looked at why URLRequestContextGetter would get leaked. I suspect
> that's a problem with the user of this code, not with the code changed.
Perhaps
> if you showed me what you were doing with it in the SDCH code, then I'd
> understand.

I went over the code in more detail, and it won't get leaked if the caller
stuffs a reference to it into a scoped_refptr<>.  If the caller doesn't, it
leaks.  Put differently, the code that constructs the object owns it, and is
responsible for deleting it, which when I write it that way seems so obvious as
to not be worth calling out--I was just making things more complicated and being
confused because URLRequestContextGetter is a ref counted class.  

That means there is an anti-pattern here--if you pass a URLRequestContextGetter
around by pointer to something that takes a reference to it and then releases
the reference, the object will go away.  But I don't actually think that has
anything to do with this CL, and I'm not going to worry about it.

(I will fix the issue you mention below before submission; just wanted to
respond to this now.)

> 
>
https://codereview.chromium.org/292063002/diff/20001/net/url_request/url_requ...
> File net/url_request/url_request_context_getter.h (right):
> 
>
https://codereview.chromium.org/292063002/diff/20001/net/url_request/url_requ...
> net/url_request/url_request_context_getter.h:75: };
> DISALLOW_COPY_AND_ASSIGN

[willchan no longer on Chromium, 2014-05-22 23:36:52]

That has nothing to do with your CL. That's a standard problem with
scoped_refptr's implicit conversions. It's a PITA to fix, and we've never
gotten around to it.


On Thu, May 22, 2014 at 4:26 PM, <rdsmith@chromium.org> wrote:

> On 2014/05/22 22:49:01, willchan (traveling 5.23-6.27) wrote:
>
>> LGTM
>>
>
> Thanks!
>
>
>
>  I haven't looked at why URLRequestContextGetter would get leaked. I
>> suspect
>> that's a problem with the user of this code, not with the code changed.
>>
> Perhaps
>
>> if you showed me what you were doing with it in the SDCH code, then I'd
>> understand.
>>
>
> I went over the code in more detail, and it won't get leaked if the caller
> stuffs a reference to it into a scoped_refptr<>.  If the caller doesn't, it
> leaks.  Put differently, the code that constructs the object owns it, and
> is
> responsible for deleting it, which when I write it that way seems so
> obvious as
> to not be worth calling out--I was just making things more complicated and
> being
> confused because URLRequestContextGetter is a ref counted class.
>
> That means there is an anti-pattern here--if you pass a
> URLRequestContextGetter
> around by pointer to something that takes a reference to it and then
> releases
> the reference, the object will go away.  But I don't actually think that
> has
> anything to do with this CL, and I'm not going to worry about it.
>
> (I will fix the issue you mention below before submission; just wanted to
> respond to this now.)
>
>
>
>
> https://codereview.chromium.org/292063002/diff/20001/net/
> url_request/url_request_context_getter.h
>
>> File net/url_request/url_request_context_getter.h (right):
>>
>
>
> https://codereview.chromium.org/292063002/diff/20001/net/
> url_request/url_request_context_getter.h#newcode75
>
>> net/url_request/url_request_context_getter.h:75: };
>> DISALLOW_COPY_AND_ASSIGN
>>
>
>
>
> https://codereview.chromium.org/292063002/
>

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

[Randy Smith (Not in Mondays), 2014-05-22 23:45:41]

On 2014/05/22 23:36:52, willchan (traveling 5.23-6.27) wrote:
> That has nothing to do with your CL. That's a standard problem with
> scoped_refptr's implicit conversions. It's a PITA to fix, and we've never
> gotten around to it.

The counter-argument is that if this class had a Create method that returned a
scoped_refptr<> and kept the constructor private, that anti-pattern wouldn't
exist (for this class).  Happy to do that if you want, but don't want to add
more burden to you before you disappear.  Have a good break!

> 
> 
> On Thu, May 22, 2014 at 4:26 PM, <mailto:rdsmith@chromium.org> wrote:
> 
> > On 2014/05/22 22:49:01, willchan (traveling 5.23-6.27) wrote:
> >
> >> LGTM
> >>
> >
> > Thanks!
> >
> >
> >
> >  I haven't looked at why URLRequestContextGetter would get leaked. I
> >> suspect
> >> that's a problem with the user of this code, not with the code changed.
> >>
> > Perhaps
> >
> >> if you showed me what you were doing with it in the SDCH code, then I'd
> >> understand.
> >>
> >
> > I went over the code in more detail, and it won't get leaked if the caller
> > stuffs a reference to it into a scoped_refptr<>.  If the caller doesn't, it
> > leaks.  Put differently, the code that constructs the object owns it, and
> > is
> > responsible for deleting it, which when I write it that way seems so
> > obvious as
> > to not be worth calling out--I was just making things more complicated and
> > being
> > confused because URLRequestContextGetter is a ref counted class.
> >
> > That means there is an anti-pattern here--if you pass a
> > URLRequestContextGetter
> > around by pointer to something that takes a reference to it and then
> > releases
> > the reference, the object will go away.  But I don't actually think that
> > has
> > anything to do with this CL, and I'm not going to worry about it.
> >
> > (I will fix the issue you mention below before submission; just wanted to
> > respond to this now.)
> >
> >
> >
> >
> > https://codereview.chromium.org/292063002/diff/20001/net/
> > url_request/url_request_context_getter.h
> >
> >> File net/url_request/url_request_context_getter.h (right):
> >>
> >
> >
> > https://codereview.chromium.org/292063002/diff/20001/net/
> > url_request/url_request_context_getter.h#newcode75
> >
> >> net/url_request/url_request_context_getter.h:75: };
> >> DISALLOW_COPY_AND_ASSIGN
> >>
> >
> >
> >
> > https://codereview.chromium.org/292063002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2014-05-22 23:50:37]

No, it doesn't matter. That's still buggy.

scoped_refptr<Foo> CreateFoo() {
  // This creates the refcounted object and increments the refcount to 1
  return make_scoped_refptr(new Foo);
}

// scoped_refptr<Foo> will implicitly convert to Foo*, which will destroy
the scoped_refptr<Foo> since it goes out of scope after the conversion,
thereby decrementing the refcount to 0 and freeing the Foo object
// foo will be created pointing to an already freed object
Foo* foo = CreateFoo();


On Thu, May 22, 2014 at 4:45 PM, <rdsmith@chromium.org> wrote:

> On 2014/05/22 23:36:52, willchan (traveling 5.23-6.27) wrote:
>
>> That has nothing to do with your CL. That's a standard problem with
>> scoped_refptr's implicit conversions. It's a PITA to fix, and we've never
>> gotten around to it.
>>
>
> The counter-argument is that if this class had a Create method that
> returned a
> scoped_refptr<> and kept the constructor private, that anti-pattern
> wouldn't
> exist (for this class).  Happy to do that if you want, but don't want to
> add
> more burden to you before you disappear.  Have a good break!
>
>
>
>
>  On Thu, May 22, 2014 at 4:26 PM, <mailto:rdsmith@chromium.org> wrote:
>>
>
>  > On 2014/05/22 22:49:01, willchan (traveling 5.23-6.27) wrote:
>> >
>> >> LGTM
>> >>
>> >
>> > Thanks!
>> >
>> >
>> >
>> >  I haven't looked at why URLRequestContextGetter would get leaked. I
>> >> suspect
>> >> that's a problem with the user of this code, not with the code changed.
>> >>
>> > Perhaps
>> >
>> >> if you showed me what you were doing with it in the SDCH code, then I'd
>> >> understand.
>> >>
>> >
>> > I went over the code in more detail, and it won't get leaked if the
>> caller
>> > stuffs a reference to it into a scoped_refptr<>.  If the caller
>> doesn't, it
>> > leaks.  Put differently, the code that constructs the object owns it,
>> and
>> > is
>> > responsible for deleting it, which when I write it that way seems so
>> > obvious as
>> > to not be worth calling out--I was just making things more complicated
>> and
>> > being
>> > confused because URLRequestContextGetter is a ref counted class.
>> >
>> > That means there is an anti-pattern here--if you pass a
>> > URLRequestContextGetter
>> > around by pointer to something that takes a reference to it and then
>> > releases
>> > the reference, the object will go away.  But I don't actually think that
>> > has
>> > anything to do with this CL, and I'm not going to worry about it.
>> >
>> > (I will fix the issue you mention below before submission; just wanted
>> to
>> > respond to this now.)
>> >
>> >
>> >
>> >
>> > https://codereview.chromium.org/292063002/diff/20001/net/
>> > url_request/url_request_context_getter.h
>> >
>> >> File net/url_request/url_request_context_getter.h (right):
>> >>
>> >
>> >
>> > https://codereview.chromium.org/292063002/diff/20001/net/
>> > url_request/url_request_context_getter.h#newcode75
>> >
>> >> net/url_request/url_request_context_getter.h:75: };
>> >> DISALLOW_COPY_AND_ASSIGN
>> >>
>> >
>> >
>> >
>> > https://codereview.chromium.org/292063002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/292063002/
>

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

[Randy Smith (Not in Mondays), 2014-05-23 15:40:13]

Ah, got it; thanks for the enlightenment.

Thomas, can you stamp the domain_reliability change?  Rietveld is confused when
it says I have ownership, and I'd rather not take advantage of the poor thing.

https://codereview.chromium.org/292063002/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_context_getter.h (right):

https://codereview.chromium.org/292063002/diff/20001/net/url_request/url_requ...
net/url_request/url_request_context_getter.h:75: };
On 2014/05/22 22:49:01, willchan (traveling 5.23-6.27) wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Deprecated (see juliatuttle), 2014-05-23 16:41:08]

On 2014/05/23 15:40:13, rdsmith wrote:
> Ah, got it; thanks for the enlightenment.
> 
> Thomas, can you stamp the domain_reliability change?  Rietveld is confused
when
> it says I have ownership, and I'd rather not take advantage of the poor thing.

domain_reliability lgtm.

[Randy Smith (Not in Mondays), 2014-05-27 12:31:10]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-05-27 12:31:37]

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

[commit-bot: I haz the power, 2014-05-27 12:55:28]

Change committed as 272953