Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1392143003:
Allways call didReceiveAuthenticationChallenge: completion handler. (Closed)
|
Created:
5 years, 2 months ago by Eugene But (OOO till 7-30) Modified:
5 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAlways call didReceiveAuthenticationChallenge: completion handler.
iOS throws an exception (which leads to a crash) if didReceiveAuthenticationChallenge:
completion handler is deallocated but never called. Crash was happening
because CertVerifierBlockAdapter did not call its completion handler
if CertVerifierAdapter was destroyed during cert verification (may
happen when Tab is closed during the load).
Changed CertVerifierBlockAdapter to always call its Verify completion
handler by making net::CertVerifier::Request non-cancelable, so each
verification request will complete.
This change does not extend a lifetime of WKWebView (retaining its completion handler
does not retain WKWebView itself).
BUG=541484
Committed: https://crrev.com/bb8be86f212df820a44993f270414d459da3964c
Cr-Commit-Position: refs/heads/master@{#357014}
Patch Set 1 #Patch Set 2 : Self review #
Total comments: 2
Patch Set 3 : Added a unittest; Updated comments; #
Total comments: 16
Patch Set 4 : Addressed Ryan's review comments #
Messages
Total messages: 19 (6 generated)
eugenebut@chromium.org changed reviewers: + rsleevi@chromium.org, stuartmorgan@chromium.org
Ryan, a side effect of this change is that cert verification requests are not cancelled even if WKWebView is deallocated. Is this a problem?
On 2015/10/09 18:50:54, eugenebut wrote: > even if WKWebView is deallocated I assume you mean WebController?
Could we still cancel all the work on our end, and just immediately call the completion with a specific 'cancelled' error? I'd prefer we not extend the WKWebView lifespan more than necessary.
On 2015/10/12 14:10:57, stuartmorgan wrote: > On 2015/10/09 18:50:54, eugenebut wrote: > > even if WKWebView is deallocated > > I assume you mean WebController? Well... both, including WebController.
Updated CL description per Ryan's comment.
Description was changed from ========== Always call didReceiveAuthenticationChallenge: completion handler. iOS throws an exception if didReceiveAuthenticationChallenge: completion handler is not called, which leads to a crash. It was happening because CertVerifierBlockAdapter does not call its completion handler if Adapted is destroyed during cert verification. Changed CertVerifierBlockAdapter to always call its Verify completion handler by making net::CertVerifier::Request non-cancelable, so each verification request will complete. BUG=541484 ========== to ========== Always call didReceiveAuthenticationChallenge: completion handler. iOS throws an exception if didReceiveAuthenticationChallenge: completion handler is not called [?? when? Presumably this means "when iOS calls the didReceiveAuthenticationChallenge callback, it will throw an exception, thus crashing, if the callback does not call the iOS-supplied completion handler"], which leads to a crash. It [?? nb: avoid pronouns like this, which can be quite ambiguous. "It" is this crash, right?] was happening because CertVerifierBlockAdapter does not call its completion handler if Adapted [?? spelling] is destroyed during cert verification. [?? when can this happen? and where are the unit tests for this in response?] Changed CertVerifierBlockAdapter to always call its Verify completion handler by making net::CertVerifier::Request non-cancelable, so each verification request will complete. [?? nb: This differs even more from the contract of the CertVerifier; it should probably be spelled out in the header for the class about this] BUG=541484 ==========
Sorry for the delays, I continue to be swamped by two different security-incidents and was out last week due to sickness. I've left comments on the comments, and tried to provide better explanation as to things. I tried to annotate with [??] in both the CL description and in the comments below. I think a gating factor to landing this is a test for this behaviour, especially if it can causes crashes. Do you plan to add a test? https://codereview.chromium.org/1392143003/diff/20001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1392143003/diff/20001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:88: // always called as required by WKWebView. // is completed. This means that the cert verification request will // not be cancellable, but it will guaranteed that the // |completion_handler| is always called [?? by what?], as required // by the WKWebView contract [?? why are we talking about WKWebView here? Is that a layering violation of the API contracts?]
Description was changed from ========== Always call didReceiveAuthenticationChallenge: completion handler. iOS throws an exception if didReceiveAuthenticationChallenge: completion handler is not called [?? when? Presumably this means "when iOS calls the didReceiveAuthenticationChallenge callback, it will throw an exception, thus crashing, if the callback does not call the iOS-supplied completion handler"], which leads to a crash. It [?? nb: avoid pronouns like this, which can be quite ambiguous. "It" is this crash, right?] was happening because CertVerifierBlockAdapter does not call its completion handler if Adapted [?? spelling] is destroyed during cert verification. [?? when can this happen? and where are the unit tests for this in response?] Changed CertVerifierBlockAdapter to always call its Verify completion handler by making net::CertVerifier::Request non-cancelable, so each verification request will complete. [?? nb: This differs even more from the contract of the CertVerifier; it should probably be spelled out in the header for the class about this] BUG=541484 ========== to ========== Always call didReceiveAuthenticationChallenge: completion handler. iOS throws an exception (which leads to a crash) if didReceiveAuthenticationChallenge: completion handler is deallocated but never called. Crash was happening because CertVerifierBlockAdapter did not call its completion handler if CertVerifierAdapter was destroyed during cert verification (may happen when Tab is closed during the load). Changed CertVerifierBlockAdapter to always call its Verify completion handler by making net::CertVerifier::Request non-cancelable, so each verification request will complete. BUG=541484 ==========
Added a unit test. Thanks for review, PTAL. https://codereview.chromium.org/1392143003/diff/20001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1392143003/diff/20001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:88: // always called as required by WKWebView. On 2015/10/19 23:34:28, Ryan Sleevi wrote: > // is completed. This means that the cert verification request will > // not be cancellable, but it will guaranteed that the > // |completion_handler| is always called [?? by what?], as required > // by the WKWebView contract [?? why are we talking about WKWebView here? Is > that a layering violation of the API contracts?] Updated comments. Hopefully they make sense now.
LGTM % comment nits. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:19: // to |net::CertVerifier::Request|. This comment needs updating. Also, an explainer on why refcounted. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:29: // Verification request must be alive until verification is completed. I find this comment a bit confusing. You're making a statement about a requirement, but it's unclear where this requirement is coming from or why. Put differently, you're not describing what this member is, but why it is, and that's a bit... odd. // Stores the current verification request. The request must outlive the VerificationContext, so that the verification // request is not cancelled, [because why] https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:88: // always called by this method, which is a part of API contract. This still reads a bit... weird. // Keep the |net::CertVerifier::Request| alive until verification completes. Because |context| is kept alive by // |callback| (through base::BindBlock), this means that the cert verification request cannot be cancelled. // However, it guarantees that |callback| - and thus |completion_handler| - will always be called, which is // a necessary part of the API contract of |CertVerifierBlockAdapter::Verify()|. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter.h (right): https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.h:34: // canceled, and their completion handlers guaranteed to be called. Should this be "completion handlers are guaranteed to be called" It's unclear whether the not is conjunctive with the "and their" or not - hence the added verb to clarify. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.h:72: // object of this class is destroyed. nit: This does not read naturally ("if object of this class is destroyed") // asynchronously. // Note: |completion_handler| is guaranteed to be called, even if the instance |Verify()| was called on is destroyed. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter_unittest.cc:152: // destroyed. comment nit: Missing some articles // Tests that the completion handler passed to |Verify()| is called, even if the adapter is destroyed. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter_unittest.cc:158: // Call |Verify| and destroy adapter. s/destroy adapter/destroy the adapter/ https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter_unittest.cc:166: // Make sure that completion handler is called. s/that completion/that the completion/
Description was changed from ========== Always call didReceiveAuthenticationChallenge: completion handler. iOS throws an exception (which leads to a crash) if didReceiveAuthenticationChallenge: completion handler is deallocated but never called. Crash was happening because CertVerifierBlockAdapter did not call its completion handler if CertVerifierAdapter was destroyed during cert verification (may happen when Tab is closed during the load). Changed CertVerifierBlockAdapter to always call its Verify completion handler by making net::CertVerifier::Request non-cancelable, so each verification request will complete. BUG=541484 ========== to ========== Always call didReceiveAuthenticationChallenge: completion handler. iOS throws an exception (which leads to a crash) if didReceiveAuthenticationChallenge: completion handler is deallocated but never called. Crash was happening because CertVerifierBlockAdapter did not call its completion handler if CertVerifierAdapter was destroyed during cert verification (may happen when Tab is closed during the load). Changed CertVerifierBlockAdapter to always call its Verify completion handler by making net::CertVerifier::Request non-cancelable, so each verification request will complete. This change does not extend a lifetime of WKWebView (retaining its completion handler does not retain WKWebView itself). BUG=541484 ==========
Thanks Ryan! Answering to Stuart's question about WebView lifetime: This CL does not extend a lifetime of WKWebView (retaining its completion handler does not retain WKWebView itself). It is totally fine to call webView completionHandler after its deallocation. Updated CL description. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:19: // to |net::CertVerifier::Request|. On 2015/10/28 18:32:27, Ryan Sleevi wrote: > This comment needs updating. > > Also, an explainer on why refcounted. Done. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:29: // Verification request must be alive until verification is completed. On 2015/10/28 18:32:27, Ryan Sleevi wrote: > I find this comment a bit confusing. You're making a statement about a > requirement, but it's unclear where this requirement is coming from or why. > > Put differently, you're not describing what this member is, but why it is, and > that's a bit... odd. > > // Stores the current verification request. The request must outlive the > VerificationContext, so that the verification > // request is not cancelled, [because why] Done. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:88: // always called by this method, which is a part of API contract. On 2015/10/28 18:32:27, Ryan Sleevi wrote: > This still reads a bit... weird. > > // Keep the |net::CertVerifier::Request| alive until verification completes. > Because |context| is kept alive by > // |callback| (through base::BindBlock), this means that the cert verification > request cannot be cancelled. > // However, it guarantees that |callback| - and thus |completion_handler| - will > always be called, which is > // a necessary part of the API contract of |CertVerifierBlockAdapter::Verify()|. Done. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter.h (right): https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.h:34: // canceled, and their completion handlers guaranteed to be called. On 2015/10/28 18:32:27, Ryan Sleevi wrote: > Should this be "completion handlers are guaranteed to be called" > > It's unclear whether the not is conjunctive with the "and their" or not - hence > the added verb to clarify. Done. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.h:72: // object of this class is destroyed. On 2015/10/28 18:32:27, Ryan Sleevi wrote: > nit: This does not read naturally ("if object of this class is destroyed") > > // asynchronously. > // Note: |completion_handler| is guaranteed to be called, even if the instance > |Verify()| was called on is destroyed. Done. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter_unittest.cc:152: // destroyed. On 2015/10/28 18:32:27, Ryan Sleevi wrote: > comment nit: Missing some articles > > // Tests that the completion handler passed to |Verify()| is called, even if the > adapter is destroyed. Done. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter_unittest.cc:158: // Call |Verify| and destroy adapter. On 2015/10/28 18:32:27, Ryan Sleevi wrote: > s/destroy adapter/destroy the adapter/ Done. https://codereview.chromium.org/1392143003/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter_unittest.cc:166: // Make sure that completion handler is called. On 2015/10/28 18:32:27, Ryan Sleevi wrote: > s/that completion/that the completion/ Done.
lgtm
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1392143003/#ps60001 (title: "Addressed Ryan's review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1392143003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1392143003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bb8be86f212df820a44993f270414d459da3964c Cr-Commit-Position: refs/heads/master@{#357014} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
