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.

Sign up for GitHub

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

Jump to bottom

REGRESSION(253037@main): [ iOS macOS ] http/tests/workers/service/basic-timeout.https.html is a constant timeout #3387

Merged
merged 1 commit into from Aug 17, 2022
Merged

REGRESSION(253037@main): [ iOS macOS ] http/tests/workers/service/basic-timeout.https.html is a constant timeout #3387

merged 1 commit into from Aug 17, 2022

Conversation

cdumez
Copy link
Contributor

@cdumez cdumez commented Aug 17, 2022 •
edited by webkit-commit-queue

a7d7335

REGRESSION(253037@main): [ iOS macOS ] http/tests/workers/service/basic-timeout.https.html is a constant timeout
https://bugs.webkit.org/show_bug.cgi?id=244001
<rdar://98739421>

Reviewed by Youenn Fablet.

After 253037@main, we no longer terminate service workers that do not respond in a timely
fashion to loads, unless those loads are navigations.

This test was testing the hung service worker termination logic using fetch requests
and would thus no longer behave as expected after 253037@main. To address the issue,
I rewrote the test to use iframe navigations instead of fetch requests.

* LayoutTests/http/tests/workers/service/basic-timeout.https-expected.txt:
* LayoutTests/http/tests/workers/service/basic-timeout.https.html:
* LayoutTests/http/tests/workers/service/resources/basic-timeout.js:
(async test.addTestFrame):
(async test.frameText):
(async test.testFrame4Loaded):
(async test.frameLoaded):
(async test):
(async test.finishThisTest): Deleted.
(async test.try): Deleted.
(async test.try.checkSuccessAgain): Deleted.
* LayoutTests/platform/glib/TestExpectations:
* LayoutTests/platform/ios/TestExpectations:
* LayoutTests/platform/mac-wk2/TestExpectations:

Canonical link: https://commits.webkit.org/253540@main

@cdumez cdumez self-assigned this Aug 17, 2022
@cdumez cdumez added New Bugs Unclassified bugs are placed in this component until the correct component can be determined. WebKit Nightly Build labels Aug 17, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 17, 2022
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Aug 17, 2022
youennf
youennf approved these changes Aug 17, 2022
View reviewed changes
LayoutTests/http/tests/workers/service/resources/basic-timeout.js Outdated
finishSWTest();
let iframe = frame.contentDocument.createElement("iframe");
iframe.src = url;
let iframeTimeoutHandle = setInterval(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

iframe and iframeTimeoutHandle could be const

LayoutTests/http/tests/workers/service/resources/basic-timeout.js
}, function(error) {
log(error);
});
frameText = function(testFrame)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why frameText = function(testFrame) instead of function frameText(testFrame)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs to have global scope otherwise I couldn't use it inside shouldBeEqualToString().

LayoutTests/http/tests/workers/service/resources/basic-timeout.js Outdated
frameText = function(testFrame)
{
let innerText = testFrame.contentDocument.body.innerText;
let newLineIndex = innerText.indexOf('\n');
Copy link
Contributor

Choose a reason for hiding this comment

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

could be const

@cdumez cdumez force-pushed the 244001_hung_service_worker_test branch from eb88421 to d488b07 Compare August 17, 2022 15:17
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 17, 2022
@cdumez cdumez added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Aug 17, 2022
@webkit-ews-buildbot webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels Aug 17, 2022
@cdumez cdumez added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Aug 17, 2022
@cdumez
REGRESSION(253037@main): [ iOS macOS ] http/tests/workers/service/bas…
a7d7335 
…ic-timeout.https.html is a constant timeout

https://bugs.webkit.org/show_bug.cgi?id=244001
<rdar://98739421>

Reviewed by Youenn Fablet.

After 253037@main, we no longer terminate service workers that do not respond in a timely
fashion to loads, unless those loads are navigations.

This test was testing the hung service worker termination logic using fetch requests
and would thus no longer behave as expected after 253037@main. To address the issue,
I rewrote the test to use iframe navigations instead of fetch requests.

* LayoutTests/http/tests/workers/service/basic-timeout.https-expected.txt:
* LayoutTests/http/tests/workers/service/basic-timeout.https.html:
* LayoutTests/http/tests/workers/service/resources/basic-timeout.js:
(async test.addTestFrame):
(async test.frameText):
(async test.testFrame4Loaded):
(async test.frameLoaded):
(async test):
(async test.finishThisTest): Deleted.
(async test.try): Deleted.
(async test.try.checkSuccessAgain): Deleted.
* LayoutTests/platform/glib/TestExpectations:
* LayoutTests/platform/ios/TestExpectations:
* LayoutTests/platform/mac-wk2/TestExpectations:

Canonical link: https://commits.webkit.org/253540@main
@webkit-commit-queue
Copy link
Collaborator

Committed 253540@main (a7d7335): https://commits.webkit.org/253540@main

Reviewed commits have been landed. Closing PR #3387 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit a7d7335 into WebKit:main Aug 17, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youennf youennf youennf approved these changes

@geoffreygaren geoffreygaren Awaiting requested review from geoffreygaren

@achristensen07 achristensen07 Awaiting requested review from achristensen07

Assignees

@cdumez cdumez

Labels
New Bugs Unclassified bugs are placed in this component until the correct component can be determined.
Projects
None yet
Milestone
No milestone
5 participants
@cdumez @webkit-commit-queue @youennf @webkit-early-warning-system @webkit-ews-buildbot