Compile and run var e:XML = <x><c/></x>; e.@* = 1; And watch the crash. Recent injection from String/XML changes.
Created attachment 408121 [details] [diff] [review] Patch the issue was that a call to internString() was added to E4XNode::setQName, without a null check. trivial fix. remainder of patch is adding isAnyName() calls to remove asserts [doesn't change behavior], and some driveby whitespace fixes
Attachment #408121 - Flags: review?(edwsmith)
What is the expected output of this? Looks like older avmshell would not fail with the above code, but would fail as soon as you tried to trace the XML. This is the current output with the patch if you trace the XML: <x (null)="1" xmlns="null"> <c/> </x> Is it correct to add a null attribute to /x/ when it did not previously exist?
Comment on attachment 408121 [details] [diff] [review] Patch you can compare with spidermonkey if interpretation of the E4X spec is in question.
pushed as changeset: 2917:d2de58311745
This is what I get running the code in firefox: --> var e = <x><c/></x>; --> e <x><c/></x> --> e.@* = 1; 1 --> e <x><c/></x> Just to test I added an attribute and it looks like they do not handle wildcard replacement at all. --> var e = <x bar="d"><c/></x>; --> e <x bar="d"><c/></x> --> e.@* = 1; 1 --> e <x bar="d"><c/></x> --> e.@bar = 1; 1 --> e <x bar="1"><c/></x>
A better question is what you get in FP10, since that's the behavior we want
Technically, this bug is fixed (no longer crashes) but we may need to confirm the behavior is identical to FP10 before closing.
Assertion in debug builds running the sample code: Assertion failed: "((an->getQName(&nam, publicNS)))" ("c:/buildbot/tamarin-redux /windows/tamarin-redux/core/XMLObject.cpp":1277)avmplus crash: exception 0x80000 003 occurred Writing minidump crash log to avmplusCrash.dmp
Created attachment 409539 [details] [diff] [review] Patch #2 Fixes the assert/crash and matches firefox output.
Comment on attachment 409539 [details] [diff] [review] Patch #2 I am not 100% sure where in the patch this is happening but when doing a wildcard attribute assignment on the XML when there are no attributes, the node is modified so that when you get the node it now contains a space. Example: var e:XML = <x><c/></x>; e.@* = 1; This will trace out the var e with "<x >" instead of "<x>", this makes e != to the original value, which should not have been modified since there were no attributes to modify
pushed as changeset changeset: 2964:8b93b0b96806 leaving bug open pending investigation of the inserted space
Trying to understand the intent of the spec with regards to wildcard attribute assignment... Spidermonkey does not handle wildcard assignment at all, attributes are never modified. TC handles wildcard assignment of single attribute. <x foo="0">; x.@*=1; --> <x foo="1"> TC does not handle multiple attributes on a node, only maintains first attribute, other attributes on node are lost. <x foo="0" bar="0">; x.@*=1; --> <x foo="1">
since it was totally broken before, current behavior is merely suboptimal. still would be nice to get it 100% right.
Created attachment 409728 [details] [diff] [review] testcase I would assume that all of the tests in this patch should function properly
Attachment #409728 - Flags: review?(stejohns) → review+
Created attachment 410656 [details] [diff] [review] Fix for rogue space insertion Fixes the issue with toString being different afterwards.
Attachment #410656 - Flags: review?(wsharp)
Looks like there is still an assertion happening in debug builds: $ $AVM e4x/Regress/regress-524214.abc Assertion failed: "((!isAnyName() && !isRtname()))" ("c:\\hg\\clean\\core\\Multiname.h":82)avmplus crash: exception 0x80000003 occurred
Created attachment 410802 [details] [diff] [review] Updated testcase update testcase to match now expected behavior
Attachment #409728 - Attachment is obsolete: true
oops, I neglected to test with the new testcase. Fix looks simple, I'll slipstream it in with the testcase.
And the plan is to match the incorrect spidermonkey handling of multiple attributes on a node? <x foo="0" bar="0">; x.@*=1; Produces --> <x foo="1"> and NOT Produces --> <x foo="1" bar="1">
(In reply to comment #19) > And the plan is to match the incorrect spidermonkey handling of multiple > attributes on a node? > > > <x foo="0" bar="0">; > x.@*=1; > Produces --> <x foo="1"> > and NOT > Produces --> <x foo="1" bar="1"> Nope, wasn't part of the plan. This bug is "fix the crash", not "make it match spidermonkey" -- if the latter is desirable, IMHO we should open a new bug.
pushed to redux as changeset: 3016:5ec7389cf24e declaring this fixed -- if we want to match (incorrect!) spidermonkey output let's open a new bug.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.