New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix random crash on app quit #11125

Merged
merged 1 commit into from Nov 17, 2017

Conversation

Projects
None yet
4 participants
@tarruda
Copy link
Contributor

tarruda commented Nov 15, 2017

Close #10051

@juturu can you confirm if this PR fixes the issue for you?

@tarruda tarruda requested a review from electron/reviewers as a code owner Nov 15, 2017

@tarruda tarruda requested review from zcbenz and deepak1556 Nov 15, 2017

@tarruda tarruda force-pushed the fix-cert-verification-random-crash-on-exit branch from 704550f to 7ddf1ba Nov 15, 2017

@ckerr
Copy link
Member

ckerr left a comment

I have a few inline comments on the implementation, but for the main point -- keeping the delegate alive after the context is destroyed -- I'm not sure I understand the implications well enough and defer to @zcbenz & @deepak1556 there

In the first place, who is still trying to use the delegate after the context is destroyed? Is this a question of the context needing to clean up better in its dtor?

@@ -34,7 +34,7 @@ class AtomCertVerifier : public net::CertVerifier {
void SetVerifyProc(const VerifyProc& proc);

const VerifyProc verify_proc() const { return verify_proc_; }
AtomCTDelegate* ct_delegate() const { return ct_delegate_; }
AtomCTDelegate* ct_delegate() const { return ct_delegate_.get(); }

This comment has been minimized.

@ckerr

ckerr Nov 15, 2017

Member

As a side issue, returning a raw pointer to a managed resource like this worries me a little... but in this case we have a cheap workaround, the only customer of this method is CertVerifierRequest which is already a friend, so we could move ct_delegate() a few lines down here into the protected section of the class

This comment has been minimized.

@tarruda

tarruda Nov 15, 2017

Contributor

👍

@@ -25,7 +25,7 @@ struct VerifyRequestParams {

class AtomCertVerifier : public net::CertVerifier {
public:
explicit AtomCertVerifier(AtomCTDelegate* ct_delegate);
explicit AtomCertVerifier(std::shared_ptr<AtomCTDelegate> ct_delegate);

This comment has been minimized.

@ckerr

ckerr Nov 15, 2017

Member

This should be a const std::shared_ptr<AtomCTDelegate>& here and in the .cc to avoid unnecessary temporaries

This comment has been minimized.

@tarruda

tarruda Nov 15, 2017

Contributor

👍

@tarruda

This comment has been minimized.

Copy link
Contributor

tarruda commented Nov 15, 2017

In the first place, who is still trying to use the delegate after the context is destroyed?

I can't answer that as I'm not very familiar with how chromium code works. In fact, I don't event know what the purpose of AtomBrowserContext is :)

What I can say is that our application was randomly crashing with a similar stack as the one posted in #10051, and that ensuring AtomCTDelegate alive after AtomBrowserContext is destroyed seemed to fix it.

It appears that whatever chromium object that calls AtomBrowserContext::CreateCertVerifier() has a possibility of using it after AtomBrowserContext is deleted, so wrapping AtomCTDelegate in a shared_ptr seemed like an appropriate fix.

@tarruda tarruda force-pushed the fix-cert-verification-random-crash-on-exit branch from 7ddf1ba to 832a950 Nov 15, 2017

@deepak1556

This comment has been minimized.

Copy link
Member

deepak1556 commented Nov 16, 2017

In the first place, who is still trying to use the delegate after the context is destroyed? Is this a question of the context needing to clean up better in its dtor?

Its possible for URLRequestContextGetter to outlive a BrowserContext lifetime and the cert verifier and all other network stuff are owned by it. Hence, the crash. That said, I am not sure why I made AtomBrowserContext the owner of AtomCTDelegate :/ The better fix here is to make brightray::URLRequestContextGetter own the delegate.

@tarruda

This comment has been minimized.

Copy link
Contributor

tarruda commented Nov 16, 2017

The better fix here is to make brightray::URLRequestContextGetter own the delegate.

👍 I will adjust the PR

@tarruda tarruda force-pushed the fix-cert-verification-random-crash-on-exit branch 3 times, most recently from 46e736c to 28345e2 Nov 16, 2017

@tarruda

This comment has been minimized.

Copy link
Contributor

tarruda commented Nov 16, 2017

@deepak1556 moved AtomCTDelegate to brightray (as RequireCTDelegate) and made it owned by brightray::URLRequestContextGetter.

@tarruda tarruda force-pushed the fix-cert-verification-random-crash-on-exit branch from 28345e2 to d2d8ae7 Nov 17, 2017

@deepak1556
Copy link
Member

deepak1556 left a comment

LGTM, just requires a few style changes 👍

@@ -138,6 +139,7 @@ URLRequestContextGetter::URLRequestContextGetter(
in_memory_(in_memory),
io_task_runner_(io_task_runner),
file_task_runner_(file_task_runner),
ct_delegate_(new RequireCTDelegate),

This comment has been minimized.

@deepak1556

deepak1556 Nov 17, 2017

Member

This creates the delegate on the UI thread, we should move the initialization into GetURLRequestContext method below.

This comment has been minimized.

@tarruda

tarruda Nov 17, 2017

Contributor

👍

@@ -280,8 +282,7 @@ net::URLRequestContext* URLRequestContextGetter::GetURLRequestContext() {

std::unique_ptr<net::TransportSecurityState> transport_security_state =
base::WrapUnique(new net::TransportSecurityState);
transport_security_state->SetRequireCTDelegate(
delegate_->GetRequireCTDelegate());
transport_security_state->SetRequireCTDelegate(ct_delegate_.get());
storage_->set_transport_security_state(std::move(transport_security_state));
storage_->set_cert_verifier(delegate_->CreateCertVerifier());

This comment has been minimized.

@deepak1556

deepak1556 Nov 17, 2017

Member

Its better to pass the ct_delegate as a parameter of CreateCertVerifier, can avoid having a GetRequireCTDelegate getter.

This comment has been minimized.

@tarruda

tarruda Nov 17, 2017

Contributor

👍

Fix random crash on app quit.
Move AtomCTDelegate to brightray as RequireCTDelegate and transfer ownership to
brightray::URLRequestContextGetter. This fixes the wrong lifetime assumptions
that result in AtomCTDelegate being used after free in some scenarios.

Close #10051

@tarruda tarruda force-pushed the fix-cert-verification-random-crash-on-exit branch from d2d8ae7 to a9a9e58 Nov 17, 2017

@tarruda

This comment has been minimized.

Copy link
Contributor

tarruda commented Nov 17, 2017

@deepak1556 done, addressed the requests.

@deepak1556
Copy link
Member

deepak1556 left a comment

👍

@jkleinsc jkleinsc merged commit ccb6651 into master Nov 17, 2017

6 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/branch This commit looks good
Details

@jkleinsc jkleinsc deleted the fix-cert-verification-random-crash-on-exit branch Nov 17, 2017

juturu pushed a commit that referenced this pull request May 21, 2018

Fix random crash on app quit
Backported from the upstream
#11125
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment