Closed Bug 621464 Opened 11 years ago Closed 10 years ago

Inconsistency with E4X XML.replace

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: jruderman, Assigned: adrake)

References

Details

(Keywords: regression, testcase, Whiteboard: [softblocker][fixed-in-tracemonkey])

Attachments

(1 file, 1 obsolete file)

print(<x>AAA</x>.replace())

opt:   undefined         toXMLString: "<x>undefined</x>"
dbg:   AAAundefined      toXMLString: "<x>\n  AAA\n  undefined\n</x>"

The first [revision where dbg outputs AAAundefined] is:
changeset:   1d1fe1d1e626
user:        Luke Wagner
date:        Mon Dec 06 10:26:58 2010 -0800
summary:     Bug 609440, part 4 - make JSString::chars() fallible (r=waldo,dvander,igor,dwitte,njn)

(I guess this was the "fallibile-ize JSString use in ctypes" patch in bug 609440?)

I reduced this from a jsfunfuzz testcase Gary sent me.  The original testcase showed a fragile difference between interp and TM (rather than between opt and dbg).
This was introduced by the blob:

@@ -6333,17 +6364,25 @@ xml_replace(JSContext *cx, uintN argc, j
<snip>
-    if (argc == 0 || !js_IdValIsIndex(vp[2], &index)) {
+    bool haveIndex;
+    if (argc == 0) {
+        haveIndex = true;
+    } else {
+        if (!js_IdValIsIndex(cx, vp[2], &index, &haveIndex))
+            return JS_FALSE;
+    }
+
+    if (!haveIndex) {

Pre-patch, argc == 0 would take the branch. Post-patch, argc == 0 does not take the branch. No other changes are made beyond this point, and so the index value is read uninitialized later.
Attached patch Patch v0 (obsolete) — Splinter Review
Changes = true to = false to preserve pre-regression behavior.
Assignee: general → adrake
Status: NEW → ASSIGNED
Attached patch Patch v1Splinter Review
With regression test for test suite.
Attachment #499790 - Attachment is obsolete: true
Bah, thanks!  (And Merry Christmas)
Attachment #499791 - Flags: review?(lw)
Attachment #499791 - Flags: review?(lw) → review+
Nominating not really because this seems to be a definite blocker, but rather this helps fuzzers and a r+ patch is available.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Whiteboard: softblocker
http://hg.mozilla.org/tracemonkey/rev/e63f5adc4280
Whiteboard: softblocker → [softblocker][fixed-in-tracemonkey]
Duplicate of this bug: 627685
http://hg.mozilla.org/mozilla-central/rev/e63f5adc4280
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.