Instantiate OriginBoundCertService in relevant places and do plumbing to pass it down to HttpNetw...

Instantiate OriginBoundCertService in relevant places and do plumbing to pass it down to HttpNetworkSession.

BUG=88782
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96296

[rkn, 2011-07-29 20:43:30]

Please review.

[wtc, 2011-08-01 18:21:24]

Thanks for the CL.

The code in off_the_record_profile_io_data.cc and
profile_impl_io_data.cc needs more work and needs careful
review.  Please do your best to understand the profile I/O
data code, and use "svn blame" to find the original author
of that code to be a reviewer.  I guess it's mirandac.
We should also ask willchan to review this CL.

You can do another round of code review with me before
asking additional reviewers.

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:260: // In-memory origin bound cert service.
It may be more accurate to say "cert store" here and on line
286 below, because only the cert store has the in-memory vs.
persistent distinction.

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:23: #include
"net/base/origin_bound_cert_service.h"
Can you find out why we need to include the origin-bound
cert headers, but not the cookie store headers?

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:171:
main_context->set_origin_bound_cert_service(
Add a comment to say:
  // For incognito, we use a non-persistent origin bound cert store.  See lines
168-169 above.

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:173: new
net::DefaultOriginBoundCertStore(NULL)));
This code should be moved before line 166, because the
comments on lines 168-169 seem to describe the code that sets
up cookie stores for incognito and extensions (lines 166-181),
so we should not add code in the middle of that.

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:187:
extensions_origin_bound_cert_service);
IMPORTANT: the extensions_origin_bound_cert_service code
should be reviewed carefully.  You need to be sure that the
extensions need origin-bound certs.

Note that the extensions_cookie_store code on lines 175-181
above seems to be restricted to only handle the
"chrome dev tools" and "extensions" URL schemes.

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:241: // BEGIN(rkn)
Remove the "BEGIN(rkn)" and "END(rkn)" comments.

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:262:
net::OriginBoundCertService* extensions_origin_bound_cert_service =
Store this in a scoped_refptr.  As a general rule, pointers
to ref-counted objects should always be stored in
scoped_refptrs to avoid reference leaks.

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:273: // END(rkn)
Is it OK to put this code next to the code that sets up the
cookie store, so that we can compare them easily?

I think some of the origin-bound cert code may need to
interleave with the cookie store code on lines
303-310.

http://codereview.chromium.org/7493025/diff/24003/chrome/common/chrome_consta...
File chrome/common/chrome_constants.h (right):

http://codereview.chromium.org/7493025/diff/24003/chrome/common/chrome_consta...
chrome/common/chrome_constants.h:55: extern const FilePath::CharType
kExtensionsOBCertFilename[];
In the code review of another CL, I asked you to remove
kExtensionsOBCertFilename unless we really need it.

Did you add kExtensionsOBCertFilename back intentionally,
or was this a merging error?

[rkn, 2011-08-01 19:32:52]

Thanks for the comments. I will study the code more and upload changes later.

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:23: #include
"net/base/origin_bound_cert_service.h"
On 2011/08/01 18:21:24, wtc wrote:
> Can you find out why we need to include the origin-bound
> cert headers, but not the cookie store headers?

I believe this is the case because off_the_record_profile_io_data.h includes
profile_io_data.h which includes cookie_monster.h.

http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:187:
extensions_origin_bound_cert_service);
On 2011/08/01 18:21:24, wtc wrote:
> IMPORTANT: the extensions_origin_bound_cert_service code
> should be reviewed carefully.  You need to be sure that the
> extensions need origin-bound certs.
> 
> Note that the extensions_cookie_store code on lines 175-181
> above seems to be restricted to only handle the
> "chrome dev tools" and "extensions" URL schemes.

I'm removing origin-bound certs for extensions until I can figure out if it is
necessary or not.

[rkn, 2011-08-01 21:06:14]

On 2011/08/01 19:32:52, rkn wrote:
> Thanks for the comments. I will study the code more and upload changes later.
> 
>
http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/off...
> File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):
> 
>
http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/off...
> chrome/browser/profiles/off_the_record_profile_io_data.cc:23: #include
> "net/base/origin_bound_cert_service.h"
> On 2011/08/01 18:21:24, wtc wrote:
> > Can you find out why we need to include the origin-bound
> > cert headers, but not the cookie store headers?
> 
> I believe this is the case because off_the_record_profile_io_data.h includes
> profile_io_data.h which includes cookie_monster.h.
> 
>
http://codereview.chromium.org/7493025/diff/24003/chrome/browser/profiles/off...
> chrome/browser/profiles/off_the_record_profile_io_data.cc:187:
> extensions_origin_bound_cert_service);
> On 2011/08/01 18:21:24, wtc wrote:
> > IMPORTANT: the extensions_origin_bound_cert_service code
> > should be reviewed carefully.  You need to be sure that the
> > extensions need origin-bound certs.
> > 
> > Note that the extensions_cookie_store code on lines 175-181
> > above seems to be restricted to only handle the
> > "chrome dev tools" and "extensions" URL schemes.
> 
> I'm removing origin-bound certs for extensions until I can figure out if it is
> necessary or not.

I'm uploading some more versions so I can view the diffs here. It is not ready
for review.

[rkn, 2011-08-03 00:39:35]

willchan: Please review.

mirandac: Please review files under "chrome/". For the purpose of the review,
OriginBoundCertService is kind of similar to CookieStore (they are created and
destroyed together, and there should be a separate one for incognito mode). To
give some background, OriginBoundCertService generates OriginBoundCerts, which
allow you to prove cookie ownership. This is only used over SSL/TLS.

By the way, it appears that URLRequestContext may not need to contain an
OriginBoundCertService. At least in the code here, all URLRequestContext does is
pass its OriginBoundCertService to HttpCache. However, I did it this way for
consistency.

http://codereview.chromium.org/7493025/diff/36023/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (left):

http://codereview.chromium.org/7493025/diff/36023/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:232: 
I moved the below block of code farther down because it depends on
origin_bound_cert_service.

[wtc, 2011-08-04 00:56:20]

LGTM.

Please wait until Friday for willchan or mirandac's review.

http://codereview.chromium.org/7493025/diff/36023/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7493025/diff/36023/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:260: // In-memory origin bound cert-store.
Nit: origin bound cert-store => origin-bound cert store

Make the same change to line 286 below.

http://codereview.chromium.org/7493025/diff/36023/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/7493025/diff/36023/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:238:
net::OriginBoundCertService* origin_bound_cert_service = NULL;
IMPORTANT: we should use a scoped_refptr or a scoped_ptr
here, depending on whether OriginBoundCertService is a
reference-counted type or not.

http://codereview.chromium.org/7493025/diff/36023/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:272: }
I suggest moving this block of code to line 287 below,
so that we don't separate the code that creates cookie_store
from the code that creates extensions_cookie_store.

http://codereview.chromium.org/7493025/diff/36023/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:289: origin_bound_cert_service);
NOTE: this code implies that OriginBoundCertService needs to
be reference-counted, because both main_context and
media_request_context "own" it.

[willchan no longer on Chromium, 2011-08-05 04:01:18]

http://codereview.chromium.org/7493025/diff/36023/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/7493025/diff/36023/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:289: origin_bound_cert_service);
On 2011/08/04 00:56:20, wtc wrote:
> NOTE: this code implies that OriginBoundCertService needs to
> be reference-counted, because both main_context and
> media_request_context "own" it.

We're generally moving toward the ProfileIOData owning the data rather than the
URLRequestContext refcounting it. Can you do that here?

[rkn, 2011-08-05 21:35:11]

> We're generally moving toward the ProfileIOData owning the data rather than
the
> URLRequestContext refcounting it. Can you do that here?

In principle, this seems like the right thing to do. I have two comments about
it.

1. The OriginBoundCertService objects constructed in
ConstructProxyScriptFetcherContext and ConstructSystemRequestContext are not
associated with a profile. This is ok because we could make them owned by
IOThread::Globals.

2. ProfileIOData doesn't own its URLRequestContexts, it has scoped_refptrs to
them. From gdb, it appears that the URLRequestContexts are destroyed some time
after ProfileIOData, which means that we would risk deleting our
OriginBoundCertService objects and retaining pointers to them. This part is more
worrisome to me.

[willchan no longer on Chromium, 2011-08-05 22:51:34]

On Sat, Aug 6, 2011 at 12:35 AM,  <rkn@chromium.org> wrote:
>> We're generally moving toward the ProfileIOData owning the data rather
>> than
>
> the
>>
>> URLRequestContext refcounting it. Can you do that here?
>
> In principle, this seems like the right thing to do. I have two comments
> about
> it.
>
> 1. The OriginBoundCertService objects constructed in
> ConstructProxyScriptFetcherContext and ConstructSystemRequestContext are not
> associated with a profile. This is ok because we could make them owned by
> IOThread::Globals.

Correct

>
> 2. ProfileIOData doesn't own its URLRequestContexts, it has scoped_refptrs
> to
> them. From gdb, it appears that the URLRequestContexts are destroyed some
> time
> after ProfileIOData, which means that we would risk deleting our
> OriginBoundCertService objects and retaining pointers to them. This part is
> more
> worrisome to me.

The plan is to make ProfileIOData own its URLRequestContexts and stop
refcounting them. Yes, it's possible for URLRequestContexts to be
destroyed after ProfileIOData, although it should not be used after
ProfileIOData goes away (otherwise it's a use-after-free bug). If you
know of any such situations, please file bugs for them and assign them
to me to triage.

>
> http://codereview.chromium.org/7493025/
>

[rkn, 2011-08-06 00:03:25]

I transferred ownership of the OriginBoundCertService to ProfileIOData. Please
review changes.

http://codereview.chromium.org/7493025/diff/53005/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7493025/diff/53005/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:513:
globals_->proxy_origin_bound_cert_service.get();
I use the proxy_origin_bound_cert_service because session_params is passed to
network_session, which is passed to
proxy_script_fetcher_http_transaction_factory.

http://codereview.chromium.org/7493025/diff/53005/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7493025/diff/53005/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:213: }
This breaks consistency because none of the others has a setter.

[willchan no longer on Chromium, 2011-08-06 14:20:49]

Dropping Miranda, she doesn't know ProfileIOData as well.

http://codereview.chromium.org/7493025/diff/53005/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7493025/diff/53005/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:213: }
On 2011/08/06 00:03:25, rkn wrote:
> This breaks consistency because none of the others has a setter.

This is fine, although I'm not sure you need the accessor. If the subclass is
constructing and setting it, then it already has the pointer. The accessors are
necessary so the subclasses can set the member into the contexts, since the
other members are constructed by the base class.

[rkn, 2011-08-07 22:51:13]

http://codereview.chromium.org/7493025/diff/53005/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7493025/diff/53005/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:213: }
On 2011/08/06 14:20:49, willchan wrote:
> On 2011/08/06 00:03:25, rkn wrote:
> > This breaks consistency because none of the others has a setter.
> 
> This is fine, although I'm not sure you need the accessor. If the subclass is
> constructing and setting it, then it already has the pointer. The accessors
are
> necessary so the subclasses can set the member into the contexts, since the
> other members are constructed by the base class.

Done.

[wtc, 2011-08-08 22:49:20]

Review comments on Patch Set 5.

rkn: I suggest some related cleanups for IOThread, to reduce
wasted objects.  You may choose to make those changes in a
separate CL to prevent this CL from dragging on too long.

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:259: context->set_cookie_store(new
net::CookieMonster(NULL, NULL));
Similarly, I think the proxy script fetcher context and system
context can share the same in-memory cookie store.

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:501: new net::DefaultOriginBoundCertStore(NULL)));
We should use globals_->system_origin_bound_cert_service in
system_params in IOThread::InitSystemRequestContextOnIOThread().

I suspect proxy_script_fetcher and system can share the
same origin-bound cert service.  Please find out exactly
what the proxy script fetcher context is used for.  I
seem to recall proxy script fetcher only needs its own
proxy service to avoid a recursion.

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:526: new
net::FtpNetworkLayer(globals_->host_resolver.get()));
I think the proxy script fetcher and system contexts can share
the same ftp transaction factory, too.

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:529: ConstructProxyScriptFetcherContext(globals_,
net_log_);
Remove this proxy_script_fetcher_context local variable.  It
is not useful.  See lines 751-752 at the end of this file,
where a similar piece of code does without the scoped_refptr.

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:74: scoped_ptr<net::OriginBoundCertService>
system_origin_bound_cert_service;
These two members should be put next to the related members.

proxy_script_fetcher_xxx_: lines 78-82, and an odd one on line 90
system_xxx_: lines 91-94

But I believe we can use the same origin-bound cert service
object for the proxy script fetcher context and the system
context.  I believe the proxy script fetcher context needs
its own proxy service and anything that depends on a proxy
service; everything else it can share with the system
context.  Please ask eroman or willchan to confirm this.

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:169: new
net::DefaultOriginBoundCertStore(NULL)));
BUG: I believe this origin-bound cert service (for incognito)
is leaked because it is not owned by anything.

I think the fix is to let OffTheRecordProfileIOData own this
origin-bound cert service.

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:238:
net::OriginBoundCertService* origin_bound_cert_service;
BUG: initialize this to NULL.

But, I believe we don't need the local variable origin_bound_cert_service
any more.  We should just use the origin_bound_cert_service_
member.

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:21: #include
"net/base/origin_bound_cert_service.h"
I think a forward declaration of OriginBoundCertService should
be enough.

[rkn, 2011-08-09 00:43:50]

I made the suggested changes.

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/7493025/diff/50011/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:238:
net::OriginBoundCertService* origin_bound_cert_service;
On 2011/08/08 22:49:20, wtc wrote:
> BUG: initialize this to NULL.
> 
> But, I believe we don't need the local variable origin_bound_cert_service
> any more.  We should just use the origin_bound_cert_service_
> member.

The reason for the local variable is that the origin_bound_cert_service_ member
is private to ProfileIOData.

[willchan no longer on Chromium, 2011-08-09 17:03:47]

LGTM

On 2011/08/09 00:43:50, rkn wrote:
> I made the suggested changes.
> 
>
http://codereview.chromium.org/7493025/diff/50011/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile_impl_io_data.cc (right):
> 
>
http://codereview.chromium.org/7493025/diff/50011/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_impl_io_data.cc:238:
> net::OriginBoundCertService* origin_bound_cert_service;
> On 2011/08/08 22:49:20, wtc wrote:
> > BUG: initialize this to NULL.
> > 
> > But, I believe we don't need the local variable origin_bound_cert_service
> > any more.  We should just use the origin_bound_cert_service_
> > member.
> 
> The reason for the local variable is that the origin_bound_cert_service_
member
> is private to ProfileIOData.

[wtc, 2011-08-09 17:43:45]

LGTM.  I suggest some changes below.

http://codereview.chromium.org/7493025/diff/55013/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/7493025/diff/55013/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:260: // In-memory origin-bound cert store.
The "In-memory cookie store" and "In-memory origin-bound cert store"
comments should now be removed from ConstructProxyScriptFetcherContext
and ConstructSystemRequestContext.

These comments should be moved to lines 503-506, where
we create the in-memory cookie store and origin-bound
cert store.

http://codereview.chromium.org/7493025/diff/55013/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/7493025/diff/55013/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:83: scoped_ptr<net::OriginBoundCertService>
origin_bound_cert_service;
These two members should be given a system_ prefix, and they
should be listed along with the other system_xxx members near
the end of this struct.

I believe the members that don't have the proxy_script_fetcher_
or system_ prefix are shared by all profiles.

http://codereview.chromium.org/7493025/diff/55013/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/7493025/diff/55013/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:246:
set_origin_bound_cert_service(origin_bound_cert_service);
I recommend moving this set_origin_bound_cert_service call
to line 288, and removing the set_origin_bound_cert_service
call on line 285.

http://codereview.chromium.org/7493025/diff/55013/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7493025/diff/55013/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:211: void
set_origin_bound_cert_service(
It is important to point out that this is not a typical setter.

Please document that OriginBoundCertService is created by
a derived class of ProfileIOData and the derived class calls
this method to set the origin_bound_cert_service_ member
and transfers ownership to the base class.

An alternative design is to make origin_bound_cert_service_
a protected member.  But I am not sure if that's a good idea.

Question: does the forward declaration of OriginBoundCertService
prevent us from defining this method inline in the header?

[rkn, 2011-08-09 20:32:05]

wtc, willchan: I uploaded three more files, which I had overlooked. Can one of
you take a look at patch set 7?

http://codereview.chromium.org/7493025/diff/55013/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7493025/diff/55013/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:211: void
set_origin_bound_cert_service(
On 2011/08/09 17:43:45, wtc wrote:
> It is important to point out that this is not a typical setter.
> 
> Please document that OriginBoundCertService is created by
> a derived class of ProfileIOData and the derived class calls
> this method to set the origin_bound_cert_service_ member
> and transfers ownership to the base class.
> 
> An alternative design is to make origin_bound_cert_service_
> a protected member.  But I am not sure if that's a good idea.
> 
> Question: does the forward declaration of OriginBoundCertService
> prevent us from defining this method inline in the header?

Yes, the forward declaration seems to prevent us from inlining the method.

[rkn, 2011-08-09 20:32:34]

On 2011/08/09 20:32:05, rkn wrote:
> wtc, willchan: I uploaded three more files, which I had overlooked. Can one of
> you take a look at patch set 7?

I mean patch set 8.

[wtc, 2011-08-09 20:56:32]

LGTM on Patch Set 8.

[willchan no longer on Chromium, 2011-08-09 21:00:52]

On 2011/08/09 20:56:32, wtc wrote:
> LGTM on Patch Set 8.

Still LGTM

[commit-bot: I haz the power, 2011-08-11 01:49:15]

Change committed as 96296