Closed Bug 1910951 Opened 8 months ago Closed 7 months ago

nsParser::mIsAboutBlank is incorrectly set with meta refresh

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
131 Branch
Tracking Flags:
Tracking Status
firefox-esr115 131+ fixed
firefox-esr128 131+ fixed
firefox131 --- fixed

People

(Reporter: sefeng, Assigned: hsivonen)

References

Details

Attachments

(1 file)

Bug 1910951 - Avoid incorrect use of nsParser when meta refresh is added to about:blank.
48 bytes, text/x-phabricator-request
diannaS
: approval-mozilla-esr115+
diannaS
: approval-mozilla-esr128+
Details | Review

STR

  1. Create a blank.htm file with <html></html as the content
  2. Load this snippet with a debug build
<html>
  <iframe></iframe>
  <script>
    const iframe = document.querySelector("iframe");
    const meta = iframe.contentDocument.createElement('meta');
    meta.httpEquiv = 'Refresh';
    meta.content = '0; url=blank.htm';
    iframe.contentDocument.head.append(meta);
  </script>
</html>

Firefox would crash at https://searchfox.org/mozilla-central/rev/a9d6fb8ae147ebd1ff0d7886fd55c812d9b57378/parser/htmlparser/nsParser.cpp#996. It looks like it set mIsAboutBlank initially, but didn't update it once meta refresh is appended.

Blocks: 1906542
Severity: -- → S3

We are not supposed to parse non-about:blank HTML with nsParser, so we should not be updating mIsAboutBlank on nsParser but we should be creating a new nsHtml5Parser instead. Why does Refresh cause Necko activity on the nsParser instance instead of going through nsHTMLDocument::StartDocumentLoad again? Or does it go through nsHTMLDocument::StartDocumentLoad again, but the document URL somehow remains at about:blank instead of blank.htm so that we create a new parser instance but of the wrong type?

Flags: needinfo?(sefeng)

Or does it go through nsHTMLDocument::StartDocumentLoad again, but the document URL somehow remains at about:blank instead of blank.htm so that we create a new parser instance but of the wrong type?

Yeah, this is the case. The url remained at about:blank at https://searchfox.org/mozilla-central/rev/0c55d51c0d2a9b672e42ad40ea54f90267f92a8e/dom/html/nsHTMLDocument.cpp#343, so a new parse was created.

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

(In reply to Sean Feng [:sefeng] from comment #2)

The url remained at about:blank at https://searchfox.org/mozilla-central/rev/0c55d51c0d2a9b672e42ad40ea54f90267f92a8e/dom/html/nsHTMLDocument.cpp#343, so a new parse was created.

That's very bad. Is GetOriginalURI wrong and the fix is simply GetURI?

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

Perhaps we should also check that the channel QIs to nsIInputStreamChannel to believe it really is about:blank.

Ah, GetURI returned the correct file URL for me. Do you have cycles to fix this Henri? Let me know if you want me to put up a patch

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

Let's see if this breaks something else:
https://treeherder.mozilla.org/jobs?repo=try&revision=4c070064b3dbc8abd2806a418272825c156d11f3

Flags: needinfo?(hsivonen)
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/478c561808ee Avoid incorrect use of nsParser when meta refresh is added to about:blank. r=sefeng

https://hg.mozilla.org/mozilla-central/rev/478c561808ee

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
status-firefox131: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 131 Branch

:hsivonen can you nominate this for uplift since it is recommended in https://bugzilla.mozilla.org/show_bug.cgi?id=1753352#c105

Flags: needinfo?(hsivonen)

Comment on attachment 9418501 [details]
Bug 1910951 - Avoid incorrect use of nsParser when meta refresh is added to about:blank.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Bug 1753352 is nominated for uplift, because of a Web compat problem with about:blank. While we don't have in-the-wild Web compat reports about this one, this is another about:blank issue (theoretically could be combined with the other one) that has Web compat problem potential, and this one has a super-simple fix.
  • User impact if declined: A particular rather odd way of causing navigation away from the initial about:blank doesn't work, which may theoretically cause site breakage somewhere.
  • Fix Landed on Version: 131
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a low-risk change, because this should affect only a special case, and that special case has now been tested.
Flags: needinfo?(hsivonen)
Attachment #9418501 - Flags: approval-mozilla-esr115?

Comment on attachment 9418501 [details]
Bug 1910951 - Avoid incorrect use of nsParser when meta refresh is added to about:blank.

:hsivonen did you mean esr128?

Attachment #9418501 - Flags: approval-mozilla-esr115? → approval-mozilla-esr128?

(In reply to Dianna Smith [:diannaS] from comment #12)

:hsivonen did you mean esr128?

I saw esr115 on bug 1753352, so I used the same flag, but I think esr128 is OK.

oh youre right, it should be both actually

Attachment #9418501 - Flags: approval-mozilla-esr115?

Comment on attachment 9418501 [details]
Bug 1910951 - Avoid incorrect use of nsParser when meta refresh is added to about:blank.

Approved for 128.3esr

Attachment #9418501 - Flags: approval-mozilla-esr128? → approval-mozilla-esr128+

Comment on attachment 9418501 [details]
Bug 1910951 - Avoid incorrect use of nsParser when meta refresh is added to about:blank.

Approved for 115.16esr

Attachment #9418501 - Flags: approval-mozilla-esr115? → approval-mozilla-esr115+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: