Closed Bug 1716290 Opened 3 years ago Closed 3 years ago

Remove protections against the document changing as part of kCharsetFromFinalUserForcedAutoDetection reload

Categories

(Core :: DOM: HTML Parser, task)

task

Tracking

()

RESOLVED FIXED
93 Branch
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.

Severity: -- → N/A
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
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

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.

(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

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

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.

Flags: needinfo?(hsivonen)

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.

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

Flags: needinfo?(hsivonen)

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.

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.

Flags: needinfo?(emilio)

(In reply to Henri Sivonen (:hsivonen) from comment #14)

I'll also file a follow-up.

Filed bug 1727750.

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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 93 Branch
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: