edited

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?

This comment has been minimized.

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.

This comment has been minimized.

edited

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.

This comment has been minimized.

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.

This comment has been minimized.

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

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.