Renaming "Content-Initiated"

[David Bokan]

Hi all!

I came across a bug recently caused by some confusion as to what "is_content_initiated" means inside Blink. Naively, one would expect !is_content_initiated ==> is_browser_initiated but this is not the case. A "content initiated" navigation from one frame to another cross-origin frame sets is_content_initiated==false.

It seems like this parameter means to describe whether a navigation is being driven internally in Blink (e.g. a synchronous same-document navigation) as opposed coming from the browser's navigation stack.

I've proposed renaming this to "is_handled_within_agent" in this CL. However, this ends up touching on some similar issues in adjacent code:

• NavigationState::CreateBrowserInitiated has the same issue - it's called for content-initiated navigations from a cross-origin frame. It seems like "NavigatedViaBrowser" is more accurate than "BrowserInitiated" which, according to docs, means that a navigation comes from browser UI and is considered more "trusted"
• NavigationState::CreateContentInitiated similarly to the parameter in DocumentLoader appears to be intended for navigations driven from Blink rather than a Content v Browser distinction.
• NavigationRequest::CreateBrowserInitiated is called for content initiated, same-document navigations initiated from a cross-origin frame. The browser_initiated_ bit in NavigationRequest and CommitParams is correctly set to false in this case.

My CL above currently:

• Renames "is_content_initiated" parameters within Blink's same-document navigation code to "is_handled_within_agent"
• Renames NavigationState::CreateBrowserInitiated to CreateBrowserNavigated
• Renames NavigationState::CreateContentInitiated to CreateAgentInternal

I didn't change NaviationRequest::CreateBrowserInitiated as I haven't looked as closely at that, though given that the caller must pass in a |browser_initiated| parameter perhaps just renaming it to a more generic Create would avoid potential confusion? There's already a TODO here to fix the name and potentially merge it with CreateContentInitiated.

Curious if anyone has thoughts on this or is opposed to the changes in my CL above.

Thanks!

David

[Charlie Reis]

I think we might need a bit more exploration here?  "Browser initiated" is usually the inverse of "renderer initiated," and I'm not sure where "content initiated" is supposed to fit in (e.g., a synonym of "renderer initiated" or a different meaning).  I would not want to rename anything until we understand whether those states should be unified or if they need to refer to different things.  

Who's familiar with the uses of "content initiated" and might be able to help?

Charlie

--
You received this message because you are subscribed to the Google Groups "navigation-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to navigation-de...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/133c1971-c675-4a0e-a7ea-cda402ed9d85n%40chromium.org.

[Matt Falkenhagen]

cc rakina and japhet

The CL in question is: https://chromium-review.googlesource.com/c/chromium/src/+/2837370

Content-initiated might be a Blink-centric concept and Nate already lgtm'd it so it might be good to go. I just wanted to double-check with this group first. From what I can see, David seems to make a good case that the particular use of "BrowserInitiated" being renamed was not  actually matching the usual concept of "browser-initiated" as described in the README, and "ContentInitiated" was also not strictly matching what one would think of as "content-initiated".

2021年4月22日(木) 7:38 Charlie Reis <cr...@chromium.org>:

[Charlie Reis]

[+alexmos, mustaq]

If this removes some inconsistent uses of the term "browser initiated," then that's a nice improvement.  Even better if we end up with a clear browser vs renderer initiated distinction going forward and the term "content initiated" is gone, assuming those were separate concepts that just had similar names.

I thought I'd heard the "content initiated" phrase in the context of user activation / user gestures, though, so I wanted to be sure someone familiar with the term had taken a look.  Alex or Mustaq, does this plan sound ok?

Charlie

[Kinuko Yasuda]

+1 for the part that "browser initiated" is typically used for the inverse of "renderer initiated", while it's been somewhat unclear to me where "content initiate" fits in.

If we can make the "content initiate" concept less ambiguous with a clearer term I'm all for it, but I'm less sure about the renames for browser-initiated.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CAH%2B8MBZQPiV6zqMn9UNzs0FS64bL3bi1--qFbdqPAXvwn%3DeoOw%40mail.gmail.com.

[David Bokan]

The renderer vs content confusion is one aspect (I'm not sure about it). However, the main thing that threw me off is the ambiguity of "initiated" inside a renderer. It can either be taken to mean:

1. Where did this renderer receive the navigation from?
2. Where did this navigation begin?
   

For a cross-origin, same document navigation (e.g. link click in one frame navigating a different, cross origin frame) interpretation #1 is from the browser but #2 is from content. It seems like some DocumentLoader code was using |is_content_initiated| using the #1 interpretation but I think most cases in IPCs and browser use #2 (and IMHO is more intuitive and matches documentation). 

I'd also just like to clarify/emphasize with respect to my change:

• The CL in question is changing only renderer-side code and doesn't actually propagate very widely
• It mainly changes the call chain of an is_content_initiated parameter from DocumentLoader in the case of a same-document commit - this is a fairly narrow change and this variable is misleading because it's false when the navigation comes from content in another renderer.
• The only other change is in NavigationState's version of is_content_initiated and Create{Browser|Content}Initiated static methods - the value of which and method used is based on the above parameter so I updated it to "BrowserNavigated"/"AgentInternal" to more clearly match interpretation #1 as well to make sure they're consistent. I think this is semantically correct because:
• NavigationsState's CreateBrowserInitiated and CreateContentInitiated here clearly want the #1 interpretation from above since the difference comes down to whether we supply a Mojo commit callback, commit+common params, CreateContentInitiated sets was_initiated_in_this_frame to true
• As far as I can tell the only place NavigationState::is_content_initiated is read is to give a default page transition value when "content initiated" - presumably because we don't have commit params from the browser to read it. I inverted it to 

As an example I created https://bokand.github.io/nav/xorigin.html to show some of this (if you add logging in the various methods). Two cases that show the misleading naming:

• Clicking "Navigate to fragment" link does a cross-origin frame, same document navigation. This should be a content-initiated navigation but:
  
• calls NavigationRequest::CreateBrowserInitiated
  
• with browser_initiated_ = false
  
• calls NavigationState::CreateBrowserInitiated
  
• Clicking "Navigate to bokand.github.io origin" then "Plain Link (Same Origin)". This is also a content initiated navigation but:
  
• calls NavigationRequest::CreateRendererInitiated
  
• calls NavigationState::CreateBrowserInitiated

In this case the browser side NavigationRequest::CreateBrowserInitiated is also misleading but I think this is a much more isolated edge case. I didn't change it in the CL since I'm less sure about it. Also the value of browser_initiated_ is correctly set in this case so doesn't seem as bad.

[Kinuko Yasuda]

Thanks David and Arthur for clarifying where the confusion comes from, it makes sense to me. (Also thanks for clarifying that NaviationRequest::CreateBrowserInitiated is not touched by the CL at least yet)

I feel I kinda like this one, which feels very explicit

NavigationState::Create()                     // Regular navigation. Driven by the browser process.

NavigationState::CreateForSynchronousCommit() // Mostly renderer-initiated same-document navigation.

While other suggestions by David and Arthur can make good sense too.

On Thu, Apr 22, 2021 at 4:42 PM Arthur Sonzogni <arthurs...@chromium.org> wrote:

Every navigation are "driven" by the browser process, except two: 

• same-document navigation (updating the #fragment, history.replaceState / pushState, ...) initiated from the document.  (excluding the browser initiated ones: omnibox + history ones)
• The synchronous about:blank re-navigation. At some point, we should get rid (and align with Firefox). More info here.

What make those navigation really unique, they are initiated from the renderer process and must take effect synchronously. As a result, an IPC round-trip with the browser process is impossible.

I think I like your suggestions, but I also liked the previous 

I tried to find other suggestion, but they aren't necessarily better than your:

NavigationState::DrivenByBrowserProcess()  // Regular navigation. Driven by the browser process.

NavigationState::DrivenByRendererProcess() // Mostly renderer-initiated same-document navigation.

or

NavigationState::Create()                     // Regular navigation. Driven by the browser process.

NavigationState::CreateForSynchronousCommit() // Mostly renderer-initiated same-document navigation.

On Wednesday, April 21, 2021 at 4:21:54 PM UTC+2 David Bokan wrote:

 I didn't change NaviationRequest::CreateBrowserInitiated as I haven't looked as closely at that, though given that the caller must pass in a |browser_initiated| parameter perhaps just renaming it to a more generic Create would avoid potential confusion? There's already a TODO here to fix the name and potentially merge it with CreateContentInitiated.

See an old CL of mine doing exactly what you described:
https://chromium-review.googlesource.com/c/chromium/src/+/2199263
There are some useful discussions within the patch. I ended up abandoning it.

--
You received this message because you are subscribed to the Google Groups "navigation-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to navigation-de...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/4fd14df1-1106-41f6-a72e-02d120dfa624n%40chromium.org.

[David Bokan]

Thanks both for the feedback and Arthur for the additional context. I agree and like:

NavigationState::Create()                     // Regular navigation. Driven by the browser process.

NavigationState::CreateForSynchronousCommit() // Mostly renderer-initiated same-document navigation.

So I'll change my CL to use these.

[Mustaq Ahmed]

Let me answer Charlie's question while the main discussion around David's CL goes on: in user activation discussion, we use "renderer-initiated" vs "browser-initiated".  I remember seeing few CLs where "content-initiated" came up because of the same confusion David is trying to fix here.  I agree with the general agreement here that "content-initiated" seems ambiguous.

Question from a bird's eye view perspective: any chance we are talking about pre-commit vs post-commit interpretations of browser-vs-renderer initiation?  For example, for David's "Navigate to bokand.github.io origin" bullet above, the mentioned calls would make sense if:

- the unloaded subframe sees the "navigate-away" request as browser-initiated (because it came from a site-isolated frame through the browser), and

- both the browser and the newly loaded frame see the navigation as renderer-initiated.

Do we know which "processes" are the primary users of NavigationRequest vs NavigationState?

--

Join go/fastathon to raise funds for Action Against Hunger

[David Bokan]

On Thu, Apr 22, 2021 at 12:28 PM 'Mustaq Ahmed' via navigation-dev <navigat...@chromium.org> wrote:

Question from a bird's eye view perspective: any chance we are talking about pre-commit vs post-commit interpretations of browser-vs-renderer initiation?

 

Just a nit: I don't think it has to do with pre vs post commit since in some cases (same-origin, same-document navigations) the renderer is the one that performs the commit. I do think, as you mention below, the confusion is based on a browser vs renderer relative interpretation.

 

For example, for David's "Navigate to bokand.github.io origin" bullet above, the mentioned calls would make sense if:

- the unloaded subframe sees the "navigate-away" request as browser-initiated (because it came from a site-isolated frame through the browser), and

- both the browser and the newly loaded frame see the navigation as renderer-initiated.

Do we know which "processes" are the primary users of NavigationRequest vs NavigationState?

 I believe NavigationRequest is exclusive to the browser while NavigationState is exclusive to the renderer, and I think this is the reason for the interpretations being different - but I think it's confusing to have the same term (e.g. "Browser Initiated") mean something different depending on whether we're in the browser or renderer, especially since such values are passed between structures in both.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CAB0cuO4tazQ7vuA8O6QJNT%2BHdu9CDzYNWi4SRPdNY1VSs96ayA%40mail.gmail.com.

[Matt Falkenhagen]

Thanks David and all for the discussion. I like the names too and lgtm'd the CL now.

2021年4月23日(金) 3:19 David Bokan <bo...@google.com>: