How can //content embedders manage lifetime of WebContents?

252 views
Skip to first unread message

Łukasz Anforowicz

unread,
Nov 30, 2022, 2:43:29 PM11/30/22
to content...@chromium.org, Daniel Murphy, Dana Jansens, Nicolas Ouellet-payeur
Hello content...@chromium.org,

I would like to improve the documentation and the shared knowledge about best practices for managing the lifetime of `content::WebContents`.  I would very much appreciate feedback and discussion on this topic.

Some notes to get the discussion started:
  • This email is motivated by seeing bugs that can be root-caused to WebContents that haven't been closed before a BrowserContext/Profile shutdown.  Example: https://crbug.com/1376879
  • This email is also motivated by the realization that I don't really know what the best practices are for managing the lifetime of WebContents in the //chrome layer or the //components layer.
    • I am only aware of BrowserContextKeyedServiceShutdownNotifierFactory and I am far from sure that this is the best practice and what its alternatives are.
    • I assume that no extra work is needed for WebContents added to the tab strip, but I don't know the details.
  • AFAIK //content/OWNERS have pushed back in the past on having a centralized repository/knowledge of all WebContents (i.e. when introducing ChromeContentBrowserClient::OnWebContentsCreated).  Still, maybe we could consider having BrowserContext track a //content-internal `std::set` of `WeakPtr<WebContents>` associated with a given BrowserContext and then in BrowserContext's destructor we could DCHECK/DwoC/CHECK that there are no WebContents left (logging the same crash keys as in RenderFrameHostImpl::AssertBrowserContextShutdownHasntStarted).
  • I hope that our conclusion can eventually be captured in the repo somewhere (//content/public/browser/web_contents.md?  doc comment in //content/public/browser/web_contents.h?).  It is a bit unfortunate that the doc comment of `content::WebContents::Create` doesn't say anything about lifetime expectations (it just says: "Creates a new WebContents.").
Thanks,

Lukasz

Dan Murphy

unread,
Dec 1, 2022, 2:44:58 PM12/1/22
to Łukasz Anforowicz, desktop-pwas-team, content...@chromium.org, Dana Jansens, Nicolas Ouellet-payeur

John Abd-El-Malek

unread,
Dec 2, 2022, 7:35:37 PM12/2/22
to Dan Murphy, Łukasz Anforowicz, desktop-pwas-team, content...@chromium.org, Dana Jansens, Nicolas Ouellet-payeur
Thanks for calling out the deficiencies. It would be good to beef our what content/ expects in its public docs; e.g. that BrowserContexts aren't deleted while there are WebContents that were created using them.

On Thu, Dec 1, 2022 at 11:45 AM 'Dan Murphy' via content-owners <content...@chromium.org> wrote:

On Wed, Nov 30, 2022 at 11:43 AM Łukasz Anforowicz <luk...@chromium.org> wrote:
Hello content...@chromium.org,

I would like to improve the documentation and the shared knowledge about best practices for managing the lifetime of `content::WebContents`.  I would very much appreciate feedback and discussion on this topic.

Some notes to get the discussion started:
  • This email is motivated by seeing bugs that can be root-caused to WebContents that haven't been closed before a BrowserContext/Profile shutdown.  Example: https://crbug.com/1376879
  • This email is also motivated by the realization that I don't really know what the best practices are for managing the lifetime of WebContents in the //chrome layer or the //components layer.
    • I am only aware of BrowserContextKeyedServiceShutdownNotifierFactory and I am far from sure that this is the best practice and what its alternatives are.
    • I assume that no extra work is needed for WebContents added to the tab strip, but I don't know the details.
  • AFAIK //content/OWNERS have pushed back in the past on having a centralized repository/knowledge of all WebContents (i.e. when introducing ChromeContentBrowserClient::OnWebContentsCreated).
To expand: this was for a public facing API 
  •   Still, maybe we could consider having BrowserContext track a //content-internal `std::set` of `WeakPtr<WebContents>` associated with a given BrowserContext and then in BrowserContext's destructor we could DCHECK/DwoC/CHECK that there are no WebContents left (logging the same crash keys as in RenderFrameHostImpl::AssertBrowserContextShutdownHasntStarted).

We should have a lot of flexibility for content-internal checking. Another option is that maybe in DCHECK builds we can have WebContentsImpl put a small class in BrowserContext's UserData to ensure that it (the WC) is deleted first? 
  • I hope that our conclusion can eventually be captured in the repo somewhere (//content/public/browser/web_contents.md?  doc comment in //content/public/browser/web_contents.h?).  It is a bit unfortunate that the doc comment of `content::WebContents::Create` doesn't say anything about lifetime expectations (it just says: "Creates a new WebContents.").
Thanks,

Lukasz

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CA%2B4qT318bboeV9Qq-LGdTuwQ5%3DCDH0fS9ykWKNHqwf_wNydgUQ%40mail.gmail.com.

Łukasz Anforowicz

unread,
Dec 5, 2022, 5:09:04 PM12/5/22
to content-owners, John Abd-El-Malek, Łukasz Anforowicz, desktop-pwas-team, content...@chromium.org, danakj, Nicolas Ouellet-payeur, Dan Murphy
Thanks for the suggestion.  The UserData-based approach certainly avoids many layering gotches.  Let me try to pursue it at https://chromium-review.googlesource.com/c/chromium/src/+/4081770.

To unsubscribe from this group and stop receiving emails from it, send an email to content-owners+unsubscribe@chromium.org.

Łukasz Anforowicz

unread,
Dec 5, 2022, 5:15:07 PM12/5/22
to content-owners, Dan Murphy, content...@chromium.org, danakj, Nicolas Ouellet-payeur, Łukasz Anforowicz, desktop-pwas-team, Alex Ilin, David Roger, Monica Basta, Mihai Sardarescu, Ramin Halavati, Sylvain Defresne, Dominic Battré, Balazs Engedy, Vasilii Sukhanov
+//chrome/browser/profiles/OWNERS
+//chrome/browser/profile_resetter/OWNERS

Are there any practical strategies or other advice for how code in the //chrome layer can manage the lifetime of WebContents?  Would you consider BrowserContextKeyedServiceShutdownNotifierFactory the best practice for this?  Is there documentation of code comments that one could link to (e.g. when updating the doc comment for WebContents::Create here)?

Thanks,

Lukasz

PS. My apologies for haphazardly copy&pasting individual code owners into the CC line.  Please let me know if there are other @chromium.org (or maybe internal) discussion lists I maybe should have used instead.

Mihai Sardarescu

unread,
Dec 7, 2022, 11:46:51 AM12/7/22
to Łukasz Anforowicz, content-owners, Dan Murphy, danakj, Nicolas Ouellet-payeur, desktop-pwas-team, Alex Ilin, David Roger, Monica Basta, Ramin Halavati, Sylvain Defresne, Dominic Battré, Balazs Engedy, Vasilii Sukhanov
Hi Łukasz,

I think it is important to note that the lifecycle of a WebContents is quite clear - the caller of  WebContents::Create() owns the WebContents object and is responsible for destroying it. For WebContents shown in the tab strip, I think the Browser owns them and all the Browser objects are destroyed when the windows are closed. I do not think the lines below fits in WebContents::Create documentation though.

You have 2 ways to be notified about Profile destruction:
* your code is running in a Profile Keyed Service, then override KeyedService::Shutdown() - this method will be called when the profile object is destroyed.
* your code is some UI code that is owned by the Browser - you need to make sure your object is destroyed as part of the browser destruction
* you have some other code that is owned by something else (what would that be?) - you'll probably need to use the BrowserContextKeyedServiceShutdownNotifierFactory - there are a bunch of examples in the code (e.g. https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/signin/turn_sync_on_helper.cc;drc=9445f434ac5f24b27d1dc842c564d8aab8890590;l=71). This allows you to add dependencies on profile services that are supposed to be destroyed only after your object is destroyed.

Hope this helps,
Mihai

David Roger

unread,
Dec 7, 2022, 12:03:41 PM12/7/22
to Mihai Sardarescu, Łukasz Anforowicz, content-owners, Dan Murphy, danakj, Nicolas Ouellet-payeur, desktop-pwas-team, Alex Ilin, David Roger, Monica Basta, Ramin Halavati, Sylvain Defresne, Dominic Battré, Balazs Engedy, Vasilii Sukhanov
It's possible for your WebContents to retain the profile, using a ScopedProfileKeepAlive.

Whether you should do this depends on the desired product behavior: is it acceptable that your web contents are automatically closed if the profile is somehow destroyed? or should we rather keep the profile alive until the user is done with the web contents? If the web contents is shown in a standalone window, then it probably should retain the profile. On the other hand, if it's shown inside a Broswer (or some other larger UI), then the parent window should probably destroy it when it is closed.

Alan Cutter

unread,
Sep 25, 2023, 9:47:40 PM9/25/23
to content-owners, Mihai Sardarescu, content-owners, Dan Murphy, danakj, Nicolas Ouellet-payeur, desktop-pwas-team, Alex Ilin, David Roger, Monica Basta, Ramin Halavati, Sylvain Defresne, Dominic Battré, Balazs Engedy, Vasilii Sukhanov, Łukasz Anforowicz
On Thursday, 8 December 2022 at 3:46:51 am UTC+11 Mihai Sardarescu wrote:
Hi Łukasz,

I think it is important to note that the lifecycle of a WebContents is quite clear - the caller of  WebContents::Create() owns the WebContents object and is responsible for destroying it. For WebContents shown in the tab strip, I think the Browser owns them and all the Browser objects are destroyed when the windows are closed. I do not think the lines below fits in WebContents::Create documentation though.

You have 2 ways to be notified about Profile destruction:
 
* your code is running in a Profile Keyed Service, then override KeyedService::Shutdown() - this method will be called when the profile object is destroyed.

This is currently the case for the WebAppProvider KeyedService (at least that's the intended behaviour).
I want to make the assertion that it should be okay to destroy KeyedService owned WebContents in KeyedService::Shutdown() as that is prior to any KeyedService destruction. We shouldn't have to destroy KeyedService owned WebContents prior to the KeyedService::Shutdown() pass. Does this sound right to Profile owners?

Dan Murphy

unread,
Sep 26, 2023, 11:54:46 AM9/26/23
to Alan Cutter, content-owners, Mihai Sardarescu, danakj, Nicolas Ouellet-payeur, desktop-pwas-team, Alex Ilin, David Roger, Monica Basta, Ramin Halavati, Sylvain Defresne, Dominic Battré, Balazs Engedy, Vasilii Sukhanov, Łukasz Anforowicz
I want to echo Alan here and refute this option from Mihai:

* your code is running in a Profile Keyed Service, then override KeyedService::Shutdown() - this method will be called when the profile object is destroyed.

This does not work for web contents that the WebAppProvider owns - if we wait until this point to destroy them, the system can still crash beforehand (See https://crbug.com/1376879). So unfortunately we have to then move our shutdown code to happen on the profile destruction listener, which isn't great.

Can we change this constraint to allow WebContents to be used up to KeyedService::Shutdown()? Or if not, document why not, and document that all web contents must be destroyed at a given earlier time?

Dan
Reply all
Reply to author
Forward
0 new messages