Crash in AdblockCnameResolveHostClient

[iefremov]

https://brave.sp.backtrace.io/p/brave/debug?time=(relative-first-seen%2Cfloating%2Cweek)&filters=((callstack.functions%2Ccontains%2CAdblockCnameResolveHostClient)%2Cptype%3Dbrowser%2C_deleted%3D0)&debug=(%2215faebf%22,0,0)

[ 00 ] <name omitted>
[ 01 ] content::BrowserContext::GetDefaultStoragePartition(content::BrowserContext*)
[ 02 ] brave::AdblockCnameResolveHostClient::AdblockCnameResolveHostClient(base::RepeatingCallback<void ()> const&, scoped_refptr<base::SequencedTaskRunner>, std::__1::shared_ptr<brave::BraveRequestInfo>, brave::EngineFlags)
[ 03 ] brave::OnShouldBlockRequestResult(bool, scoped_refptr<base::SequencedTaskRunner>, base::RepeatingCallback<void ()> const&, std::__1::shared_ptr<brave::BraveRequestInfo>, brave::EngineFlags)
[ 04 ] void base::internal::ReplyAdapter<brave::EngineFlags, brave::EngineFlags>(base::OnceCallback<void (brave::EngineFlags)>, std::__1::unique_ptr<brave::EngineFlags, std::__1::default_delete<brave::EngineFlags> >*)
[ 05 ] base::internal::Invoker<base::internal::BindState<void (*)(base::OnceCallback<void (brave::EngineFlags)>, std::__1::unique_ptr<brave::EngineFlags, std::__1::default_delete<brave::EngineFlags> >*), base::OnceCallback<void (brave::EngineFlags)>, base::internal::OwnedWrapper<std::__1::unique_ptr<brave::EngineFlags, std::__1::default_delete<brave::EngineFlags> >, std::__1::default_delete<std::__1::unique_ptr<brave::EngineFlags, std::__1::default_delete<brave::EngineFlags> > > > >, void ()>::RunOnce(base::internal::BindStateBase*)
[ 06 ] base::(anonymous namespace)::PostTaskAndReplyRelay::RunReply(base::(anonymous namespace)::PostTaskAndReplyRelay)
[ 07 ] base::internal::Invoker<base::internal::BindState<void (*)(base::(anonymous namespace)::PostTaskAndReplyRelay), base::(anonymous namespace)::PostTaskAndReplyRelay>, void ()>::RunOnce(base::internal::BindStateBase*)
[ 08 ] <name omitted>

[iefremov]

cc @antonok-edm @mariospr

[mariospr]

@iefremov I think this crash might be happening after all because of the assumption we made in the end on that we could simply use the context stored in ctx, instead of trying to retrieve the web contents first and early return if needed, which is what the original code did:

  AdblockCnameResolveHostClient(
      const ResponseCallback& next_callback,
      scoped_refptr<base::SequencedTaskRunner> task_runner,
      std::shared_ptr<BraveRequestInfo> ctx) {
    [...]

    auto* web_contents = GetWebContents(
        ctx->render_process_id, ctx->render_frame_id, ctx->frame_tree_node_id);
    if (!web_contents) {
      start_time_ = base::TimeTicks::Now();
      this->OnComplete(net::ERR_FAILED, net::ResolveErrorInfo(), base::nullopt);
      return;
    }

    content::BrowserContext* context = web_contents->GetBrowserContext();
    
    [...]

    network::mojom::NetworkContext* network_context =
        content::BrowserContext::GetDefaultStoragePartition(context)
            ->GetNetworkContext();

    [...]
  }

Instead, we now do this:

  AdblockCnameResolveHostClient(
      const ResponseCallback& next_callback,
      scoped_refptr<base::SequencedTaskRunner> task_runner,
      std::shared_ptr<BraveRequestInfo> ctx,
      EngineFlags previous_result) {

    [...]

    network::mojom::NetworkContext* network_context =
        content::BrowserContext::GetDefaultStoragePartition(
            ctx->browser_context)
            ->GetNetworkContext();

    [...]
  }

@iefremov It looks like there could be some race condition and blindly trusting ctx->browser_context might not be the right thing to do, so I'll prepare a patch to go back to retrieving the WebContents from the frame_tree_node_id instead + early returning with net::ERR_FAILED in case it can't be found, if that's ok.

[iefremov]

yeah it seems this callback can outlive the browser context... Due to using shared_ptr and not canceling properly. So let's revert for now. Thanks @mariospr

[mariospr]

@iefremov Great. Just checked that all unit tests and all browser tests keep passing after that change, so here is the PR:
brave/brave-core#8921

[iefremov]

@mariospr could you please uplift to beta?

[mariospr]

Uplift available in brave/brave-core#8934