This is a reland of commit 33493d2d85
This restores a branch in RenderViewHostImpl::CreateRenderView() that
was mistakenly removed in the original CL.
Original change's description:
> [prerender] Fix crash when discarding a speculative main RenderFrameHost
>
> Previously, starting a prerender navigation would create a speculative
> RenderFrameHost for the root frame tree node in the browser process.
> However, the corresponding RenderFrame in the renderer process would
> be created as the active/current main RenderFrame. This is a result of
> the interaction between `CreateProxiesForSiteInstanceGroup()`, which
> skips creating a proxy if it thinks the early commit optimization will
> be used, and `PerformEarlyRenderFrameHostSwapIfNeeded()`, which skips
> performing early commits in prerendering frame trees.
>
> Intentional or not, this is problematic because:
>
> - The browser process assumes speculative RenderFrameHosts can be
> cleaned up by deleting the corresponding RenderFrame in the renderer
> process.
> - The renderer process assumes the active/current main RenderFrame is
> never explicitly deleted, and that it will only be implicitly deleted
> as a side effect of explicitly deleting its blink::WebView.
> - Blink assumes a blink::WebView always has an active main frame (i.e.
> non-null). Rather than crashing non-deterministically, the renderer
> process CHECK-fails if the browser tries to explicitly delete a
> non-provisional main RenderFrame.
>
> Taken together, this means discarding a speculative main RenderFrameHost
> during a prerender navigation will crash the renderer process. Needless
> to say, this is not desirable. Previous fixes have centered around
> identifying the cases that would discard a speculative RenderFrameHost
> and disabling prerendering for those cases.
>
> However, this doesn't fix the underlying issue and ends up being a game
> of whack-a-mole. This CL implements a proper fix: the corresponding
> main RenderFrame in the renderer process is now created as a provisional
> frame during a prerender navigation. This is implemented using a
> placeholder RemoteFrame; RenderDocument already uses this concept for
> local -> local frame swaps.
>
> This CL also refactors several ancillary components to make the updated
> logic easier to follow:
>
> - Rather than using a non-null `previous_frame_token` as the implicit
> signal for creating a provisional main local frame, Mojo now uses the
> CreateProvisionalLocalMainFrameParams variant subtype to convey this
> intent. This is a required cleanup, since prerender navigations should
> not set a `previous_frame_token`, but still want the placeholder
> RemoteFrame + provisional LocalFrame behavior.
> - With the updated union, the different main frame cases are also broken
> out more distinctly in `AgentSchedulingGroup::CreateView()`.
> - Rename `EnsureWidgetInitialized()` to `InitializeWidgetAtSwap()`,
> since that is the only situation it is called.
> - Remove `previous_frame_token_for_compositor_reuse` from the frame
> widget creation params, and replace it with the `reuse_compositor`
> bool. The frame token is redundant, since the frame widget creation
> params are always paired with frame creation params.
> - Refactor `WillDetach()` and `InitializeWidgetAtSwap()` to account for
> the removal of `previous_frame_token_for_compositor_reuse`. It turns
> out the renderer process already has all the information needed to
> determine the previous frame; passing it directly significantly
> simplifies this coordination.
>
> Finally, this also removes the crash instrumentation that was previously
> added to help track down situations where prerender navigations would
> discard speculative main RenderFrameHosts. Since this no longer crashes,
> the instrumentation is no longer needed.
>
> Bug: 40076091
> Change-Id: I1c19853c75580492653e608afb1db3442a264b3a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5922799
> Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1369647}
Bug: 40076091
Change-Id: I10ecdf8233f6dfd42e0478c819447831d50256c3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5937586
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1369798}
This reverts commit 33493d2d85.
Reason for revert: CL has bug :)
Original change's description:
> [prerender] Fix crash when discarding a speculative main RenderFrameHost
>
> Previously, starting a prerender navigation would create a speculative
> RenderFrameHost for the root frame tree node in the browser process.
> However, the corresponding RenderFrame in the renderer process would
> be created as the active/current main RenderFrame. This is a result of
> the interaction between `CreateProxiesForSiteInstanceGroup()`, which
> skips creating a proxy if it thinks the early commit optimization will
> be used, and `PerformEarlyRenderFrameHostSwapIfNeeded()`, which skips
> performing early commits in prerendering frame trees.
>
> Intentional or not, this is problematic because:
>
> - The browser process assumes speculative RenderFrameHosts can be
> cleaned up by deleting the corresponding RenderFrame in the renderer
> process.
> - The renderer process assumes the active/current main RenderFrame is
> never explicitly deleted, and that it will only be implicitly deleted
> as a side effect of explicitly deleting its blink::WebView.
> - Blink assumes a blink::WebView always has an active main frame (i.e.
> non-null). Rather than crashing non-deterministically, the renderer
> process CHECK-fails if the browser tries to explicitly delete a
> non-provisional main RenderFrame.
>
> Taken together, this means discarding a speculative main RenderFrameHost
> during a prerender navigation will crash the renderer process. Needless
> to say, this is not desirable. Previous fixes have centered around
> identifying the cases that would discard a speculative RenderFrameHost
> and disabling prerendering for those cases.
>
> However, this doesn't fix the underlying issue and ends up being a game
> of whack-a-mole. This CL implements a proper fix: the corresponding
> main RenderFrame in the renderer process is now created as a provisional
> frame during a prerender navigation. This is implemented using a
> placeholder RemoteFrame; RenderDocument already uses this concept for
> local -> local frame swaps.
>
> This CL also refactors several ancilliary components to make the updated
> logic easier to follow:
>
> - Rather than using a non-null `previous_frame_token` as the implicit
> signal for creating a provisional main local frame, Mojo now uses the
> CreateProvisionalLocalMainFrameParams variant subtype to convey this
> intent. This is a required cleanup, since prerender navigations should
> not set a `previous_frame_token`, but still want the placeholder
> RemoteFrame + provisional LocalFrame behavior.
> - With the updated union, the different main frame cases are also broken
> out more distinctly in `AgentSchedulingGroup::CreateView()`.
> - Rename `EnsureWidgetInitialized()` to `InitializeWidgetAtSwap()`,
> since that is the only situation it is called.
> - Remove `previous_frame_token_for_compositor_reuse` from the frame
> widget creation params, and replace it with the `reuse_compositor`
> bool. The frame token is redundant, since the frame widget creation
> params are always paired with frame creation params.
> - Refactor `WillDetach()` and `InitializeWidgetAtSwap()` to account for
> the removal of `previous_frame_token_for_compositor_reuse`. It turns
> out the renderer process already has all the information needed to
> determine the previous frame; passing it directly significantly
> simplifies this coordination.
>
> Finally, this also removes the crash instrumentation that was previously
> added to help track down situations where prerender navigations would
> discard speculative main RenderFrameHosts. Since this no longer crashes,
> the instrumentation is no longer needed.
>
> Bug: 40076091
> Change-Id: I1c19853c75580492653e608afb1db3442a264b3a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5922799
> Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1369647}
Bug: 40076091
Change-Id: Ic6caff54e1d3085603809fdefb036b277c4720d5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5938653
Auto-Submit: Daniel Cheng <dcheng@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1369651}
Previously, starting a prerender navigation would create a speculative
RenderFrameHost for the root frame tree node in the browser process.
However, the corresponding RenderFrame in the renderer process would
be created as the active/current main RenderFrame. This is a result of
the interaction between `CreateProxiesForSiteInstanceGroup()`, which
skips creating a proxy if it thinks the early commit optimization will
be used, and `PerformEarlyRenderFrameHostSwapIfNeeded()`, which skips
performing early commits in prerendering frame trees.
Intentional or not, this is problematic because:
- The browser process assumes speculative RenderFrameHosts can be
cleaned up by deleting the corresponding RenderFrame in the renderer
process.
- The renderer process assumes the active/current main RenderFrame is
never explicitly deleted, and that it will only be implicitly deleted
as a side effect of explicitly deleting its blink::WebView.
- Blink assumes a blink::WebView always has an active main frame (i.e.
non-null). Rather than crashing non-deterministically, the renderer
process CHECK-fails if the browser tries to explicitly delete a
non-provisional main RenderFrame.
Taken together, this means discarding a speculative main RenderFrameHost
during a prerender navigation will crash the renderer process. Needless
to say, this is not desirable. Previous fixes have centered around
identifying the cases that would discard a speculative RenderFrameHost
and disabling prerendering for those cases.
However, this doesn't fix the underlying issue and ends up being a game
of whack-a-mole. This CL implements a proper fix: the corresponding
main RenderFrame in the renderer process is now created as a provisional
frame during a prerender navigation. This is implemented using a
placeholder RemoteFrame; RenderDocument already uses this concept for
local -> local frame swaps.
This CL also refactors several ancilliary components to make the updated
logic easier to follow:
- Rather than using a non-null `previous_frame_token` as the implicit
signal for creating a provisional main local frame, Mojo now uses the
CreateProvisionalLocalMainFrameParams variant subtype to convey this
intent. This is a required cleanup, since prerender navigations should
not set a `previous_frame_token`, but still want the placeholder
RemoteFrame + provisional LocalFrame behavior.
- With the updated union, the different main frame cases are also broken
out more distinctly in `AgentSchedulingGroup::CreateView()`.
- Rename `EnsureWidgetInitialized()` to `InitializeWidgetAtSwap()`,
since that is the only situation it is called.
- Remove `previous_frame_token_for_compositor_reuse` from the frame
widget creation params, and replace it with the `reuse_compositor`
bool. The frame token is redundant, since the frame widget creation
params are always paired with frame creation params.
- Refactor `WillDetach()` and `InitializeWidgetAtSwap()` to account for
the removal of `previous_frame_token_for_compositor_reuse`. It turns
out the renderer process already has all the information needed to
determine the previous frame; passing it directly significantly
simplifies this coordination.
Finally, this also removes the crash instrumentation that was previously
added to help track down situations where prerender navigations would
discard speculative main RenderFrameHosts. Since this no longer crashes,
the instrumentation is no longer needed.
Bug: 40076091
Change-Id: I1c19853c75580492653e608afb1db3442a264b3a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5922799
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1369647}
This CL removes no-longer-used headers from
RenderAccessibilityImpl and related files. A few consumers of
render_accessibility_impl.h needed to have some additional
includes added for IWYU.
This CL also removes load_inline_text_boxes_ids_ from
RenderAccessibilityImpl, as that isn't used anywhere.
No behavioral changes are expected.
AX-Relnotes: n/a
Change-Id: Ifbc871477830301d89654c393d2e7e77697ad2ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5901000
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Stefan Zager <szager@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Kurt Catti-Schmidt <kschmi@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1363135}
This reduces some flaky tests when a race condition happens and a full serialization occurs that needed info from the location updates.
If we're sending a full serialization anyways it actually makes sense to use one serialization than two seperate ones anyways.. Less overhead.
Change-Id: Ie64acd34e6da92344b423dc013ba40497e5d17d0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5878322
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Dominic Farolino <dom@chromium.org>
Reviewed-by: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Commit-Queue: Ahmed Elwasefi (Ahmad45123) <a.m.elwasefi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1362560}
SerializeFrame now returns serialized resources in a callback.
A subsequent change will take advantage of this and fetch some
resources asynchronously.
This change should be a no-op.
Bug: 363289333
Change-Id: If00c8b47add50e384921c29e2a88a167b5b4a058
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5899715
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1362478}
Now that we don't have any reports for browser vs renderer calculated
origin mismatch, we can start setting origin_to_commit in
CommitNavigationParams for all cases, to be used by the renderer
in all cases (except when inheriting bits like document.domain).
This behavior is protected behind a flag.
Bug: 888079
Change-Id: I782ad14713c1228f46fea8347121784a3cfc5c07
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5873739
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1360576}
Duplicate navigations are navigations that are the same as the
current ongoing navigations, and likely to be triggered accidentally
by e.g. double link/button clicks. This CL makes it so that we
record duplicate navs as a separate NavigationDiscardReason.
Also, currently when a new renderer-initiated navigation is
triggered while there is already another navigation initiated by
the same renderer ongoing, the older navigation is canceled by
overwriting the NavigationClient. This cancellation is incorrectly
marked as kInternalCancellation because the overwrite didn't
set a reason. This CL fixes that by setting the reason to
indicate that the navigation client is disconnected because of
a new navigation / duplicate navigation. This CL also makes
form submission-initiated cancellation mark the disconnection
correctly (as "new navigation").
Bug: 366060351
Change-Id: I084ca4dbd56805192cf4d553ab866e011d7d1917
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5856745
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1355642}
Given the high volume of reports, try to avoid a renderer crash when a
same-document navigation fails to commit because no current history item
exists in the renderer process. Instead, restart the navigation as
cross-document and send a DumpWithoutCrashing report for now.
Crash keys included:
* history_no_item-is_main_frame: Main frame vs subframe.
* history_no_item-renderer_commit_state: Whether renderer is still on
initial empty document (0), or in a different NavigationCommitState.
* history_no_item-browser_history_offset: Browser's view of current
history index.
* history_no_item-browser_history_len: Browser's view of history length.
* history_no_item-renderer_history_len: Renderer's view of history
length.
Bug: 41489044
Change-Id: I294ac92e6fb2682e5c2b4adbe3040fb0fff0f201
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5859355
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1355430}
We currently ignore duplicated link clicks in favor of continuing the
older navigation. This CL expands this behavior to non-link click
navigations, both on the renderer and the browser side. Also adds
a histogram to see how often we hit the duplicated case, and a
timeout threshold for categorizing two navigations as duplicates,
set to 2000ms by default.
Bug: 366060351
Change-Id: Ic65c05217ef52a72f8ee0aa07c4be6c94b13caa5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5856811
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1355065}
If 2 link clicks to the same URL happens on the same time, it's likely
that it is caused by accident. We probably should keep the first
navigation since they will most likely result in the same page.
This CL adds the logic to do that, protected behind a flag.
Bug: 366060351
Change-Id: I344fcfbf0831b0fa16fcb19b8695f2986a1cf612
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5837959
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1354370}
https://chromium-review.googlesource.com/c/chromium/src/+/5801311/4/content/renderer/render_frame_impl.cc#5516 accidentally switched from checking the WebUI bindings to checking for any bindings.
If we write a script like window.location.href = "/test.html" in the head section, the expected behavior is that `DOMContentLoaded` will not be triggered due to the early redirection. However, due to the above error, OpenURL will be called in the Browser, resulting in render not being detached immediately, so the DOM will be fully loaded.
This CL fixes the issue by checking the same two WebUI bindings as the original code did.
Bug: 365712501
Change-Id: Ic1769a64b8b89d08b50255c5aa810b0f5a83f5c2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5850489
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: q lamry <zhaoy@microsoft.com>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1353727}
Specifically if we're going to use a resource from the cache
then a lot of the work during prepare isn't necessary. To enable
this I've moved some of the work earlier, specifically into
a new function PopulateResourceRequestBeforeCacheAccess. I've also
introduced a new client interface specifically for modifying the url
(rather than through WillSendRequest). In FetchContext I renamed
PopulateResourceRequest to UpgradeResourceRequestForLoader. In
resource_requestion_utils I renamed PrepareResourceRequest to
UpgradeResourceRequestForLoader.
This change is quite risky, so I added a kill switch. Unfortunately
the kill switch makes the code pretty ugly at the moment. This is
part of the reason I haven't looked into combining
PopulateResourceRequest and PrepareResourceRequest. I will tackle
that assuming this change sticks.
There are a couple of things not gated by the kill switch:
. new function is always called on client
. transparent placeholder image logic in ResourceFetcher.
These changes are pretty straightforward though, and it would
have made the code even more awkward to gate it by the kill switch.
Bug: 359910398
Change-Id: Ic25b302b9b3372084ff23496e623d0b6a21ed908
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5794358
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1347392}
We have Navigation.CommitRenderFrame to measure the time taken for the
navigation commit in the renderer. But we also want to see the breakdown
of frame types (MainFrame or Subframe), and the new navigation case in
the outermost main frame.
Bug: 352578800
Change-Id: I602b324e83f8cbeda9d46a8ef021b73b4e783320
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5803884
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Shunya Shishido <sisidovski@chromium.org>
Reviewed-by: Minoru Chikamune <chikamune@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1346501}
This CL implements the barest knowledge the renderer should need to have
of whether it represents a partitioned popin. This knowledge is
necessary to ensure proper checks for data access, like the cookies
tested here, are possible.
This series of CLs implement core components of the Partitioned Popin
system, significant additional effort will be needed to align with the
explainer and I2P, but all of that will depend on this work:
(1) Implement `popin` window feature
(2) `popin` feature triggers tab modal popup
(3) `popin` feature triggers third-party storage partitioning
(4) Renderer awareness of popin top-origin
(5) Limit window.opener access for popin
(6) Add permissions policy for popin
Explainer: https://explainers-by-googlers.github.io/partitioned-popins/
I2P: https://groups.google.com/a/chromium.org/g/blink-dev/c/ApU_zUmpQ2g/
Bug: 340606651
Change-Id: I036c08c2be0f7b8a315b1b517ddbdd3cb72927a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5797543
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Auto-Submit: Ari Chivukula <arichiv@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1344788}
Before this CL, the maximum bucket of the following UMAs were 10
seconds. But the data showed that 30% of the data fell into the maximum
bucket. This CL extends the maximum bucket up to 1 hour.
Navigation.RendererRunLoopStartToFirstCommitNavigation.MainFrame
Navigation.RendererRunLoopStartToFirstCommitNavigation.Subframe
OBSOLETE_HISTOGRAMS=Patterned histogram Navigation.RendererRunLoopStartToFirstCommitNavigation.{FrameType} is replaced by Navigation.RendererRunLoopStartToFirstCommitNavigation2.{FrameType}
Bug: 342967843
Change-Id: Idd882a6524ddb4481dc899a0c5d98001151c78ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5776182
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Minoru Chikamune <chikamune@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1339561}
With PDF enforcements from r1319795, PDF renderer processes are not
allowed to access localStorage and sessionStorage; the browser process
expects that the renderers would never use these interfaces to begin
with and terminates the renderers if it does see such an access (in
DOMStorageContextWrapper::IsRequestValid()).
However, it turns out that these interfaces may still be accessed via
DevTools when switching context to the PDF document. The goal of this
CL is to make this fail more gracefully than with a renderer kill, by
denying access to localStorage/sessionStorage in Blink, similarly to
how storage access is denied for other cases like opaque origins or
the WebPreferences::local_storage_enabled setting. This is done by
adding plumbing to ask ChromeContentRendererClient, which knows
whether the current renderer is for PDF. See
https://crbug.com/357014503 for a summary of alternatives
considered. Longer-term, we should explore whether PDF documents
should just get an opaque origin, which would be a more comprehensive
fix if it doesn't break compat.
Bug: 357014503
Change-Id: I90995daceccd4b448523d44bbfc5229d4f27348a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5768152
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1338897}
This CL adds the following UMAs and trace logs.
- Navigation.RendererRunLoopStartToFirstCommitNavigation.MainFrame
- Navigation.RendererRunLoopStartToFirstCommitNavigation.Subframe
These UMAs and trace logs help us to know how long we can spend some
preparation work when the renderer process is started before the first
navigation on the renderer.
Bug: 342967843
Change-Id: I1c2ce60266576c3023e40ba77ba0ed8029189468
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5751245
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Commit-Queue: Minoru Chikamune <chikamune@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1337279}
Given a media playing iframe, this CL implements the plumbing required
to send the frame's FrameVisibility to the WebMediaPlayer object
responsible for the frame's media playback. This CL also cover the
scenario where a local frame needs to send a frame visibility update to
a remote frame.
During LocalFrameView::RunIntersectionObserverSteps all the MainFrame's
children will have their FrameVisibility status checked. Then, this data
should be sent to the frames RenderFrameImpl object, which implements
the blink::WebLocalFrameClient interface. Then, the frame's
RendererWebMediaPlayerDelegate, which implements the RenderFrameObserver
interface will also be informed of the visibility change. Finally, the
frame WebMediaPlayerImpl and WebMediaPlayerMS objects will be informed
of the frame visibility changes.
There are 2 different routes to send the visibility change data to a
frame's RenderFrameImpl object from its FrameView object:
- Local Frames
The visibility information is kept in the renderer process.
LocalFrameView::VisibilityChanged will pass this information to the
frame's RenderFrameImpl by calling
RenderFrameImpl::OnFrameVisibilityChanged.
- Remote Frames
The visibility change information needs to be sent first to the browser
process, so that it can then redirected to the renderer process which
contains the RemoteFrame's LocalFrame equivalent. The info flow is:
Renderer process:
- RemoteFrameView::VisibilityChanged
Browser process:
- RenderFrameProxyHost::VisibilityChanged
- CrossProcessFrameConnector::OnVisibilityChanged
- RenderFrameHostImpl::VisibilityChanged
Renderer process
- LocalFrameMojoHandler::OnFrameVisibilityChanged
- RenderFrameImpl::OnFrameVisibilityChanged
Then the process continues as the Local Frame case.
This feature is under active development. Therefore, its behavior is still under discussion and may change in the future.
Bug: 351354996
Change-Id: Ie1247ff31564b599a25327ec217cd6e3df43228d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5689283
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Gabriel Brito <gabrielbrito@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1335033}
At a high-level this CL moves the
`content::RenderFrameImpl::browser_interface_broker_proxy_` field into
`blink::LocalFrame`. This CL is based on an earlier attempt from
@mstensho at https://crrev.com/c/5105333 (thanks!).
This CL is motivated by the desire to make `BrowserInterfaceBrokerProxy`
store `blink::HeapMojoRemote<T>` (instead of a regular
`mojo::Remote<T>`) which means that `BrowserInterfaceBrokerProxy` can
**only** be used as a field of garbage collected classes (since
`HeapMojoRemote` has a `Member<T>` field and exposes a `Trace` method).
This CL does exactly this - after this CL `BrowserInterfaceBrokerProxy`
will only be embedded in garbage-collected classes (like
`blink::LocalFrame` and `blink::WorkerGlobalScope` and *unlike*
`content::RenderFrameImpl`).
The move requires splitting `BrowserInterfaceBrokerProxy` into a pure
virtual interface (exposing a public, somewhat Blink-agnostic API that
uses types like `CrossVariantMojo...`) and an impl (using Blink/WTF
idioms). The move opportunistically simplifies the public API by
pushing `Bind` and `is_bound` methods into the impl. The move
temporarily introduces some layering-related ickiness which will be
resolved by the follow-up CL at https://crrev.com/c/5651622.
Bug: 41482945
AX-Relnotes: n/a.
Change-Id: I411092bf1301e4f7f3994c8ec11532ab1a9eba25
Binary-Size: Artifact of PGO - see https://crbug.com/344608183#comment9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5647559
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1326840}
This CL does not change any behavior; it is a mechanical change that
introduces a new 2-state enum to replace a boolean, and renames the
associated methods/variables.
(This CL also includes the corresponding mojo version of the enum, and
EnumTraits specialization and tests.)
This CL *does* change a test (url_request_mojom_traits_unittest.cc) to
supply a non-default value for the relevant field, rather than the
field's default value (since this test is about round-tripping the
mojo struct).
Bug: b:348671111, 344608182
Change-Id: I89c8657235e60734513cfc840fe61d3c12b80a77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5683463
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Auto-Submit: Chris Fredrickson <cfredric@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Owners-Override: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1325075}
This manager is only used to report metrics calculated in cc/metrics.
It's location in cc/trees predates the cc/metrics folder.
This change moves it, ahead of some follow ups to it's ownership, and
potential new UKMs
Bug: 334977830
Change-Id: Icbab339fe18ea4ef0f06b42b03f023125f4801a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5648501
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Ian Vollick <vollick@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1322830}
To address breakage of named window reuse across a back navigation
when a proactive BrowsingInstance swap has happened, we offer the
option to use an explicit opener relation for same-window navigations
as an opt-out from proactive swaps. This applies to elements that
support rel=opener (a, area, and form). We also introduce "opener"
as a window feature (for window.open()).
Usage looks like the following:
Before:
<a href="next.html">next</a>
After:
<a href="next.html" rel="opener">next</a>
Before:
location.href = getNextPageUrl();
After:
window.open(getNextPageUrl(), '_self', 'opener');
Note that the opt-out only affects proactive swaps. It cannot be used
to bypass swaps that are required (e.g. for COOP).
Usage of conflicting rel types (rel="noopener opener") resolves in
favour of noopener.
WPTs still need to be added in a future CL.
Explainer: https://github.com/explainers-by-googlers/future-browsing-context-group-dependency-hint
Intent to Prototype: https://groups.google.com/u/1/a/chromium.org/g/blink-dev/c/sI1zySADmNs
Bug: 333743493
Change-Id: Ie25acba119de231ed2b2532c2c1c2212e705264c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5560607
Reviewed-by: Brendon Tiszka <tiszka@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Stephen Nusko <nuskos@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1320669}
This CL changes the signature of the `GetBrowserInterfaceBroker` method
from:
`blink::BrowserInterfaceBrokerProxy* GetBrowserInterfaceBroker()`
to:
`const blink::BrowserInterfaceBrokerProxy& GetBrowserInterfaceBroker()`.
Motivation:
* The old implementation never returns null - this is communicated
in the new API by returning a reference instead of a pointer.
* Users of the old API never call `Bind` or `Reset` - this is
communicated/enforced in the new API by returning a **`const`**
reference (`Bind` and `Reset` methods of `BrowserInterfaceBrokerProxy`
are non-`const`).
Bug: 41482945
AX-Relnotes: n/a.
Change-Id: Ifec4c1eb0ee556f9e10a6da2e0f90f4c39bd2647
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5651361
Owners-Override: Alexander Timin <altimin@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1319846}
To help determine the reason a navigation gets abandoned, we now
save the NavigationDiscardReason that's passed in to functions that
can cancel navigations, and expand it to be more informative. This
value will also now be saved in PageLoadMetricsObserver's
FailedProvisionalLoadInfo, to later be passed in to observers.
Example usage: crrev.com/c/5631386.
Bug: 347706997
Change-Id: I7698ed8f5d8f7527ad5006b5ceec8f98d85696a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5631388
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1318957}
At present, the length of the initiator_base_url sent from the renderer
to the embedder is not checked. If that length exceeds url::kMaxURLChars
then Mojo will replace it with an empty GURL, causing the
CommonNavigationParams sent to have an empty base url value that
is not nullopt. This leads to a check failure in
NavigationRequest's constructor.
This CL checks the base url value in the renderer, and if it exceeds
the maximum length, does not include it in CommonNavigationParams.
If the browser receives a non-nullopt initiator_base_url with an
empty url, it's assumed something is wrong and the renderer is
terminated with BADMESSAGE.
Bug: 346908892
Change-Id: Ibea8740099af7269b5e46b25e06923c5edc33c74
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5641878
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Luc Nguyen <lucnguyen@google.com>
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1318077}
This recent patch made zoom level a property on WebFrameWidget rather
than WebView:
https://chromium-review.googlesource.com/c/chromium/src/+/5571113
However, it left the propagation routes for zoom information in a bit of
a crazy state. This CL is a pure refactor with no behavioral change,
intended only to make the propagation mechanism more consistent and
sane.
All of the inputs to the zoom factor calculation arrive via IPC to a
frame widget. Zoom level is handled directly by WebFrameWidget; all
other factors are delegated to WebView, which propagates them down to
all WebFrameWidgets contained by the WebView. The WebFrameWidget in turn
is responsible for propagating zoom factor to the root LocalFrame of the
widget, and the root frame is responsible for propagating it to
descendant non-root frames.
This also activates the path for propagating zoom level from embedder to
process-isolated iframe via VisualProperties::zoom_level. Currently,
that value is ignored by RenderWidgetHostImpl::GetVisualProperties in
favor of the WebContents-level value. After this change, the value set
by the embedder will be used instead. Currently, the two values are
always identical, so this is a pure plumbing change with no behavior
difference. A follow-up CL will allow the value set by the embedder to
deviate from the WebContents-level value based on CSS zoom.
Bug: chromium:329482480
Change-Id: I18991c9db9f1d1be8ffad7677d6a551ab70495f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5611483
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: Stefan Zager <szager@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1313827}
The browser allocates a new LocalSurfaceId for a newly committed
Document *after* receiving the DidCommitNavigation from the renderer.
However, the new Document is installed in the widget when the renderer
receives the commit message. For same-RFH navigations, this means the
new Document can start rendering frames before it has received a new
LocalSurfaceId via SynchronizeVisualProperties dispatched in response
to the DidCommitNavigation IPC by the browser process.
Since caching screenshots for a navigation is done by associating the
frames produced by a Document to its LocalSurfaceId, the above can
result in incorrect capture.
This change fixes this by allocating a new LocalSurfaceId when sending
the CommitNavigation IPC to the renderer. This ensures that the
compositor receives a new LocalSurfaceId at the same time as it
switches the Document rendering to this compositor.
R=rakina@chromium.org, liuwilliam@chromium.org
Fixed: 41491612
Change-Id: I5e2336c5ef20b841cb21e1d88dd28ece41be797f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5583357
Auto-Submit: Khushal Sagar <khushalsagar@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Khushal Sagar <khushalsagar@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1313372}
This was generated by replacing " NOTREACHED()" with
" NOTREACHED_IN_MIGRATION()" and running git cl format.
This prepares for making NOTREACHED() [[noreturn]] alongside
NotReachedIsFatal migration of existing inventory.
Bug: 40580068
Change-Id: I3b48b89911ac5e9ffcb211622992f917f8f9e8d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5539619
Auto-Submit: Peter Boström <pbos@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Owners-Override: Lei Zhang <thestig@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1301096}
The canonical bug format is TODO(crbug.com/<id>). TODOs of the
following forms will all be migrated to the new format:
- TODO(crbug.com/<old id>)
- TODO(https://crbug.com/<old id>)
- TODO(crbug/<old id>)
- TODO(crbug/monorail/<old id>)
- TODO(<old id>)
- TODO(issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/u/1/issues/<old id>)
- TODO(bugs.chromium.org/<old id>)
Bug id mapping is sourced from go/chrome-on-buganizer-prod-issues.
See go/crbug-todo-migration for details.
#crbug-todo-migration
Bug: b/321899722
Change-Id: Ibc66b8c440e4bcdef414e77fef4d9874d2ea9951
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5493800
Auto-Submit: Alison Gale <agale@chromium.org>
Commit-Queue: Alison Gale <agale@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Owners-Override: Alison Gale <agale@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1293330}
