Remove protections against the document changing as part of kCharsetFromFinalUserForcedAutoDetection reload
Categories
(Core :: DOM: HTML Parser, task)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox93 | --- | fixed |
People
(Reporter: hsivonen, Assigned: hsivonen)
References
Details
Attachments
(1 file)
The HTML parser has a bunch of complexity arising from taking into account the situation where the pre-reload document seen by the detector and the post-reload (with kCharsetFromFinalUserForcedAutoDetection) document differ such that after reload the document has changed to have an ASCII-incompatible encoding declaration. I that case, we try to honor the declaration instead of the override.
While this is theoretically prudent, the code could be simpler if we ignored this theoretical threat.
|
||
Updated•3 years ago
|
|
Assignee | |
Comment 1•3 years ago
|
||
|
|
Assignee | |
Updated•3 years ago
|
|
|
Assignee | |
Comment 2•3 years ago
|
||
|
|
Assignee | |
Comment 3•3 years ago
|
||
|
|
Assignee | |
Comment 4•3 years ago
|
||
|
|
Assignee | |
Comment 5•3 years ago
|
||
Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ab805f2926d5 Remove protections against the document changing as part of kCharsetFromFinalUserForcedAutoDetection reload. r=emk
|
||
Comment 7•3 years ago
•
|
||
Backed out for causing failures on ink-header-preload.html. CLOSED TREE
Link to backout : https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=c0bbd0090a1910bc80d34efb7b13551275930222
Push with failures : https://treeherder.mozilla.org/jobs?repo=autoland&searchStr=os%2Cx%2C10.15%2Cwebrender%2Copt%2Cweb%2Cplatform%2Ctests%2Ctest-macosx1015-64-qr%2Fopt-web-platform-tests-e10s%2Cwpt4&revision=ab805f2926d583986019186024907f5096935606
Link with failure log : https://treeherder.mozilla.org/logviewer?job_id=349626899&repo=autoland&lineNumber=7160
Added new link to failure log : https://treeherder.mozilla.org/logviewer?job_id=349622606&repo=autoland&lineNumber=1726
|
|
Assignee | |
Comment 8•3 years ago
|
||
Looks like a previously-known intermittent: bug 1690168. On surface, I don't see a reason why this patch would affect that test, but I'll re-check.
|
|
Assignee | |
Comment 9•3 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #8)
Looks like a previously-known intermittent: bug 1690168. On surface, I don't see a reason why this patch would affect that test, but I'll re-check.
I looked at this in Pernosco, and as suggested by the test case source, the test case doesn't seem to trigger what was changed here. Chances are that this is an unlucky timing thing and the test needs to be marked as unreliable.
Furthermore, Pernosco shows the link prefetch happening only once even though the log message suggests twice.
emilio, it appears that you wrote the test. Any ideas what's going on?
Here's the Pernosco session: https://pernos.co/debug/leZVYqPaeFGyIP5z5C59oQ/index.html
|
||
Comment 10•3 years ago
|
||
I'm on PTO today and I might not have time to dig into it (on the phone now), but my guess would be that this patch changes the charset that we load the link header and/or preload with, which might cause the <link> element not to hit the preloaded resource (and thus be loaded twice).
This would happen if this function returned something different after your patch for preloads vs. for the link load.
ni?ing you to check if this could be a reasonable explanation. If it is not I can dig tomorrow or Monday worst case.
|
|
||
Comment 11•3 years ago
|
||
The old intermittent was on verify and is sorta expected because after reload the image that hits the cache and will not be loaded again. So the test is expected to fail on TV.
|
|
Assignee | |
Comment 12•3 years ago
•
|
||
Thanks. I'll check out if Document::mCharacterSet differs during preloads before and after the patch.
In any case, the observation that preload-time encoding of the document object matters for preloads has rather significant implications for bug 1701828. :-(
|
|
Assignee | |
Comment 13•3 years ago
|
||
Indeed, it looks like we do a preload while Document::mCharacterSetSource == kCharsetUninitialized, which is an OK time to start the HTTP fetch but not an appropriate time to parse CSS.
|
|
Assignee | |
Comment 14•3 years ago
|
||
This seems to have revealed a bad performance blunder:
It looks like we invalidate preloads if the HTML page doesn't have its character encoding declared on the HTTP layer and the resulting encoding isn't windows-1252.
I'm going to proceed making it so that instead of windows-1252 succeeding, UTF-8 succeeds, because if the HTTP-level charset is missing, <meta charset=utf-8> is the common case.
I'll also file a follow-up.
|
|
Assignee | |
Comment 15•3 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #14)
I'll also file a follow-up.
Filed bug 1727750.
|
|
Assignee | |
Comment 16•3 years ago
|
||
|
||
Comment 17•3 years ago
|
||
Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f4419523fbf8 Remove protections against the document changing as part of kCharsetFromFinalUserForcedAutoDetection reload. r=emk,emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/30199 for changes under testing/web-platform/tests
|
||
Comment 19•3 years ago
|
||
| bugherder | ||
Upstream PR merged by moz-wptsync-bot
Description
•