Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate iframe & object resource timing code paths #38348

Closed
wants to merge 1 commit into from

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Feb 3, 2023

So far some of the logic in resource timing for subframe navigations
iframe/object/embed) was duplicated, e.g. both in blink and in content.

This has led to race conditions, inconsistencies and sometimes
XSS leaks.

This patch attempts to improve the situation by consolidating the code
paths:

  • NavigationRequest receives is_container_initiated, which ensures only
    container-initiated navigations are reported to the parent. This
    is a clarification of something that was ambiguous in the spec
    previously (iframe resource timing: entry should only be created for frame-initiated navigation whatwg/html#8846).
    It later uses ParentResourceTimingAccess to decide if a navigation
    should report to its parent with/without response details
    (status code and mime-type), or not report at all (TAO-fail, not
    an iframe, not container-initiated).

  • Both object fallbacks and cancelled navigations (204/205) report
    to the parent via RenderFrameImpl, and blink converts that to a
    ResourceTimingInfo object. This allows us to remove the duplicated
    resource timing creation code in //content.

  • We report fallback resource timing also for plugin error events and
    not only for load events.

Bug: 1399862
Bug: 1410705
Change-Id: Id37d23cd02eee9e38f812e6f3da99caedafdee3d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4214695
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Noam Rosenthal <nrosenthal@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1110433}

Revert "Consolidate iframe & object resource timing code paths"

This reverts commit 5dcb6f7b01d5f51144a9ba847c34bb0cdc344ccb.

Reason for revert: MSan failures crbug.com/1420057
Change-Id: Id37d23cd02eee9e38f812e6f3da99caedafdee3d

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-4214695 branch 12 times, most recently from 75a4ec7 to 6822cb3 Compare February 8, 2023 17:30
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-4214695 branch 13 times, most recently from d1b0a5e to 623e08f Compare February 21, 2023 18:06
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-4214695 branch 6 times, most recently from 88101ec to 3a16269 Compare February 27, 2023 10:46
So far some of the logic  in resource timing for subframe navigations
iframe/object/embed) was duplicated, e.g. both in blink and in content.

This has led to race conditions, inconsistencies and sometimes
XSS leaks.

This patch attempts to improve the situation by consolidating the code
paths:

- NavigationRequest receives is_container_initiated, which ensures only
  container-initiated navigations are reported to the parent. This
  is a clarification of something that was ambiguous in the spec
  previously (whatwg/html#8846).
  It later uses ParentResourceTimingAccess to decide if a navigation
  should report to its parent with/without response details
  (status code and mime-type), or not report at all (TAO-fail, not
  an iframe, not container-initiated).

- Both object fallbacks and cancelled navigations (204/205) report
  to the parent via RenderFrameImpl, and blink converts that to a
  ResourceTimingInfo object. This allows us to remove the duplicated
  resource timing creation code in //content.

- We report fallback resource timing also for plugin error events and
  not only for load events.

Bug: 1399862
Bug: 1410705
Change-Id: Id37d23cd02eee9e38f812e6f3da99caedafdee3d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4214695
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Noam Rosenthal <nrosenthal@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1110433}
@WeizhongX
Copy link
Contributor

WPT Command: python3 ./wpt run --channel=nightly --verify --verify-no-chaos-mode --verify-repeat-loop=0 --verify-repeat-restart=10 --github-checks-text-file=/home/test/artifacts/checkrun.md --affected base_head --log-mach-level=info --log-mach=- -y --no-pause --no-restart-on-unexpected --install-fonts --no-headless --verify-log-full --binary=/home/test/build/firefox/firefox firefox

Some affected tests had inconsistent (flaky) results:

Unstable results

Test Subtest Results Messages
/resource-timing/entries-for-object-frame-options-deny.html Test that object elements with X-Frame-Options: Deny produce resource timing entries FAIL: 8/10, PASS: 2/10 assert_equals: expected 1 but got 0

These may be pre-existing or new flakes. Please try to reproduce (see the above WPT command, though some flags may not be needed when running locally) and determine if your change introduced the flake. If you are unable to reproduce the problem, please tag @web-platform-tests/wpt-core-team in a comment for help.

@WeizhongX WeizhongX closed this Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants