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

This CL adds functionality that correctly handles network error of Subresource Web Bundles. #28617

Merged
merged 1 commit into from
Apr 23, 2021

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Apr 21, 2021

The problem is: when Web Bundle fetching fails due to a network error,
Subresource fetch doesn't fail forever.
One such case (subresource-loading-cors-error test) was
timing out previously but passes successfully with this change.

This CL also adds 2 WPT tests:

  1. subresource-loading-network-error.https.tentative.sub.html
  2. subresource-loading-web-bundle-fetch-failed.https.tentative.html

Test #1 is a scenario with a different network error than the CORS
one, but with the same issue of subresource fetching timing out
without the change. It passes successfully after the change.

Test #2 is a scenario with a Web bundle not found error, which is
not directly influenced by the code added in this CL, but it expands
the test coverage which was found to be lacking the error cases before.

Bug: 1168449

Change-Id: Ia3abb967e36274becc86e317bc51b1272d3ae679
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2826001
Reviewed-by: Tsuyoshi Horo <[email protected]>
Reviewed-by: Hayato Ito <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Commit-Queue: Miras Myrzakerey <[email protected]>
Cr-Commit-Position: refs/heads/master@{#875532}

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 changed the title Preliminary solution, WIP This CL handles the error case when we fail to fetch the Web Bundle due to network errors. Apr 21, 2021
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2826001 branch 2 times, most recently from 3f084be to ce28bb1 Compare April 22, 2021 07:37
@chromium-wpt-export-bot chromium-wpt-export-bot changed the title This CL handles the error case when we fail to fetch the Web Bundle due to network errors. This CL handles the timeout problem with Subresource fetching from a WebBundle. Apr 22, 2021
@chromium-wpt-export-bot chromium-wpt-export-bot changed the title This CL handles the timeout problem with Subresource fetching from a WebBundle. This CL adds functionality that correctly handles network error of Subresource Web Bundles. Apr 22, 2021
Subresource Web Bundles.

The problem is: when Web Bundle fetching fails due to a network error,
Subresource fetch doesn't fail forever.
One such case (subresource-loading-cors-error test) was
timing out previously but passes successfully with this change.

This CL also adds 2 WPT tests:
1. subresource-loading-network-error.https.tentative.sub.html
2. subresource-loading-web-bundle-fetch-failed.https.tentative.html

Test #1 is a scenario with a different network error than the CORS
one, but with the same issue of subresource fetching timing out
without the change. It passes successfully after the change.

Test #2 is a scenario with a Web bundle not found error, which is
not directly influenced by the code added in this CL, but it expands
the test coverage which was found to be lacking the error cases before.

Bug: 1168449

Change-Id: Ia3abb967e36274becc86e317bc51b1272d3ae679
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2826001
Reviewed-by: Tsuyoshi Horo <[email protected]>
Reviewed-by: Hayato Ito <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Commit-Queue: Miras Myrzakerey <[email protected]>
Cr-Commit-Position: refs/heads/master@{#875532}
@jpchase
Copy link

jpchase commented Apr 23, 2021

The checks seem to have failed due to #28209 (a generic Chrome crashing issue), not specific to the tests. The checks ran fine for Safari and Firefox. I'm going to force merge.

@jpchase jpchase merged commit 2f52688 into master Apr 23, 2021
@jpchase jpchase deleted the chromium-export-cl-2826001 branch April 23, 2021 17:10
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