Should we have RenderFrameHostObserver?

[Charlie Reis]

Hi content-owners, etc!

  It looks like a RenderFrameHostObserver class was added in December, in https://chromium-review.googlesource.com/c/chromium/src/+/4065979.  That's a pretty large change to how we handle notifying features about state changes in content/, and in the (distant) past we've pushed back against it.  For example:

https://groups.google.com/a/chromium.org/g/site-isolation-dev/c/7u1tLpRRW0Q/m/8Mnvi_X9QtQJ

I'm loathe to have something like RenderFrameHostObserver; our experience with having multiple observers in the browser process for WC and RVH were not good (confusing as to where callbacks/state go). It also seems burdensome to require every WCO to also create RFHOs and manage their lifetime.

The motivation in the class-level comment suggests it is to avoid having to use WebContentsObserver from a DocumentUserData:

// This API is appropriate for observer classes extending

// |content::DocumentUserData| (which have a 1-1 relationship and are owned by

// RenderFrameHost) to track the state of a single RenderFrameHost instead of

// the whole frame tree (see WebContentsObserver::RenderFrameHostStateChanged)

I'm not sure if all the previous reasons to avoid adding it still apply (as we move towards RenderDocument and having more document-specific features), but I think we should be careful and intentional about adding a new observer interface like this.  I'd like us to consider the pros and cons before this gets more widely used.

Thoughts?

Charlie

 

[Charlie Reis]

Yes, there may indeed be reasons to consider it.  I just don't want to add it for convenience in a single CL without thinking about the wide-ranging implications (e.g., making features choose between WCO and RFHO, which APIs belong where, what happens as new APIs get added to one and not the other, etc).  This feels like something that should have a design doc and discussion, with a clear statement of the problems we're trying to solve by introducing a new observer interface (like the ones you mention).  It's also a place where we should carefully consider any API changes that might reduce the likelihood of RFH UaFs.

Charlie

On Tue, Jan 10, 2023 at 11:03 AM <dan...@chromium.org> wrote:

Hey Charlie,

I know I have heard thoughts about splitting up WebContentsObserver many times before, as it presents a huge vtable, there's 1000s of wasted calls each time it is used, and it is very hard to hold with so many things. Given that, I was curious if you see RenderFrameHostObserver to be aligned with splitting up WebContentsObserver or if it is doing something entirely different?

If it's aligned then maybe that changes the discussion a little bit, or maybe not?

Cheers,

Dana

--
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/CAH%2B8MBaXb77mBhKVfPWKDktX%2B%2Bn2sr779aTE2jS9NW8QAtd4OQ%40mail.gmail.com.

[Charlie Reis]

I'm not aware of any metrics around WebContentsObserver slowdowns, but it's certainly plausible that it contributes to performance issues.  It's possible the Catan team might have a better sense, or someone could pick this up as a project to measure.

For the time being, I'll suggest changing the current BFCache cases in RenderFrameHostObserver to use WebContentsObserver instead (which can probably be observed via existing RenderFrameHostChanged events?), and removing RenderFrameHostObserver for now.  I'm personally open to the idea of adding a new observer type, but that should be reviewed at more of a design level first, covering some of the topics we've discussed here (as well as things like whether it should be RFHO vs DocumentObserver, or how it will scale to many use cases differently than WCO).

Also happy to hear thoughts from other content owners!

Thanks,

Charlie

On Wed, Jan 11, 2023 at 12:51 AM Thomas Nguyen <tun...@chromium.org> wrote:

I know I have heard thoughts about splitting up WebContentsObserver many times before, as it presents a huge vtable, there's 1000s of wasted calls each time it is used, and it is very hard to hold with so many things

Indeed, that is the major reason for RFHO creation, in the single case of BFCache. I could revert it back and add the logic to WCO, but I also would love to know about the ideas of splitting WCO. Do you have any previous metrics/number/discussion?

On Wed, Jan 11, 2023 at 1:50 AM Kentaro Hara <har...@chromium.org> wrote:

+1 to Charlie that we should discuss whether we want to split WCO into more specific observers or not in general -- if the answer is yes, RFHO is great; otherwise not :)

From my perspective, the biggest cons of the giant WCO is the performance overhead of wasted method calls. Do we have any evidence to believe that the overhead is a problem in practice?

On Wed, Jan 11, 2023 at 4:16 AM <dan...@chromium.org> wrote:

On Tue, Jan 10, 2023 at 2:14 PM Charlie Reis <cr...@chromium.org> wrote:

Yes, there may indeed be reasons to consider it.  I just don't want to add it for convenience in a single CL without thinking about the wide-ranging implications (e.g., making features choose between WCO and RFHO, which APIs belong where, what happens as new APIs get added to one and not the other, etc).  This feels like something that should have a design doc and discussion, with a clear statement of the problems we're trying to solve by introducing a new observer interface (like the ones you mention).  It's also a place where we should carefully consider any API changes that might reduce the likelihood of RFH UaFs.

All agreement from me there.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAHtyhaR_bG8Q3YMSxPgv5T3pPQL%2BZnUNu7_SEDP_13NDXwCOqQ%40mail.gmail.com.

--

Kentaro Hara, Tokyo