TM: followup work for support String(v) -- String constructor called as a converter

VERIFIED FIXED in mozilla1.9.1

Status

()

defect
P1
major
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({verified1.9.1})

Trunk
mozilla1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

TraceRecorder::stringify is fallible but also returns null for non-primitive t to tell its caller to abort. We should fix this.

On IRC we discussed making it call the String constructor as a function to convert (with valueOf hint "string") but that helps the only caller who might pass a non-primitive, which is the String called as a function case just added for bug 482349.

Also, we should handle null => "null".

/be
Flags: blocking1.9.1?
Priority: -- → P1
Flags: blocking1.9.1? → blocking1.9.1+
Posted patch fix (obsolete) — Splinter Review
Attachment #366658 - Flags: review?(jwalden+bmo)
Comment on attachment 366658 [details] [diff] [review]
fix

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp

>-        /* We can't stringify objects here (use imacros instead), just return NULL. */
>-        return NULL;
>+        /* Callers must deal with non-primitive (real object) values by calling an imacro.
>+           We don't try to guess about which imacro, with what valueOf hint, here. */
>+        JS_ASSERT(JSVAL_IS_NULL(v));
>+        return INS_CONSTPTR(cx->runtime->atomState.nullAtom);

Good ol' SpiderMonkey major comment style, please.


>@@ -6709,7 +6711,7 @@ TraceRecorder::functionCall(bool constru
>                 return call_imacro(call_imacros.String);
>             LIns *i = stringify(v);
>             if (!i)
>-                ABORT_TRACE("can't stringify value");
>+                return false;

We can't actually hit this, can we?  Object's taken care of by imacro, integers/doubles/integers/strings/integers/booleans/integers taken care of by stringify itself, and can insCall return NULL by itself?  I'm guessing the answer is no as I'm pretty sure this would fail everywhere otherwise; please enlighten me if it is otherwise.  This should be an assert that stringify didn't return NULL if my understanding is correct.

With a trace-test to verify this works, with recorderAborted jitstats to verify non-aborting here, r=me.
Attachment #366658 - Flags: review?(jwalden+bmo) → review+
Posted patch fix, v2 (obsolete) — Splinter Review
Attachment #366658 - Attachment is obsolete: true
Attachment #366680 - Flags: review?(jwalden+bmo)
Attachment #366680 - Attachment is patch: true
Attachment #366680 - Attachment mime type: application/octet-stream → text/plain
Posted patch fix, v2Splinter Review
Attachment #366680 - Attachment is obsolete: true
Attachment #366683 - Flags: review?
Attachment #366680 - Flags: review?(jwalden+bmo)
Comment on attachment 366683 [details] [diff] [review]
fix, v2

>diff --git a/js/src/trace-test.js b/js/src/trace-test.js

>+function testNullToString()
>+{
>+  var a = [];
>+  for (var i = 0; i < 10; i++)
>+    a.push(String(null));
>+  for (i = 0; i < 10; i++) {
>+    var t = typeof a[i];
>+    if (t != "string")
>+      a.push(t);
>+  }
>+  return a.join(",");
>+}
>+testNullToString.expected = "null,null,null,null,null,null,null,null,null,null";
>+testNullToString.jitstats = {
>+  recorderStarted: 2,
>+  sideExitIntoInterpreter: 2
>+};
>+test(testNullToString);

This wants a recorderAborted: 0 as well; two side-exits should mean no aborts here, but better to be precise than miss a regression somehow.  Otherwise all looks good.
Attachment #366683 - Flags: review? → review+
In tm:

http://hg.mozilla.org/tracemonkey/rev/89b61367af4c

/be
Whiteboard: fixed-in-tracemonkey
sayrer, did this miss the m-c bus?

/be
http://hg.mozilla.org/mozilla-central/rev/89b61367af4c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 482783
http://hg.mozilla.org/tracemonkey/rev/3fb1b5d6dc60
/cvsroot/mozilla/js/tests/js1_8_1/trace/trace-test.js,v  <--  trace-test.js
new revision: 1.13; previous revision: 1.12
Flags: in-testsuite+
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
cvsroot/mozilla/js/tests/js1_8_1/trace/trace-test.js,v  <--  trace-test.js
new revision: 1.14; previous revision: 1.13

/cvsroot/mozilla/js/tests/shell.js,v  <--  shell.js
You need to log in before you can comment on or make changes to this bug.