526662 - XMLParser needs to stop at NUL char

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

The FP9/FP10 version of XMLParser16 always stopped parsing when it found a null character (charcode = 0), regardless of string length. The current version does not. It needs to for bug compatibility, since some extant code relies on this behavior.

Comment 1

[Steven Johnson]

Attachment: Patch

Simple fix, with testcase.

Comment 2

[Brent Baker]

Comment on attachment 410406 [details] [diff] [review]
Patch

QE: testcase will need to be tweaked to use AddTestcase() so that it can be converted into and ATS test and copyright date should be 2009

Comment 3

[Steven Johnson]

Brent: want me to push the test as-is and let it be fixed after the fact?

Comment 4

[Brent Baker]

Yes, push now to get the testcase in a just leave the "in-testsuite ?" flag so that we can clean up the testcase

Comment 5

[Steven Johnson]

changeset:   2996:45cae11a4ebb

Comment 6

[Brent Baker]

Fixed the testcase to use the required AddTestCase() framework:

changeset:   3013:7ae09b930464

Comment 7

[Lars T Hansen]

If I understand this correctly it will need versioning in the future when we undo this compatibility fix, but the present bug restores the status quo and so does not need versioning, and it is required for FP10.1.

Comment 8

[Steven Johnson]

(In reply to comment #7)
> If I understand this correctly it will need versioning in the future when we
> undo this compatibility fix, but the present bug restores the status quo and so
> does not need versioning, and it is required for FP10.1.

Correct. (Though this is a bug that we could probably get by without ever "fixing"... the use case for parsing XML that contains NUL chars is apparently pretty low, since we've never supported it and apparently no one's ever complained...)

Comment 9

[Steven Johnson]

To be clear: the issue is that it's legal to can have a string that contains char 0, and it's (theoretically) legal to parse it as XML... but our parser has never allowed it, so there's no real reason to start now... IMHO we could leave it as-is forever, and not worry about versioning it. "Real" xml pretty much never uses char 0, this was only found in buggy code that relied on the bug!

Comment 10

[Dan Smith]

Moving to Future milestone.

Comment 11

[Steven Johnson]

I vote we just resolve it as fixed -- IMHO the need to parse past a null char is (apparently) nonexistent.

Comment 12

[Steven Johnson]

Ping -- I once again want to vote for not bothering with a fix (versioned or not); stopping on NUL is what we've always done and no one has ever requested otherwise. If I don't hear from someone claiming this as super-important for some reason, I'm marking this resolved/fixed.

Comment 13

[Lars T Hansen]

(In reply to comment #12)
> Ping -- I once again want to vote for not bothering with a fix (versioned or
> not); stopping on NUL is what we've always done and no one has ever requested
> otherwise. If I don't hear from someone claiming this as super-important for
> some reason, I'm marking this resolved/fixed.

It's probably not super-important, but software that has arcane and surprising exceptions to easily-remembered rules is harder to use than software that does not.  AS3 is already quite difficult to use by this measure and anything we can do to reduce the number of exceptions is good IMO.

(Whether introducing a versioned fix increases or decreases the number of exceptions from a simple rule is left as a conundrum for the reader to ponder.)

Comment 14

[Edwin Smith]

Since this bug is about fixing XML parser to be bug-compatible, fine to close it when fixed, but stopping at the NUL character is still a bug that should be fixed.  The XML and String specs are clear on this.  A separate bug should be fine, and it can be targeted/debated as we see fit.

Comment 15

[Steven Johnson]

Attachment: version-checked fix

SWF11+ no longer stops at a null char.

Comment 16

[Steven Johnson]

http://hg.mozilla.org/tamarin-redux/rev/f7356d657fc2

Comment 17

[Tamarin Bot]

changeset: 5878:b8787691a544
user:      Brent Baker <[e-mail]>
summary:   Bug 526662: vm_args file was misnamed and was not being picked up properly by runtests (r+brbaker)

http://hg.mozilla.org/tamarin-redux/rev/b8787691a544