meta within head but after a kilobyte triggers a reload
Categories
(Core :: DOM: HTML Parser, defect, P3)
Tracking
()
People
(Reporter: karlcow, Assigned: hsivonen)
References
(Regressed 1 open bug, )
Details
Attachments
(2 files)
So not sure this is a bug, but that's an interesting sequence of events.
Steps to reproduce
- Visit
https://us.etrade.com/e/t/user/login
- Type random stuff in to the User ID and password fields and submit the form
Actual:
Blink browsers and Firefox receive a different error message.
Expected:
Same error message for firefox and blink browsers
This happens because on Firefox the page is loaded twice after it initiated an XHR HTTP POST for the login. The reason for the reload is not triggered by the site itself but by the pre-parsing to find the html charset declaration: <meta charset="utf-8">
The page was reloaded, because the character encoding declaration of the HTML document was not found when prescanning the first 1024 bytes of the file. The encoding declaration needs to be moved to be within the first 1024 bytes of the file.
I wonder if it could have unintended consequences in certain scenarios. (no idea).
The initial regression range in the webcompat report was
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ca257801d86f5d4e9c1768722c7131216c07d38f&tochange=4be2a981c307898ac2858db37872044a962d922c
And we can't get a better one because we get into a series of
2:49.27 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ca257801d86f5d4e9c1768722c7131216c07d38f&tochange=4be2a981c307898ac2858db37872044a962d922c
2:49.27 INFO: Switching bisection method to taskcluster
2:49.27 INFO: Getting mozilla-central builds between ca257801d86f5d4e9c1768722c7131216c07d38f and 4be2a981c307898ac2858db37872044a962d922c
2:51.09 WARNING: Skipping build ca257801d86f: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.shippable.revision.ca257801d86f5d4e9c1768722c7131216c07d38f.firefox.macosx64-opt'
2:51.11 WARNING: Skipping build 4be2a981c307: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.shippable.revision.4be2a981c307898ac2858db37872044a962d922c.firefox.macosx64-opt'
2:52.16 WARNING: Skipping build ca5c8cdce0c9: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.shippable.revision.ca5c8cdce0c9af1ccc1afe0a0cec0a5ed97d4b78.firefox.macosx64-opt'
2:52.17 WARNING: Skipping build d23a0a0ffa93: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.shippable.revision.d23a0a0ffa93fe273c571592bdbff6aa68f6ca2b.firefox.macosx64-opt'
2:52.19 WARNING: Skipping build 72a8d8c20180: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.shippable.revision.72a8d8c20180a068fd37f0bbf4619963486b0755.firefox.macosx64-opt'
β¦
3:01.30 WARNING: Skipping build 7dd1ac96a354: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.shippable.revision.7dd1ac96a354295013e0c78eccf7e880ae645b3f.firefox.macosx64-opt'
3:01.30 CRITICAL: Last build 4be2a981c307 is missing, but mozregression can't find a build after - so it is excluded, but it could contain the regression!
3:01.91 WARNING: Skipping build b19e0c207cfd: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.shippable.revision.b19e0c207cfd521f933583ca1293d62c34ead546.firefox.macosx64-opt'
3:01.93 WARNING: Skipping build e545376c3391: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.shippable.revision.e545376c33914c25a9252a74e09bd2b5b3e8ed76.firefox.macosx64-opt'
3:01.94 WARNING: Skipping build de4a32bbbe50: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.shippable.revision.de4a32bbbe50d2a21e38eab7a4042378cc435aa8.firefox.macosx64-opt'
3:01.94 INFO: There are no build artifacts for these changesets (they are probably too old).
![]() |
Reporter | |
Updated•4 years ago
|
![]() |
Reporter | |
Comment 1•4 years ago
|
||
In the regression range, we can find Bug 1546783 which was touching the speculative parsing algorithm.
But maybe it's not necessary related, just like it could be.
Comment 2•4 years ago
|
||
I'd be extremely surprised if that was the culprit, because that should only make us scan inside <style>
elements, there's no <style>
or anything before the meta, and the patch shouldn't really change anything else...
I manually backed out that change (as it was pretty simple) and the bug still reproduces. There are a couple networking changes that could potentially cause that? Henri, do you know?
I guess worst-case we could manually bisect.
Assignee | ||
Comment 3•4 years ago
|
||
Retitling to make it clear that the POST is from XHR and we're not reloading a POST navigation response.
Assignee | ||
Comment 4•4 years ago
|
||
This shouldn't be a recent Gecko-side regression; we've had the reload behavior approximately forever (strictly at one kilobyte since Firefox 4). The site is a classic case of having excessive IE conditional comments that push <meta charset="utf-8">
beyond the first kilobyte. Having an XHR POST in the mix is unfortunate.
I suggest contacting E*TRADE and asking them to move <meta charset="utf-8">
after the <head>
start tag and before the <title>
start tag.
The relevant conformance requirement is in the second bullet point in the list under https://html.spec.whatwg.org/#charset
Assignee | ||
Comment 5•4 years ago
|
||
(Longer term, as a more complicated project, we probably have to concede that since other browsers aren't following the spec, we need to start doing what they are doing about the meta
prescan: prescanning until the start of <body>
or the first 1024 bytes, whichever is larger.)
![]() |
Reporter | |
Comment 6•4 years ago
|
||
Let's check if Google would be ready to change their implementation too.
And agreed for contacting E*Trade.
Assignee | ||
Comment 7•4 years ago
|
||
(In reply to Karl Dubostπ‘ :karlcow from comment #6)
Let's check if Google would be ready to change their implementation too.
All indications so far have been that they aren't. :-(
![]() |
Reporter | |
Comment 8•4 years ago
|
||
So currently their implementation is a bit more complex than just go to </head>
. It's a mix of 1024 rule and then the strategy after which is according to the code and comment:
// We still don't have an encoding, and are in the head. The following tags
// are allowed in <head>: SCRIPT|STYLE|META|LINK|OBJECT|TITLE|BASE
// We stop scanning when a tag that is not permitted in <head> is seen, rather
// when </head> is seen, because that more closely matches behavior in other
// browsers; more details in <http://bugs.webkit.org/show_bug.cgi?id=3590>.
// Additionally, we ignore things that looks like tags in <title>, <script>
// and <noscript>; see:
// <http://bugs.webkit.org/show_bug.cgi?id=4560>
// <http://bugs.webkit.org/show_bug.cgi?id=12165>
// <http://bugs.webkit.org/show_bug.cgi?id=12389>
// Since many sites have charset declarations after <body> or other tags that
// are disallowed in <head>, we don't bail out until we've checked at least
// bytesToCheckUnconditionally bytes of input.
![]() |
Reporter | |
Comment 9•4 years ago
|
||
First attempt at contact done.
https://webcompat.com/issues/68512#issuecomment-811585677
Assignee | ||
Comment 10•4 years ago
|
||
I suggest the following Gecko change to match WebKit/Blink semantics (except the timing and buffer boundary dependency) with the minimum and most performant Gecko changes:
When the first 1024 bytes have been sniffed, if we have neither a meta
nor a bogo-XML declaration, i.e. we fall back to chardetng, make a note whether the stream started with <?xml
and activate the current file:
-only machinery for buffering bytes and preventing tree ops from being flushed to the main thread. Make the meta scanner look at comments, meta
, and the start of body
(from the tree builder, not from the tokenizer).
If the stream started with <?xml
and the first comment looks like a bogo-XML declaration that disagrees with chardetng, note the disagreeing encoding.
If there is a encoding meta
that agrees with chardetng, stop remembering bytes and stop preventing the treeops from being flushed. If there's an encoding meta
that disagrees with chardetng, discard the tree ops and rewind the stream as in the file:
case and start over with the declared encoding.
If body
starts and we're still remembering bytes and preventing tree ops from being flushed (i.e. we haven't seen an encoding meta
): If an encoding from a bogo-XML declaration was noted, discard the tree ops and rewind the stream and start over with that encoding. Otherwise, stop remembering bytes and stop preventing the tree ops from being flushed.
annevk, how does this sound? (This would allow for the removal of the 1024-byte limit from the bogo-XML decl spec PR, since this would as-if arbitrary-length bogo-XML declarations.)
Assignee | ||
Comment 11•4 years ago
|
||
The change described in the previous comment would be messy to implement without implementing bug 1687635 first.
Comment 12•4 years ago
|
||
What does "note the disagreeing encoding" mean in the <?xml
case? What would this mean for the 1024-byte limit on <meta>
? Overall I think this is reasonable though and probably the path of least resistance.
Assignee | ||
Comment 13•4 years ago
|
||
(In reply to Anne (:annevk) from comment #12)
What does "note the disagreeing encoding" mean in the
<?xml
case?
Storing it in an object field for use in the "If an encoding from a bogo-XML declaration was noted" steps.
What would this mean for the 1024-byte limit on
<meta>
?
It would mean that meta
within 1024 bytes would perform marginally better than meta
anywhere in head
without speculative load side effects, but meta
later in head
would work without a reload and without script side effects but with potential speculative load side effects. meta
after 1024 and after head
would still reload.
Comment 14•4 years ago
|
||
Henri: could you please prioritize this ticket and set the severity? It seems you've quite a clear understanding of it.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 15•3 years ago
|
||
Assignee | ||
Comment 16•3 years ago
|
||
This is known to break View Source and unlabeled ISO-2022-JP from file:
URLs, but let's see what else breaks:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=35b7af9eabe537f744cf027c834f232357a5f62c
Assignee | ||
Comment 17•3 years ago
|
||
Assignee | ||
Comment 18•3 years ago
|
||
Assignee | ||
Comment 19•3 years ago
|
||
Assignee | ||
Comment 20•3 years ago
|
||
Assignee | ||
Comment 21•3 years ago
|
||
Assignee | ||
Comment 22•3 years ago
|
||
Assignee | ||
Comment 23•3 years ago
|
||
Assignee | ||
Comment 24•3 years ago
|
||
Assignee | ||
Comment 25•3 years ago
|
||
Assignee | ||
Comment 26•3 years ago
|
||
Assignee | ||
Comment 27•3 years ago
|
||
Assignee | ||
Comment 28•3 years ago
|
||
Assignee | ||
Comment 29•3 years ago
|
||
Assignee | ||
Comment 30•3 years ago
|
||
Assignee | ||
Comment 31•3 years ago
|
||
Assignee | ||
Comment 32•3 years ago
|
||
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 33•3 years ago
|
||
Assignee | ||
Comment 34•3 years ago
|
||
Now it's possible that the code might be done. More testing and more tests still needed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=916ceaa5b3ef880667981e62d49d565103befcc7
Assignee | ||
Comment 36•3 years ago
|
||
XML View Source needed tweaking:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b3cf8cac5d9e4857229a887ce1614042e5955c1
I still need to write more tests, but now this looks almost ready.
Assignee | ||
Comment 37•3 years ago
|
||
Assignee | ||
Comment 38•3 years ago
|
||
Rebased after bug 1651513:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c939f0b3f10533b3f103a9a0fb63d4d24befb703
Assignee | ||
Comment 39•3 years ago
|
||
Need to look at
REFTEST TEST-UNEXPECTED-FAIL | parser/htmlparser/tests/reftest/view-source:vs-after-head-after-1kb.html == parser/htmlparser/tests/reftest/vs-after-head-after-1kb-ref.html | image comparison, max difference: 255, number of differing pixels: 18
Assignee | ||
Comment 40•3 years ago
|
||
Need to look at
docshell/test/browser/browser_bug92473.js
browser/base/content/test/general/browser_bug477014.js
Assignee | ||
Comment 41•3 years ago
|
||
Assignee | ||
Comment 42•3 years ago
|
||
A bunch of tests starting from devtools/client/debugger/test/mochitest/browser_dbg-breaking.js
are broken.
Assignee | ||
Comment 43•3 years ago
|
||
Assignee | ||
Comment 44•3 years ago
|
||
Assignee | ||
Comment 45•3 years ago
|
||
Assignee | ||
Comment 46•3 years ago
|
||
Assignee | ||
Comment 47•3 years ago
|
||
This should fix dev tools:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f01bb716849d6a396ce7bb2958922e8401203e3
Assignee | ||
Comment 48•3 years ago
|
||
And this should fix dev tools in the case where EOF commits the parser to the speculated encoding inherited from the parent frame.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=865ca4df4754b2998d1206ee0001a19daec8d9fe
Assignee | ||
Comment 49•3 years ago
|
||
And still tweaking how data is reported to dev tools:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0e6bf8d18654103b9fc9ba5d55b7ecad19a6325
Assignee | ||
Comment 50•3 years ago
|
||
Assignee | ||
Comment 51•3 years ago
|
||
Assignee | ||
Comment 52•3 years ago
|
||
Assignee | ||
Comment 53•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Comment 54•3 years ago
|
||
Assignee | ||
Comment 55•3 years ago
|
||
New "shippable" builds after a rebase:
https://treeherder.mozilla.org/jobs?repo=try&revision=7be68f73402e14053ba20d896abc24ea8b6ea94a
Assignee | ||
Comment 56•3 years ago
|
||
New "shippable" builds after a rebase and after fixing HTML View Source for files that start with a BOM or with a UTF-16 XML declaration:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdcea49109a2f6347ff6acb3ebea7b515a675300
Assignee | ||
Comment 57•3 years ago
|
||
New "shippable" builds after a rebase with console messages rephrased (no other changes in behavior):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df1bf695a4d9a6646deb7440daabf8ca5ce3e71e
Assignee | ||
Comment 58•3 years ago
|
||
New "shippable" builds after a rebase (no other changes beyond rebase):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f85a77787d4df60ae9c5ed69fa168db69e9d2580
Assignee | ||
Comment 59•3 years ago
|
||
Retitling the bug to better capture the scope of what's being fixed.
Assignee | ||
Comment 60•3 years ago
|
||
Try run after addressing review comments:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7da5df95035588c3a8cc5e3de3b157ce45688d6
![]() |
Reporter | |
Updated•3 years ago
|
Assignee | ||
Comment 61•3 years ago
|
||
Retest after rebase (no changes other than rebase after last week's changes to address review comments, but this is the first test round after last week's change to address review comments):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc30f97514a29f5bf042c424b352eff265709182
Assignee | ||
Comment 62•3 years ago
|
||
Regenerating "shippable" build after rebase; no other changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7abc3fcb6d5a52a8138d74117a104b2eb0f4dfb3
Assignee | ||
Comment 63•3 years ago
|
||
Regenerating "shippable" build after rebase; no other changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40d615f81c4cc331cf2dac86ddee85f0910e7600
Assignee | ||
Comment 65•3 years ago
|
||
Rebased and added comments and one assertion, broader try run ahead of landing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bdf15208b0c03e0124e3c2eb27ed003b1eb8a2a
Assignee | ||
Comment 66•3 years ago
|
||
Test run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c564d4a391950f4935a76169b198170c7a2874e3
Comment 67•3 years ago
|
||
Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3dfd3c94a105 meta charset rewrite. r=smaug
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/31927 for changes under testing/web-platform/tests
Comment 69•3 years ago
|
||
Backed out for causing mochitest failures on browser_hsts_host.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/28315f772b05d2aa856071bb984ef2ca7cf49b78
Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=3dfd3c94a105e095aada0b356f1106370de222d3&selectedTaskRun=TB5-dW0QRr2kXafint4qJA.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=360367221&repo=autoland&lineNumber=37863
Failure line: TEST-UNEXPECTED-FAIL | dom/security/test/https-only/browser_hsts_host.js | Uncaught exception - undefined - timed out after 50 tries.
Upstream PR was closed without merging
Assignee | ||
Comment 71•3 years ago
|
||
(In reply to Norisz Fay [:noriszfay] from comment #69)
Backed out for causing mochitest failures on browser_hsts_host.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/28315f772b05d2aa856071bb984ef2ca7cf49b78
Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=3dfd3c94a105e095aada0b356f1106370de222d3&selectedTaskRun=TB5-dW0QRr2kXafint4qJA.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=360367221&repo=autoland&lineNumber=37863
Failure line: TEST-UNEXPECTED-FAIL | dom/security/test/https-only/browser_hsts_host.js | Uncaught exception - undefined - timed out after 50 tries.
lyavor, do you have a guess of why https://searchfox.org/mozilla-central/source/dom/security/test/https-only/browser_hsts_host.js might fail like this consistently when it didn't in the try run comment 66?
Assignee | ||
Comment 74•3 years ago
|
||
(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #73)
It also failed in the Try run.
Good point.
The subsequent failures are of the form
TEST-UNEXPECTED-FAIL | dom/security/test/https-only/browser_httpsonly_prefs.js | uncaught exception - ReferenceError: ok is not defined at onNewMessage@chrome://mochitests/content/browser/dom/security/test/https-only/browser_hsts_host.js:105:5
which indicates a failure to load parts of the test harness itself.
Assignee | ||
Comment 75•3 years ago
|
||
Let's try adding a HTTP level charset to the sjs that these tests load:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbdfa16e9549a324b94f0be64de467e26889b49b
Let's also try hard-coding about: pages to UTF-8 to avoid timing issues with about:newtab
:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0402a222b8d6029bf49fb239efca0bbf95c52981
Assignee | ||
Comment 76•3 years ago
|
||
Round two of sjs tweak:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60fd87f45be1b674b0727e653d128545a48d739d
Hard-code not only about:
but moz-extension:
, too:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4127f7e460f7cc2a450c6229887c03fe861cd04f
which indicates a failure to load parts of the test harness itself.
Or can it be that the test harness definitions go away before the method invocation in the callback is checked for what it refers to?
Assignee | ||
Comment 77•3 years ago
|
||
onNewMessage@chrome://mochitests/content/browser/dom/security/test/https-only/browser_hsts_host.js:105:5
which indicates a failure to load parts of the test harness itself.
Or even more likely, perhaps this is about failing to remove the listener in the first failing test?
Assignee | ||
Comment 78•3 years ago
|
||
With more logging:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6cd34aae96548bba265e3c8c26942addd475211
Assignee | ||
Comment 79•3 years ago
|
||
Looks like this patch changes a logged message from 'Upgrading insecure request' to 'Upgrading insecure speculative TCP connection', which doesn't match the test expectation.
Assignee | ||
Comment 80•3 years ago
|
||
Assignee | ||
Comment 81•3 years ago
|
||
Assignee | ||
Comment 82•3 years ago
|
||
Comment 85•3 years ago
|
||
Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9a8abd87cc79 meta charset rewrite. r=smaug https://hg.mozilla.org/integration/autoland/rev/e135fe933c4e addendum - Make browser_hsts_host.js accept speculative connection in place of speculative request. r=necko-reviewers,valentin
Assignee | ||
Comment 86•3 years ago
|
||
Note to sheriff: The try run in comment 83 shows that the addendum addresses the failure that this was backed out over in comment 69. When assessing the TV failure with that test, it's worth noting that bug 1735683 was already on file.
Assignee | ||
Comment 87•3 years ago
|
||
Also, a bunch of other TV failures seem to be https://bugzilla.mozilla.org/show_bug.cgi?id=1726575
Assignee | ||
Comment 88•3 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #87)
Also, a bunch of other TV failures seem to be https://bugzilla.mozilla.org/show_bug.cgi?id=1726575
That one was resolved by a backout, but this seems like the same kind of harness unreliability with view-source:
reftests.
Comment 90•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9a8abd87cc79
https://hg.mozilla.org/mozilla-central/rev/e135fe933c4e
Updated•3 years ago
|
Comment 91•3 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #79)
Looks like this patch changes a logged message from 'Upgrading insecure request' to 'Upgrading insecure speculative TCP connection', which doesn't match the test expectation.
Correct,
I am sorry for the late reply.
"Upgrading insecure speculative TCP connection" didn't exist back then.
Upstream PR merged by moz-wptsync-bot
Updated•3 years ago
|
Updated•3 years ago
|
Description
•