Skip to content

xml5ever: keep question marks in processing instruction data - #779

Open
FadeHack wants to merge 1 commit into
servo:mainfrom
FadeHack:xml5ever-pi-question-marks
Open

xml5ever: keep question marks in processing instruction data#779
FadeHack wants to merge 1 commit into
servo:mainfrom
FadeHack:xml5ever-pi-question-marks

Conversation

@FadeHack

Copy link
Copy Markdown

Fixes #774

The PI after state was throwing away the ? that got it there whenever that ? turned out not to be the start of a ?>. So this:

<?xml-stylesheet href="style.xsl?v=2"?>

came out of the tokenizer as href="style.xslv=2", pointing at the wrong resource, and nothing was reported. <?target a?b?c?> came out as abc.

XML 1.0 section 2.6 says a processing instruction runs up to the first ?>, so a ? anywhere else is ordinary data and belongs in the data string. libxml2 and Python's minidom both keep it.

What is in here

Three things, all in the same state:

  1. On ? we now push the pending ? into the data and stay in the PI after state, so a run of question marks is preserved except for the one that actually closes the instruction.
  2. On anything else we push the pending ? and the character, then go back to the PI data state. It used to push the character and stay in the PI after state, which meant the next > closed the instruction even though the character before it was not a ?. That made <?target a?b>c?> stop early with data ab. Going back to the PI data state is what the XML5 draft says to do here anyway.
  3. At EOF the pending ? was dropped for the same reason, so an unterminated instruction lost its last character. It is kept now.

About the skipped test

One xml5lib test, PI tag with char in PiAfter state, expects <?xml \t\n ?m?> to produce data m, so it disagrees with this change. It matches the XML5 draft, whose PI after state appends the ? when it sees another ? but silently drops it for anything else. That asymmetry reads like an oversight in the draft rather than something intended, and following it means the data of a processing instruction can silently differ from what every other XML parser produces.

Since the test data lives in a submodule I cannot change here, I skipped that one case with a comment pointing at the issue. Happy to take this a different way if you would rather the draft won, or if you would rather I send a patch to Ygg01/xml5lib-tests first and bump the submodule.

Testing

Added unit tests in xml5ever/src/tokenizer/mod.rs covering the href case from the issue, interior question marks, runs of question marks, the early > termination, empty data, and the unterminated case. All three fail without the change.

cargo test --all, cargo fmt --all -- --check and cargo clippy --all-features --all-targets are clean.

The PI after state was throwing away the '?' that got it there whenever
that '?' turned out not to be the start of a '?>'. So a processing
instruction like <?xml-stylesheet href="style.xsl?v=2"?> came out with
the href pointing at the wrong resource, and no error was raised.

XML 1.0 section 2.6 says a processing instruction runs up to the first
'?>', so a '?' anywhere else is ordinary data and belongs in the data
string. That is what libxml2 and Python's minidom do too.

The same state had a second problem. On any other character it pushed
that character and stayed in the PI after state, so the next '>' ended
the instruction even though the character before it was not a '?'. That
made <?target a?b>c?> stop early with data "ab". It now goes back to the
PI data state, which is what the XML5 draft says to do anyway.

At EOF the pending '?' was dropped for the same reason, so an
unterminated instruction lost its last character. It is kept now.

One xml5lib test, "PI tag with char in PiAfter state", expects the old
output. It follows the XML5 draft, whose PI after state appends the '?'
when it sees another '?' but silently drops it for anything else. That
asymmetry looks like an oversight in the draft rather than something
intended, so I skipped that test with a note instead of matching it.

Fixes servo#774
@github-actions github-actions Bot added the V-non-breaking A non-breaking change label Aug 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

V-non-breaking A non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

XML tokenizer drops question marks from processing-instruction data

1 participant