WebContents Unique identifier proposal - Addressing Dangling Pointers

[Arthur Sonzogni]

Dear Content Owners,

I'm proposing we add unique IDs to the WebContents class to address the issue of dangling pointers.

Given the centrality of WebContents, I suspect others have considered this approach. Do you know if we are pushing back against it?

Context: Following the MiraclePtr rewrite for containers, we know about dangling pointers in std::vector, std::set, std::map, etc... For instance, tests are keeping track of the set of deleted WebContents using dangling pointers. This is not ideal in general, because addresses are reused...
WeakPtr doesn't address this use case, because they are null after invalidation.

Note: I am not even talking about introducing a ID => WebContents* function. This is an orthogonal question.

[John Abd-El-Malek]

Is there a need beyond tests? I ask because if it's just tests, they can stash IDs using its parent class of base::SupportsUserData.

--
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/ac48f2f5-6f32-4800-b65b-525ce80c2db0n%40chromium.org.

[Nasko Oskov]

We already have that, in a kind of indirect way. The root FrameTreeNode has an id and it is constant for the lifetime of the WebContents object, so it can be identified uniquely. Is that insufficient?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CALhVsw134%3DJ-9V2Kc4sBR2F4F65MBpVSnRz%3D1x4W6frA_K6Rsg%40mail.gmail.com.

[Arthur Sonzogni]

I think both solutions would be good enough for now.

Thank you both!

 

Is there a need beyond tests?

Unsure about this yet. I need to process all the containers with dangling WebContent first.

We already have that, [...] FrameTreeNode [...] Is that insufficient?

FrameTreeNode is not public. That being said, we can weirdly extract this ID via the RenderFrameHost.

WebContent()->GetPrimaryMainFrame()->GetFrameTreeNodeId()

I see some embedders using it.

[Nasko Oskov]

On Fri, 15 Mar 2024 at 10:08, Arthur Sonzogni <arthurs...@chromium.org> wrote:

I think both solutions would be good enough for now.

Thank you both!

 

Is there a need beyond tests?

Unsure about this yet. I need to process all the containers with dangling WebContent first.

We already have that, [...] FrameTreeNode [...] Is that insufficient?

FrameTreeNode is not public. That being said, we can weirdly extract this ID via the RenderFrameHost.

WebContent()->GetPrimaryMainFrame()->GetFrameTreeNodeId()

I see some embedders using it.

Yes. We have needed the FTN id which is useful for some practical use cases, but have historically resisted having a public unique identifier for WebContents. The historical reasoning is that WebContents should be created by the owner of it and have a direct relationship and looking up random WebContents has been an antipattern in the earlier days of Chrome.

[Kentaro Hara]

What are the tests testing by holding pointers to already deleted WebContents?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAA%3DmyAv_9D-P7_XSXX0iEQ7RYMXY7uvXnzh46nOWWh3nFiQhhg%40mail.gmail.com.

--

Kentaro Hara, Tokyo

[Arthur Sonzogni]

`discarded_contents_` here for instance. 

Arthur @arthursonzogni

[Kentaro Hara]

`discarded_contents_` here for instance. 

Hmm... I think something strange is going on.

  if (discarded_contents_.find(content.web_contents) != discarded_contents_.cend())
      return false;

^^^ This is saying that the WebContents object pointed to by content.web_contents may have been freed. Then it is wrong to use raw_ptr for content.web_contents... right?

(In theory, I cannot really imagine cases where you need to keep holding pointers to already freed objects.)

--

Kentaro Hara, Tokyo

[Arthur Sonzogni]

This is saying that the WebContents object pointed to by content.web_contents may have been freed. 

I was talking about the one stored in `discarded_contents`. They keep storing pointers about WebContents that are about to be deleted.

I was able to remove the `DanglingUntriaged` in WebContentsData::web_contents. So we know it is no more dangling (in tests). This is a good thing, as it is sometimes dereferenced in production. This was only about a wrong destroy order in tests.

 (In theory, I cannot really imagine cases where you need to keep holding pointers to already freed objects.)

 

Yes ideally.

The majority of the occurrences are involuntary. Some of them are voluntary, though:

• https://source.chromium.org/search?q=raw_ptr.*Experimental.*dead.*;%20-f:third_party&ss=chromium&ssfr=1
• https://source.chromium.org/search?q=raw_ptr.*Experimental.*delet.*;%20-f:third_party&ss=chromium&ssfr=1

Of course, voluntary or involuntary, It's best to put a stop to it.

Arthur @arthursonzogni