Closed Bug 1701828 Opened 4 years ago Closed 3 years ago

meta within head but after a kilobyte triggers a reload

Categories

(Core :: DOM: HTML Parser, defect, P3)

Product:
Core
Component:
DOM: HTML Parser
Version:
Firefox 89
Platform:
Unspecified
Windows 10
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
97 Branch
Project Flags:
Webcompat Priority P2
Tracking Flags:
Tracking Status
firefox89 --- wontfix
firefox97 --- fixed

People

(Reporter: karlcow, Assigned: hsivonen)

References

(Regressed 1 open bug,
URL
)

Details

Attachments

(2 files)

Bug 1701828 - meta charset rewrite.
(deleted), text/x-phabricator-request
Details
Bug 1701828 addendum - Make browser_hsts_host.js accept speculative connection in place of speculative request.
(deleted), text/x-phabricator-request
Details

So not sure this is a bug, but that's an interesting sequence of events.

Steps to reproduce

  1. Visit https://us.etrade.com/e/t/user/login
  2. 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).
Webcompat Priority: --- → ?

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.

Flags: needinfo?(emilio)

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.

Flags: needinfo?(emilio) → needinfo?(hsivonen)

Retitling to make it clear that the POST is from XHR and we're not reloading a POST navigation response.

Summary: meta charset not found triggers reload after an HTTP POST → meta charset not found triggers reload after an XHR HTTP POST

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

Flags: needinfo?(hsivonen)

(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.)

Let's check if Google would be ready to change their implementation too.
And agreed for contacting E*Trade.

(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. :-(

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.

https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/html/parser/html_meta_charset_parser.cc;l=58-84

First attempt at contact done.
https://webcompat.com/issues/68512#issuecomment-811585677

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.)

Flags: needinfo?(annevk)

The change described in the previous comment would be messy to implement without implementing bug 1687635 first.

Depends on: 1687635

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.

Flags: needinfo?(annevk)

(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.

Henri: could you please prioritize this ticket and set the severity? It seems you've quite a clear understanding of it.

Flags: needinfo?(hsivonen)
Severity: -- → S3
Flags: needinfo?(hsivonen)
Priority: -- → P3
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attached file Bug 1701828 - meta charset rewrite. (deleted) —

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

Attachment #9241532 - Attachment description: Bug 1701828 - meta charset rewrite → Bug 1701828 - meta charset rewrite.
Attachment #9241532 - Attachment description: Bug 1701828 - meta charset rewrite. → WIP: Bug 1701828 - meta charset rewrite.

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

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.

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

Need to look at
docshell/test/browser/browser_bug92473.js
browser/base/content/test/general/browser_bug477014.js

A bunch of tests starting from devtools/client/debugger/test/mochitest/browser_dbg-breaking.js are broken.

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

And still tweaking how data is reported to dev tools:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0e6bf8d18654103b9fc9ba5d55b7ecad19a6325

Attachment #9241532 - Attachment description: WIP: Bug 1701828 - meta charset rewrite. → Bug 1701828 - meta charset rewrite.

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

New "shippable" builds after a rebase with console messages rephrased (no other changes in behavior):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df1bf695a4d9a6646deb7440daabf8ca5ce3e71e

New "shippable" builds after a rebase (no other changes beyond rebase):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f85a77787d4df60ae9c5ed69fa168db69e9d2580

Retitling the bug to better capture the scope of what's being fixed.

Summary: meta charset not found triggers reload after an XHR HTTP POST → meta within head but after a kilobyte triggers a reload

Try run after addressing review comments:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7da5df95035588c3a8cc5e3de3b157ce45688d6

Webcompat Priority: ? → P2

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

Depends on: 1741219

Regenerating "shippable" build after rebase; no other changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7abc3fcb6d5a52a8138d74117a104b2eb0f4dfb3

Regenerating "shippable" build after rebase; no other changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40d615f81c4cc331cf2dac86ddee85f0910e7600

Rebased and added comments and one assertion, broader try run ahead of landing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bdf15208b0c03e0124e3c2eb27ed003b1eb8a2a

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

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.

Flags: needinfo?(hsivonen)
Upstream PR was closed without merging

(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?

Flags: needinfo?(lyavor)

Bug 1735683 is a previous intermittent.

(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.

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

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?

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?

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.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=930033ce72bc53858e136f39acfc88739a94b9e6

Flags: needinfo?(lyavor)
Flags: needinfo?(hsivonen)
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

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.

Also, a bunch of other TV failures seem to be https://bugzilla.mozilla.org/show_bug.cgi?id=1726575

(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.

Bug 1725924 looks like the harness bug.

https://hg.mozilla.org/mozilla-central/rev/9a8abd87cc79
https://hg.mozilla.org/mozilla-central/rev/e135fe933c4e

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox97: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch
Regressions: 1745094
Depends on: 1745142
Regressions: 1745142

(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
status-firefox89: affected → wontfix
Flags: in-testsuite+
Regressions: 1744715
Component: DOM: Core & HTML → DOM: HTML Parser
No longer depends on: 1745142
Regressions: 1759738
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: