I'm no longer working on content settings. msramek/engedy - could you ptal at this? Thanks!

Passing further to engedy@/hkamila@, since this is a good bug to get to know the codebase. I wouldn't have time to have a look anytime soon.

But off the top of my head, I don't know why we may have ever need reference tracking. The ownership structure is clear:

HostContentSettingsMap > Provider > OriginIdentifierValueMap > Rule > [no linked_ptr!] Value

> I don't know why we may have ever need reference tracking.

Perhaps you don't logically *need* it, but I've tried to remove linked_ptr several times and each time I run across parts of the code where there is no clear ownership.

It's less than a day's work for someone who knows the code.

Do we also consider part of the issue here that it is now discouraged to heap-allocate base::Values?

My current push is to kill linked_ptr, but given that the content settings' use of linked_ptr is to hold base::Values, it might make sense to address both at the same time. Go for it!

Will do!

Actually, the more that I hack at this, the more clear it becomes that this is not GoodFirstBug.

My main issue with content settings, which won't be fixed by a transition off of linked_ptr, is that it uses ownership semantics that are incompatible with modern C++. For example, look at ProviderInterface::SetWebsiteSetting. Let me quote the comment on that function:

  // Asks the provider to set the website setting for a particular [...] tuple. If the
  // provider accepts the setting it returns true and takes the ownership of the
  // |value|. Otherwise false is returned and the ownership of the |value| stays
  // with the caller.

All ownership transfers should use smart pointers, https://google.github.io/styleguide/cppguide.html#Ownership_and_Smart_Pointers . However, your design is impossible to build using smart pointers. Either |value| is a raw pointer, in which case its ownership is *never* transferred, or it's a unique_ptr, in which case its ownership is *always* transferred. Passing it with a raw pointer and having the function *sometimes* take ownership and return a bool whether it did or not is a design that we need to move away from.

From inspection, it's not clear if this design is needed any more. You could probably move to using unique_ptr in this case, and have SetWebsiteSetting always take ownership. However, that's work that should be done by the core content settings team, not be a GoodFirstBug.

On a happier note, on my third try I seem to have succeeded in building a CL that removes linked_ptr from content settings. Content settings will still will have a mix of Value, std::unique_ptr<Value>, and Value* 😢 but at least it won't have linked_ptr<Value>. Lemme run it through the trybots and I'll send it your way.

Thank you for investing your time into this, Avi!

I have to admit that I also had to do a double-take the first time I encountered that particular method; and fully agree with you that this interface needs to be refactored.

Would you like to use this bug to track that work or should I file a new bug for that?

I figured out how to get linked_ptr out of content settings without a ton of work, so I'm linking it (https://chromium-review.googlesource.com/c/chromium/src/+/1377523) to the original linked_ptr bug. You can use this bug to track the overall work of figuring out content settings ownership in general.