Getting the right URLRequestContext for async revalidations

[Adam Rice]

I am adding support for async revalidations for the stale-while-revalidate Cache-Control directive to content::ResourceDispatcherHostImpl. The async revalidation may outlive the render process that issued the original request, but it will not outlive the profile.

Currently I am using the same URLRequestContext as the original request, using

info->filter()->GetContexts(new_request, &resource_context, &request_context);

at https://codereview.chromium.org/1041993004/diff/220001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode2489

This seems wrong in that it provides no guarantee that the URLRequestContext will outlive the render process. What is the correct way to get a URLRequestContext that shares the same HttpCache and network configuration as the original request, but will remain valid until the profile is destroyed? I am assuming that RemoveResourceContext() will be called when the profile is torn down, destroying my async revalidation requests since they share the ResourceContext as the original request.

Thanks,

Adam Rice

[Matt Menke]

There are no other URLRequestContexts that share caches with the main context (Other than perhaps the extensions context - not at all sure about that one).  So the context that gives you should last as long as the profile does.

It looks to me like RemoveResourceContext() is indeed called by ProfileIOData immediately before the contexts are destroyed.

--
You received this message because you are subscribed to the Google Groups "net-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to net-dev+u...@chromium.org.
To post to this group, send email to net...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/net-dev/CAHixhFr8Cbqg%3Dn%3DKcgj9YVo8ectQUoKmo3dKxHvpi7Fqh53f_g%40mail.gmail.com.

[David Benjamin]

Ownership of objects around the RDH is rather a mess (as you noted with thing seeming wrong), but CancelRequestsForContext should get called when the URLRequestContext is torn down, yes.

Please make sure Matt or I reviews this change. We're starting some discussions on how to fix the ownership and make the ResourceLoader stack better suit the needs of browser-initiated requests (and hopefully a future ServiceWorker implementation that complicates layering less) and should be kept informed of any interesting things happening to it.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/net-dev/CAEK7mvpgcXfhc5_740%3Dae2LMeTv%2BaF_aXP%2ByvudvMoSYN6zAQA%40mail.gmail.com.

[Adam Rice]

Thank you.

Perhaps I should add a browser test to protect against future changes of behaviour. IIUC, the lifecycle of these objects is out of the control of //content.

[David Benjamin]

Actually, why is this in the RDH at all? stale-while-revalidate is a notion about the HTTP cache, no? Above HttpTransaction, there's a ton of other logic at play (redirects, ServiceWorker, UI prompts for client auth, etc.). By launching these requests from the RDH, you'll sit above all these features when you want to sit below. Consider redirects, for example. The revalidated entry cache entry should *not* follow redirects, since caching is a property of an individual HTTP resource. If a request hits the //net HTTP cache, ServiceWorker has already done whatever mapping it wishes to do; you don't want to repeat that logic when revalidating. If the request needs to request auth that no longer exists, we cannot prompt; at this point, the user may have closed the tab or moved on to a different document, so the auth prompt will be out of context.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/net-dev/CAHixhFrXpCjxWP0o1FioD_U3%2BAO_oK8GfgVK7Ehh98E4bHpurQ%40mail.gmail.com.

[Adam Rice]

The reason it is in the RDH is because we need some of the logic from net::URLRequest to run. The biggest issue with implementing it in net::HttpCache was that any cookies set on an async revalidation response were lost. We also weren't processing the Strict-Transport-Security and Public-Key-Pins headers.

There were various other issues which you can read about in the original design doc if you are interested: https://docs.google.com/document/d/1DMCIMAKjyKeYiu69jlI5OsO2pGyAMb81XflYK4hxsNM/edit

UI prompts for client auth and SSL are disabled on the async revalidation pages. The interactions with ServiceWorker are still unexplored, but since I work very close to people working on ServiceWorker I'm not too worried about that.

[Ryan Sleevi]

On Thu, Jun 18, 2015 at 7:36 PM, 'Adam Rice' via net-dev <net...@chromium.org> wrote:

UI prompts for client auth and SSL are disabled on the async revalidation pages.

Wait, what? This could be really bad (namely, it could end up breaking client auth by entering bad sockets into the pool).

 

The interactions with ServiceWorker are still unexplored, but since I work very close to people working on ServiceWorker I'm not too worried about that.

I definitely want to echo David's concerns and say it does seem right that this should be pushed lower. While you work very close to the people working on it, we want to make sure the layering isn't dangerous or require close working, and it does seem like the current complexity leaves a decent amount of room for that to happen.

[David Benjamin]

The issues around HSTS, etc., seem surmountable. Moreover, they are bounded, whereas the stuff between //net and the web platform are unbounded, so having to add special-cases for each of these is the more complex option. Had you considered redirects, for instance? Putting it low-level means that, rather than having to special-case disable each of the automatic handling for various events one-by-one, you opt into them, which I think is correct for where this feature is situated in the stack.

Not to mention the RDH is a huge mess right now. I hope to address this once my current priority is sorted, but I don't think any of the current plans make it something for which stale-while-revalidate makes sense.

One possibility: move that code to HttpNetworkTransaction. It already needs special-cases to avoid replaying on cache responses, which is kind of odd. But I haven't looked closely yet at whether that would be correct. (I think the main indicative question there is: how are cookies and range requests supposed to interact?)

Another possibility: Factor that code out and have both HttpCache::Transaction and the URLRequest portions call them.

[David Benjamin]

On Thu, Jun 18, 2015 at 10:49 PM David Benjamin <davi...@chromium.org> wrote:

The issues around HSTS, etc., seem surmountable. Moreover, they are bounded, whereas the stuff between //net and the web platform are unbounded, so having to add special-cases for each of these is the more complex option. Had you considered redirects, for instance? Putting it low-level means that, rather than having to special-case disable each of the automatic handling for various events one-by-one, you opt into them, which I think is correct for where this feature is situated in the stack.

Not to mention the RDH is a huge mess right now. I hope to address this once my current priority is sorted, but I don't think any of the current plans make it something for which stale-while-revalidate makes sense.

One possibility: move that code to HttpNetworkTransaction. It already needs special-cases to avoid replaying on cache responses, which is kind of odd. But I haven't looked closely yet at whether that would be correct. (I think the main indicative question there is: how are cookies and range requests supposed to interact?)

Another possibility: Factor that code out and have both HttpCache::Transaction and the URLRequest portions call them.

Er, that was unclear. "that code" is HSTS, etc. And by "HttpCache::Transaction" I mean the hypothetical stale-while-revalidate implementation sitting somewhere around the HttpCache level.

David
 

[Adam Rice]

On 19 June 2015 at 11:40, Ryan Sleevi <rsl...@chromium.org> wrote:

Wait, what? This could be really bad (namely, it could end up breaking client auth by entering bad sockets into the pool).

do_not_prompt_for_login is set in the ResourceRequestInfoImpl object for the request, which means that the request is cancelled if auth is requested.

I've just noticed that SSL client certificate requests are actually honoured, which could be bad if they pop up UI. However, it doesn't appear that the UI requires the client tab to still exist (as far as I can tell it doesn't even have a notion of being associated with a tab), so it will work. My original intent was just to cancel the request if a client certificate is requested, as is done for prerendering.

[Ryan Sleevi]

On Thu, Jun 18, 2015 at 7:58 PM, Adam Rice <ri...@google.com> wrote:

On 19 June 2015 at 11:40, Ryan Sleevi <rsl...@chromium.org> wrote:

Wait, what? This could be really bad (namely, it could end up breaking client auth by entering bad sockets into the pool).

do_not_prompt_for_login is set in the ResourceRequestInfoImpl object for the request, which means that the request is cancelled if auth is requested.

So then that's not the same as what our existing logic does, and would also break on proxies.

 

I've just noticed that SSL client certificate requests are actually honoured, which could be bad if they pop up UI. However, it doesn't appear that the UI requires the client tab to still exist (as far as I can tell it doesn't even have a notion of being associated with a tab), so it will work.

Yes it is associated with a tab and do need to prompt UI.

If they just get continued (via sending a NULL cert), and you're using the URLRequestContext, then we're entering bad sockets into the pool.

 

My original intent was just to cancel the request if a client certificate is requested, as is done for prerendering.

I do want to echo again that it doesn't seem like this is going down the right design path. I realize this is meant for a data gathering experiment, but there's definitely concerns about how to rationalize this behaviour and how it works with the overall networking layer.

The very notion of "Stale while Revalidate" is problematic if we don't have a good answer for dealing with background requests. This is very much an issue that Service Workers face as well, and by failing to address these, we leave a very non-deterministic behaviour for devs & operators.

This is very similar to some of the issues going on with XHR, with websockets, with fetch, and with service workers (e.g. https://code.google.com/p/chromium/issues/detail?id=329884#c26 ), and my fear is if we keep punting for minimum viable products, we're going to find it's a stop-ship bug at some later point (I've seen this happen several times now, and that's why I'm concerned)

[David Benjamin]

Other issues to consider:

What happens if the user has an extension which changes the request? By the time a request has hit the HttpCache, all of that processing has already happened. If you start at URLRequest or above, the extension will re-run everything, and so you may end up revalidating the wrong thing. Skipping the extension is fine here because you're already working with the post-extensions parameters. Just like HttpCache::Transaction doesn't (and shouldn't) go back and ask extensions for reprocessing every time it needs to make an HTTP network request (it may make several).

Is the request associated with a RenderFrame? (In the CL, it was.) That'll trigger UI in various other parts of Chrome that hook into the RDH. This part is hard to reason about because we haven't historically had very strong abstractions there (indeed, it and the ServiceWorker concerns are the primary motivation for my ResourceLoader redesign) and it's grown organically. It's also very unbounded as random new Chrome features tend to hook into it. It also bounds the lifetime to the RenderFrame without the RenderFrame knowing about it, which is wrong. The RDH's purpose in life is to maintain ownership of requests on behalf of child processes because you can't have cross-process scoped_ptrs.

What happens to requests outside the RDH?

[Adam Rice]

I would rather have discussions take place on the new implementation design doc https://docs.google.com/document/d/1nBhr25nSJgoyAh4S1-U5h2sH70Iz4RR0NAfXNL79G5Y/edit so that they are in context and we can easiy see what has and has not been resolved.

Having already implemented stale-while-revalidate in net::HttpCache and found out the hard way that it cannot work, this is a sore point for me and I would rather not pick over old scabs.

It's true that many problems can be solved given sufficiently large rewrites of //net, however I believe the fundamental problem of object lifetimes cannot be solved in any reasonable timeframe.

The fundamental problem of object lifetimes is that //net code has no control over, nor visibility into, the lifetime of URLRequestContext. When a URLRequest exists that uses a particular URLRequestContext, then //net code can assume that the URLRequestContext is valid. When no URLRequest exists, then //net code can make no assumptions about the validity of the URLRequestContext.

What this means in effect is that //net code can create no background requests whose lifetime is not chained to the lifetime of a URLRequest.

Specifically, //net code cannot create async revalidations.

I was not able to explain the increase in crash rate at the time when I experimentally enabled the old stale-while-revalidation implementation, however now I believe it is directly attributable to this lifetime issue.

The lowest-level piece of code that does have visibility into the lifetime of net::URLRequestContext objects is content::ResourceDispatcherHostImpl. The stale-while-revalidate code has to run on every normal request, so I don't believe adding an extra level of abstraction to support it is viable from a performance perspective.

[David Benjamin]

On Mon, Jun 22, 2015 at 1:55 PM 'Adam Rice' via net-dev <net...@chromium.org> wrote:

I would rather have discussions take place on the new implementation design doc https://docs.google.com/document/d/1nBhr25nSJgoyAh4S1-U5h2sH70Iz4RR0NAfXNL79G5Y/edit so that they are in context and we can easiy see what has and has not been resolved.

Sure. We can move most of it there. Though I do find that sufficiently high-level discussions are awkward to do on a document. It seems better suited for smaller discussions on low-level details.
 

Having already implemented stale-while-revalidate in net::HttpCache and found out the hard way that it cannot work, this is a sore point for me and I would rather not pick over old scabs.

It's true that many problems can be solved given sufficiently large rewrites of //net, however I believe the fundamental problem of object lifetimes cannot be solved in any reasonable timeframe.

The fundamental problem of object lifetimes is that //net code has no control over, nor visibility into, the lifetime of URLRequestContext. When a URLRequest exists that uses a particular URLRequestContext, then //net code can assume that the URLRequestContext is valid. When no URLRequest exists, then //net code can make no assumptions about the validity of the URLRequestContext.

What this means in effect is that //net code can create no background requests whose lifetime is not chained to the lifetime of a URLRequest.

Specifically, //net code cannot create async revalidations.

//net knows when its own objects are destroyed because //net has the destructor. In the previous HttpCache version, who owned the async revalidations? Were they just fired off somewhere or owned by an object?

Certainly just being fired off somewhere won't work, but being owned by, say, HttpCache, should be fine. How did the previous version work? Was it using URLRequest for async revalidations or HttpNetworkTransaction and HttpCache::Transaction. If it was using URLRequest, that would certainly result in lifetime difficulties since nothing promises the lower-level objects get destroyed at the same time.

But if it was using the HttpTransaction layer (as I think it should, since you don't want to participate in redirects or NetworkDelegate), I don't see a problem. The net/http layer isn't even aware of URLRequestContext at all, just the lower-level objects hanging off of it.

If there's any point at which, say, HttpCache exists, but the pointers in its HttpNetworkSession are invalid, that's a bug and we should fix it. Just as URLRequest may freely assume its URLRequestContext outlives it, HttpCache should be able to freely assume all of its unowned parameters outlive it.
 

I was not able to explain the increase in crash rate at the time when I experimentally enabled the old stale-while-revalidation implementation, however now I believe it is directly attributable to this lifetime issue.

Do you have a sample stack trace? Or was it just an overall crash rate increase without any of the traces being helpful?
 

The lowest-level piece of code that does have visibility into the lifetime of net::URLRequestContext objects is content::ResourceDispatcherHostImpl. The stale-while-revalidate code has to run on every normal request, so I don't believe adding an extra level of abstraction to support it is viable from a performance perspective.

I don't believe an extra level of abstraction is needed or desirable. (Though the RDH does need work which will probably end up doing that. It's orthogonal though.) I don't think it should live above URLRequestContext at all. While most of the concerns I have are due to being part of RDH, even being above URLRequest will incur some problems: e.g., extensions should not see these revalidations just like they don't see the revalidations internal to an HttpCache::Transaction.

David
 

On 20 June 2015 at 01:02, David Benjamin <davi...@chromium.org> wrote:

Other issues to consider:

What happens if the user has an extension which changes the request? By the time a request has hit the HttpCache, all of that processing has already happened. If you start at URLRequest or above, the extension will re-run everything, and so you may end up revalidating the wrong thing. Skipping the extension is fine here because you're already working with the post-extensions parameters. Just like HttpCache::Transaction doesn't (and shouldn't) go back and ask extensions for reprocessing every time it needs to make an HTTP network request (it may make several).

Is the request associated with a RenderFrame? (In the CL, it was.) That'll trigger UI in various other parts of Chrome that hook into the RDH. This part is hard to reason about because we haven't historically had very strong abstractions there (indeed, it and the ServiceWorker concerns are the primary motivation for my ResourceLoader redesign) and it's grown organically. It's also very unbounded as random new Chrome features tend to hook into it. It also bounds the lifetime to the RenderFrame without the RenderFrame knowing about it, which is wrong. The RDH's purpose in life is to maintain ownership of requests on behalf of child processes because you can't have cross-process scoped_ptrs.

What happens to requests outside the RDH?

On Thu, Jun 18, 2015 at 11:04 PM Ryan Sleevi <rsl...@chromium.org> wrote:

On Thu, Jun 18, 2015 at 7:58 PM, Adam Rice <ri...@google.com> wrote:

On 19 June 2015 at 11:40, Ryan Sleevi <rsl...@chromium.org> wrote:

Wait, what? This could be really bad (namely, it could end up breaking client auth by entering bad sockets into the pool).

do_not_prompt_for_login is set in the ResourceRequestInfoImpl object for the request, which means that the request is cancelled if auth is requested.

So then that's not the same as what our existing logic does, and would also break on proxies.

 

I've just noticed that SSL client certificate requests are actually honoured, which could be bad if they pop up UI. However, it doesn't appear that the UI requires the client tab to still exist (as far as I can tell it doesn't even have a notion of being associated with a tab), so it will work.

Yes it is associated with a tab and do need to prompt UI.

If they just get continued (via sending a NULL cert), and you're using the URLRequestContext, then we're entering bad sockets into the pool.

 

My original intent was just to cancel the request if a client certificate is requested, as is done for prerendering.

I do want to echo again that it doesn't seem like this is going down the right design path. I realize this is meant for a data gathering experiment, but there's definitely concerns about how to rationalize this behaviour and how it works with the overall networking layer.

The very notion of "Stale while Revalidate" is problematic if we don't have a good answer for dealing with background requests. This is very much an issue that Service Workers face as well, and by failing to address these, we leave a very non-deterministic behaviour for devs & operators.

This is very similar to some of the issues going on with XHR, with websockets, with fetch, and with service workers (e.g. https://code.google.com/p/chromium/issues/detail?id=329884#c26 ), and my fear is if we keep punting for minimum viable products, we're going to find it's a stop-ship bug at some later point (I've seen this happen several times now, and that's why I'm concerned)

--
You received this message because you are subscribed to the Google Groups "net-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to net-dev+u...@chromium.org.
To post to this group, send email to net...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/net-dev/CAHixhFpK7gX_2pw_CiPc2PACoF6sUSmLQNATZRwo%2BDe-jXVk6g%40mail.gmail.com.

[Adam Rice]

On 23 June 2015 at 03:17, David Benjamin <davi...@chromium.org> wrote:

//net knows when its own objects are destroyed because //net has the destructor. In the previous HttpCache version, who owned the async revalidations? Were they just fired off somewhere or owned by an object?

They were owned by HttpCache in the obvious way. You can read the original CL at https://codereview.chromium.org/455623003/ or sync back to before the revert b38b3f9fbe286d7305d89d5089d18a423d9ab2bc if you want to try it out (there is an enable-stale-while-revalidate flag in about:flags).

But if it was using the HttpTransaction layer (as I think it should, since you don't want to participate in redirects or NetworkDelegate), I don't see a problem. The net/http layer isn't even aware of URLRequestContext at all, just the lower-level objects hanging off of it.

You have to participate in NetworkDelegate to implement cookies. See  URLRequest::CanSetCookie().

If there's any point at which, say, HttpCache exists, but the pointers in its HttpNetworkSession are invalid, that's a bug and we should fix it. Just as URLRequest may freely assume its URLRequestContext outlives it, HttpCache should be able to freely assume all of its unowned parameters outlive it.

I agree with you in principle. However, I have dealt with so many issues with URLRequestContext lifetime that I have no faith that it can ever be fixed. Anyone can create a URLRequestContext and change the lifetime semantics of the members however they like. Even the implementations in //net appear to be wrong. Example: url_request_context_storage.h:82

  // The ChannelIDService must outlive the HttpTransactionFactory.

  scoped_ptr<ChannelIDService> channel_id_service_;

  scoped_ptr<FraudulentCertificateReporter> fraudulent_certificate_reporter_;

  scoped_ptr<HttpAuthHandlerFactory> http_auth_handler_factory_;

  scoped_ptr<ProxyService> proxy_service_;

  // TODO(willchan): Remove refcounting on these members.

  scoped_refptr<SSLConfigService> ssl_config_service_;

  scoped_ptr<NetworkDelegate> network_delegate_;

  scoped_ptr<HttpServerProperties> http_server_properties_;

  scoped_ptr<HttpUserAgentSettings> http_user_agent_settings_;

  scoped_refptr<CookieStore> cookie_store_;

  scoped_ptr<TransportSecurityState> transport_security_state_;

  scoped_ptr<HttpTransactionFactory> http_transaction_factory_;

Either the comment is wrong, or the code is wrong. It's been that way since 2012. 

Thank you for appreciating the complexity of the issues. There are a lot of people asking why it hasn't shipped already.

 

Do you have a sample stack trace? Or was it just an overall crash rate increase without any of the traces being helpful?

The crashes were all in unrelated code, typical of heap corruption. Even the crashes that were unique to the experimental group were in unrelated code. 

[Matt Menke]

On Tue, Jun 23, 2015 at 3:33 PM, Adam Rice <ri...@google.com> wrote:

On 23 June 2015 at 03:17, David Benjamin <davi...@chromium.org> wrote:

//net knows when its own objects are destroyed because //net has the destructor. In the previous HttpCache version, who owned the async revalidations? Were they just fired off somewhere or owned by an object?

They were owned by HttpCache in the obvious way. You can read the original CL at https://codereview.chromium.org/455623003/ or sync back to before the revert b38b3f9fbe286d7305d89d5089d18a423d9ab2bc if you want to try it out (there is an enable-stale-while-revalidate flag in about:flags).

But if it was using the HttpTransaction layer (as I think it should, since you don't want to participate in redirects or NetworkDelegate), I don't see a problem. The net/http layer isn't even aware of URLRequestContext at all, just the lower-level objects hanging off of it.

You have to participate in NetworkDelegate to implement cookies. See  URLRequest::CanSetCookie().

If there's any point at which, say, HttpCache exists, but the pointers in its HttpNetworkSession are invalid, that's a bug and we should fix it. Just as URLRequest may freely assume its URLRequestContext outlives it, HttpCache should be able to freely assume all of its unowned parameters outlive it.

I agree with you in principle. However, I have dealt with so many issues with URLRequestContext lifetime that I have no faith that it can ever be fixed. Anyone can create a URLRequestContext and change the lifetime semantics of the members however they like. Even the implementations in //net appear to be wrong. Example: url_request_context_storage.h:82

  // The ChannelIDService must outlive the HttpTransactionFactory.

  scoped_ptr<ChannelIDService> channel_id_service_;

  scoped_ptr<FraudulentCertificateReporter> fraudulent_certificate_reporter_;

  scoped_ptr<HttpAuthHandlerFactory> http_auth_handler_factory_;

  scoped_ptr<ProxyService> proxy_service_;

  // TODO(willchan): Remove refcounting on these members.

  scoped_refptr<SSLConfigService> ssl_config_service_;

  scoped_ptr<NetworkDelegate> network_delegate_;

  scoped_ptr<HttpServerProperties> http_server_properties_;

  scoped_ptr<HttpUserAgentSettings> http_user_agent_settings_;

  scoped_refptr<CookieStore> cookie_store_;

  scoped_ptr<TransportSecurityState> transport_security_state_;

  scoped_ptr<HttpTransactionFactory> http_transaction_factory_;

Either the comment is wrong, or the code is wrong. It's been that way since 2012. 

What's wrong with that?  The channel id service is outliving the HttpTransaction factory there.

[Matt Menke]

On Tue, Jun 23, 2015 at 3:38 PM, Matt Menke <mme...@chromium.org> wrote:

On Tue, Jun 23, 2015 at 3:33 PM, Adam Rice <ri...@google.com> wrote:

On 23 June 2015 at 03:17, David Benjamin <davi...@chromium.org> wrote:

//net knows when its own objects are destroyed because //net has the destructor. In the previous HttpCache version, who owned the async revalidations? Were they just fired off somewhere or owned by an object?

They were owned by HttpCache in the obvious way. You can read the original CL at https://codereview.chromium.org/455623003/ or sync back to before the revert b38b3f9fbe286d7305d89d5089d18a423d9ab2bc if you want to try it out (there is an enable-stale-while-revalidate flag in about:flags).

But if it was using the HttpTransaction layer (as I think it should, since you don't want to participate in redirects or NetworkDelegate), I don't see a problem. The net/http layer isn't even aware of URLRequestContext at all, just the lower-level objects hanging off of it.

You have to participate in NetworkDelegate to implement cookies. See  URLRequest::CanSetCookie().

If there's any point at which, say, HttpCache exists, but the pointers in its HttpNetworkSession are invalid, that's a bug and we should fix it. Just as URLRequest may freely assume its URLRequestContext outlives it, HttpCache should be able to freely assume all of its unowned parameters outlive it.

I agree with you in principle. However, I have dealt with so many issues with URLRequestContext lifetime that I have no faith that it can ever be fixed. Anyone can create a URLRequestContext and change the lifetime semantics of the members however they like. Even the implementations in //net appear to be wrong. Example: url_request_context_storage.h:82

  // The ChannelIDService must outlive the HttpTransactionFactory.

  scoped_ptr<ChannelIDService> channel_id_service_;

  scoped_ptr<FraudulentCertificateReporter> fraudulent_certificate_reporter_;

  scoped_ptr<HttpAuthHandlerFactory> http_auth_handler_factory_;

  scoped_ptr<ProxyService> proxy_service_;

  // TODO(willchan): Remove refcounting on these members.

  scoped_refptr<SSLConfigService> ssl_config_service_;

  scoped_ptr<NetworkDelegate> network_delegate_;

  scoped_ptr<HttpServerProperties> http_server_properties_;

  scoped_ptr<HttpUserAgentSettings> http_user_agent_settings_;

  scoped_refptr<CookieStore> cookie_store_;

  scoped_ptr<TransportSecurityState> transport_security_state_;

  scoped_ptr<HttpTransactionFactory> http_transaction_factory_;

Either the comment is wrong, or the code is wrong. It's been that way since 2012. 

What's wrong with that?  The channel id service is outliving the HttpTransaction factory there.

Oh, and I agree that request context lifetime semantics are bad and broken.  Sharing objects between contexts has lead to a huge number of lifetime issues, and then there's the issue of URLRequest lifetimes as well...

[Adam Rice]

On 24 June 2015 at 04:38, Matt Menke <mme...@chromium.org> wrote:

  // The ChannelIDService must outlive the HttpTransactionFactory.

  scoped_ptr<ChannelIDService> channel_id_service_;

  scoped_ptr<FraudulentCertificateReporter> fraudulent_certificate_reporter_;

  scoped_ptr<HttpAuthHandlerFactory> http_auth_handler_factory_;

  scoped_ptr<ProxyService> proxy_service_;

  // TODO(willchan): Remove refcounting on these members.

  scoped_refptr<SSLConfigService> ssl_config_service_;

  scoped_ptr<NetworkDelegate> network_delegate_;

  scoped_ptr<HttpServerProperties> http_server_properties_;

  scoped_ptr<HttpUserAgentSettings> http_user_agent_settings_;

  scoped_refptr<CookieStore> cookie_store_;

  scoped_ptr<TransportSecurityState> transport_security_state_;

  scoped_ptr<HttpTransactionFactory> http_transaction_factory_;

Either the comment is wrong, or the code is wrong. It's been that way since 2012. 

What's wrong with that?  The channel id service is outliving the HttpTransaction factory there.

I'm sorry. How embarrassing. I'm just going to go crawl in a hole for a while.