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

HTML: Add tentative tests for speculative HTML parsing #24521

Merged
merged 27 commits into from
Sep 14, 2021
Merged

HTML: Add tentative tests for speculative HTML parsing #24521

merged 27 commits into from
Sep 14, 2021

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Jul 8, 2020

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 July 8, 2020 22:46 Inactive
@zcorpan zcorpan mentioned this pull request Jul 8, 2020
Closed
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 September 2, 2020 12:28 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 September 3, 2020 08:20 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 September 3, 2020 09:02 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 September 3, 2020 09:38 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 September 3, 2020 09:54 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 September 3, 2020 11:40 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 September 3, 2020 12:33 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 September 3, 2020 19:35 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 September 3, 2020 20:24 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 September 4, 2020 08:09 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 September 4, 2020 10:04 Inactive
@zcorpan
Copy link
Member Author

zcorpan commented Sep 4, 2020

I'm done with adding new tests for now. If someone is interested in reviewing, I believe this is in a reviewable state (though there's no spec proposal yet).

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 October 13, 2020 12:47 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 October 13, 2020 13:09 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-24521 October 26, 2020 19:53 Inactive
@zcorpan
Copy link
Member Author

zcorpan commented Sep 8, 2021

Rebased again to resolve conflicts.

The wpt-firefox-nightly-stability check now failed for test img-srcset

Test Subtest Results Messages
/html/syntax/speculative-parsing/generated/page-load/img-srcset.tentative.html Speculative parsing, page load: img-srcset FAIL: 1/10, PASS: 9/10 Unhandled rejection: assert_not_equals: speculative case did not fetch got disallowed value ""

I could reproduce this locally by reloading the test a couple of times in Firefox Nightly. @hsivonen is this a known issue? Or is 1500ms for the blocking script insufficient for stable test results?

@zcorpan
Copy link
Member Author

zcorpan commented Sep 8, 2021 •
edited
Loading

Increasing the time to 3000ms doesn't fix the flakiness when testing locally. I can submit a bug for Gecko. Edit: https://bugzilla.mozilla.org/show_bug.cgi?id=1729702

@hsivonen
Copy link
Member

hsivonen commented Sep 8, 2021

Not known. I'll try to catch this is rr. Thanks.

@mfreed7
Copy link
Contributor

mfreed7 commented Sep 10, 2021

Sorry for the delay re-reviewing the speculative parser stuff! These tests LGTM.

So Chromium has about 15 tests that fail and would need attention.

This one (meta-charset-script-src.tentative.html) passes for document.write() for all browsers, but fails page load on all browsers. Any clues there?

@zcorpan
Copy link
Member Author

zcorpan commented Sep 12, 2021 •
edited
Loading

Thanks, @mfreed7 !

This one (meta-charset-script-src.tentative.html) passes for document.write() for all browsers, but fails page load on all browsers. Any clues there?

Good catch.

So Firefox fails with this message:

Unhandled rejection: assert_not_equals: speculative case did not fetch got disallowed value ""

Chrome and Safari:

Unhandled rejection: assert_equals: expected param-encodingcheck values to match between speculative and non-speculative expected "%26%23286%3B" but got "%D0"

See this comment for how encodingcheck works: https://github.com/web-platform-tests/wpt/pull/24521/files#diff-4035c7e78333e9b93cce273d96a8235c5a4db3d498f4731ea225166187e5ec34R494-R504

So in Chrome and Safari, the URL character encoding for the script src fetch is windows-1254, and there's a <meta charset=windows-1254> before it. I think the charset scan looks further than 1024 bytes if you're still in <head>, right?

I don't recall why the test expects windows-1252, right now.

I'm not sure why Firefox doesn't fetch here. @hsivonen do you know?

@zcorpan
Copy link
Member Author

zcorpan commented Sep 13, 2021

@mfreed7 can you approve this PR with GitHub's review feature (in the Files Changed tab), please? Then we can merge. Thanks!

mfreed7
mfreed7 approved these changes Sep 13, 2021
View reviewed changes
Copy link
Contributor

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

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

LGTM!

@zcorpan zcorpan changed the title HTML: Add tentative tests for preload scanner HTML: Add tentative tests for speculative HTML parsing Sep 14, 2021
@zcorpan
Re-add 'Fix a mistake in document.write <meta charset> test'
fc10383 
The fix in 1481185 was accidentally reverted in the next commit c258849
@zcorpan
Copy link
Member Author

zcorpan commented Sep 14, 2021 •
edited
Loading

On the document-write/meta-charset test:

In https://github.com/whatwg/html/pull/5959/files#diff-41cf6794ba4200b839c53531555f0f3998df4cbb01a4d5cb0b94e3ca5e23947dR110885 it's specced that the speculative parser ignores the encoding declaration (because, changing the encoding can cause a re-navigation per spec). There was a bug in the test that I had fixed before but accidentally reverted, and now have fixed again (fc10383) -- the charset scanner shouldn't find the encoding declaration nested in a document.write().

This fix doesn't change the test results, though.

@zcorpan zcorpan merged commit 9afacb7 into master Sep 14, 2021
@zcorpan zcorpan deleted the bocoup/preload-scanner branch September 14, 2021 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfreed7 mfreed7 mfreed7 approved these changes

@annevk annevk Awaiting requested review from annevk

@domenic domenic Awaiting requested review from domenic

@foolip foolip Awaiting requested review from foolip

@jdm jdm Awaiting requested review from jdm

@jgraham jgraham Awaiting requested review from jgraham

@zqzhang zqzhang Awaiting requested review from zqzhang

@deniak deniak Awaiting requested review from deniak

Assignees

@jgraham jgraham

Labels
assets html infra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@zcorpan @hsivonen @mfreed7 @jgraham @wpt-pr-bot @moz-wptsync-bot