Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
999243002:
Chromecast buildfix: ClientCertificateDelegate changes. (Closed)
DescriptionChromecast buildfix: ClientCertificateDelegate changes.
See: https://codereview.chromium.org/859213006/
R=davidben@chromium.org,lcwu@chromium.org
BUG=None
Committed: https://crrev.com/c645303c79f837036c90f732de8be5538c0f57b8
Cr-Commit-Position: refs/heads/master@{#320201}
Patch Set 1 #
Total comments: 4
Patch Set 2 : base::Owned(delegate.release()) #
Total comments: 5
Patch Set 3 : explicitly continue with no cert on invalid url #Messages
Total messages: 16 (3 generated)
gunsch@chromium.org changed reviewers: + lcwu@chromium.org - dougsteed@chromium.org
Mrrf, sorry about that. Is this built on any try bot configuration in Chromium? https://codereview.chromium.org/999243002/diff/1/chromecast/browser/cast_cont... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/999243002/diff/1/chromecast/browser/cast_cont... chromecast/browser/cast_content_browser_client.cc:237: delegate->ContinueWithCertificate(CastNetworkDelegate::DeviceCert()); This won't work. It has to be called on the IO thread. What if you keep this function as-is and then switch the delegate.Pass() to a base::Owned(delegate.release()) in line 225?
https://codereview.chromium.org/999243002/diff/1/chromecast/browser/cast_cont... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/999243002/diff/1/chromecast/browser/cast_cont... chromecast/browser/cast_content_browser_client.cc:237: delegate->ContinueWithCertificate(CastNetworkDelegate::DeviceCert()); On 2015/03/11 22:57:59, David Benjamin wrote: > This won't work. It has to be called on the IO thread. Sorry, I meant UI thread.
We do have a fairly new trybot (git cl try -m tryserver.chromium.linux -b cast_shell) and are going through the motions to promote it. Right now it's running at 10% or manual triggering. https://codereview.chromium.org/999243002/diff/1/chromecast/browser/cast_cont... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/999243002/diff/1/chromecast/browser/cast_cont... chromecast/browser/cast_content_browser_client.cc:237: delegate->ContinueWithCertificate(CastNetworkDelegate::DeviceCert()); On 2015/03/11 22:59:28, David Benjamin wrote: > On 2015/03/11 22:57:59, David Benjamin wrote: > > This won't work. It has to be called on the IO thread. > > Sorry, I meant UI thread. I can use base::Owned, but it's a little ugly (release the scoped_ptr ref and continue). Alternately, I could base::Passed the scoped_ptr, but then I'd need yet another helper function to make the call, so maybe that's better.
lgtm https://codereview.chromium.org/999243002/diff/1/chromecast/browser/cast_cont... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/999243002/diff/1/chromecast/browser/cast_cont... chromecast/browser/cast_content_browser_client.cc:237: delegate->ContinueWithCertificate(CastNetworkDelegate::DeviceCert()); On 2015/03/11 23:05:59, gunsch wrote: > > On 2015/03/11 22:59:28, David Benjamin wrote: > > On 2015/03/11 22:57:59, David Benjamin wrote: > > > This won't work. It has to be called on the IO thread. > > > > Sorry, I meant UI thread. > > I can use base::Owned, but it's a little ugly (release the scoped_ptr ref and > continue). Alternately, I could base::Passed the scoped_ptr, but then I'd need > yet another helper function to make the call, so maybe that's better. Yeah, the release() bugged me in the other place I ended up having to do this. Dunno if we have a better way to a scoped_ptr into the this slot. (Does base::Owned(blah.Pass()) work?) https://codereview.chromium.org/999243002/diff/20001/chromecast/browser/cast_... File chromecast/browser/cast_content_browser_client.cc (left): https://codereview.chromium.org/999243002/diff/20001/chromecast/browser/cast_... chromecast/browser/cast_content_browser_client.cc:206: delegate->SelectClientCertificate(nullptr); NOTE: This is a behavior change from before my CL. Although it's probably what you want? Selecting null is to affirmatively continue without a certificate. Letting the dtor run is to abort the request. Before, only the former was possible. I intentionally left the behavior as-is because I wasn't sure what this code was supposed to do[*]. (Though it should never get to this point anyway.) [*] I also still need to meet with you folks about it. It's unlikely to do what you want because client auth decisions in one request apply from then on into the entire BrowsingContext.
https://codereview.chromium.org/999243002/diff/20001/chromecast/browser/cast_... File chromecast/browser/cast_content_browser_client.cc (left): https://codereview.chromium.org/999243002/diff/20001/chromecast/browser/cast_... chromecast/browser/cast_content_browser_client.cc:206: delegate->SelectClientCertificate(nullptr); On 2015/03/11 23:26:38, David Benjamin wrote: > [*] I also still need to meet with you folks about it. It's unlikely to do what > you want because client auth decisions in one request apply from then on into > the entire BrowsingContext. Er, to clarify, that's not a result of my change or anything. This has been true forever. If one request selects a certificate, it applies to all requests in the BrowserContext from then on. If one requests selects no certificate (rather than abort), it *also* applies to all requests in the BrowserContext from then on.
https://codereview.chromium.org/999243002/diff/20001/chromecast/browser/cast_... File chromecast/browser/cast_content_browser_client.cc (left): https://codereview.chromium.org/999243002/diff/20001/chromecast/browser/cast_... chromecast/browser/cast_content_browser_client.cc:206: delegate->SelectClientCertificate(nullptr); Hmm, is that really the case? Before your CL, this read "callback.run(NULL)", and the other cases I see seem to have changed to just let ClientCertificateDelegate get destroyed. E.g. (from the diff): https://codereview.chromium.org/download/issue859213006_720001.diff ContentBrowserClient::SelectClientCertificate and AwContentBrowserClient::SelectClientCertificate Were those locations a behavior change in your CL, or the new code path for maintaining the same behavior?
https://codereview.chromium.org/999243002/diff/20001/chromecast/browser/cast_... File chromecast/browser/cast_content_browser_client.cc (left): https://codereview.chromium.org/999243002/diff/20001/chromecast/browser/cast_... chromecast/browser/cast_content_browser_client.cc:206: delegate->SelectClientCertificate(nullptr); On 2015/03/11 23:32:13, gunsch wrote: > Hmm, is that really the case? Before your CL, this read "callback.run(NULL)", > and the other cases I see seem to have changed to just let > ClientCertificateDelegate get destroyed. E.g. (from the diff): callback.run(NULL) is equivalent to SelectClientCertificate(NULL). There was no way to abort a client certificate request at all before. (That was the point of the CL.) > https://codereview.chromium.org/download/issue859213006_720001.diff > > ContentBrowserClient::SelectClientCertificate > and > AwContentBrowserClient::SelectClientCertificate > > Were those locations a behavior change in your CL, or the new code path for > maintaining the same behavior? Those were a behavior change. (A reviewer asked me to sort those out there. I probably should have done it in a follow-up. Oh well. Those places were less unsound, so I was more comfortable changing them.)
https://codereview.chromium.org/999243002/diff/20001/chromecast/browser/cast_... File chromecast/browser/cast_content_browser_client.cc (left): https://codereview.chromium.org/999243002/diff/20001/chromecast/browser/cast_... chromecast/browser/cast_content_browser_client.cc:206: delegate->SelectClientCertificate(nullptr); On 2015/03/11 23:35:23, David Benjamin wrote: > On 2015/03/11 23:32:13, gunsch wrote: > > Hmm, is that really the case? Before your CL, this read "callback.run(NULL)", > > and the other cases I see seem to have changed to just let > > ClientCertificateDelegate get destroyed. E.g. (from the diff): > > callback.run(NULL) is equivalent to SelectClientCertificate(NULL). There was no > way to abort a client certificate request at all before. (That was the point of > the CL.) > > > https://codereview.chromium.org/download/issue859213006_720001.diff > > > > ContentBrowserClient::SelectClientCertificate > > and > > AwContentBrowserClient::SelectClientCertificate > > > > Were those locations a behavior change in your CL, or the new code path for > > maintaining the same behavior? > > Those were a behavior change. (A reviewer asked me to sort those out there. I > probably should have done it in a follow-up. Oh well. Those places were less > unsound, so I was more comfortable changing them.) Got it. I'll go with ContinueWithCertificate(nullptr) for now and talk more with Doug about the intended behavior. Thanks for the information.
The CQ bit was checked by gunsch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/999243002/#ps40001 (title: "explicitly continue with no cert on invalid url")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/999243002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c645303c79f837036c90f732de8be5538c0f57b8 Cr-Commit-Position: refs/heads/master@{#320201} |
