Closed Bug 913885 Opened 7 years ago Closed 7 years ago

Differential Testing: Different output message involving schedulegc

Categories

(Core :: JavaScript Engine, defect, major)

x86_64
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: gkw, Assigned: jorendorff)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, testcase)

Attachments

(1 file)

try {
    Object.defineProperty(this, "a1", {
        get: function() {
            return [({}), eval("eval(\"\",\"\")", x), ({})]
        }
    })
    a1;
    var x, u;
} catch (e) {}
try {
    schedulegc(5);
    a1
} catch (e) {}

prints the following on js opt shell on m-c changeset 3697f962bb7b without any CLI arguments, when the testcase is passed in as a CLI argument:

1613-interesting.js:4:12 warning: Support for eval(code, scopeObject) has been removed. Use |with (scopeObject) eval(code);| instead.
1613-interesting.js:4:0 warning: Support for eval(code, scopeObject) has been removed. Use |with (scopeObject) eval(code);| instead.

However it prints the following with --ion-eager:

1613-interesting.js:4:12 warning: Support for eval(code, scopeObject) has been removed. Use |with (scopeObject) eval(code);| instead.
1613-interesting.js:4:0 warning: Support for eval(code, scopeObject) has been removed. Use |with (scopeObject) eval(code);| instead.
1613-interesting.js:4:0 warning: Support for eval(code, scopeObject) has been removed. Use |with (scopeObject) eval(code);| instead.


My configure flags are:

CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin11.4.0 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <more NSPR compilation flags>
Flags: needinfo?(jdemooij)
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/77e2eaaf2fbb
user:        Jason Orendorff
date:        Fri Sep 06 12:05:30 2013 -0500
summary:     Bug 893186, part 5 - Delete JS_GetPropertyAttributes and many similar APIs; remove half a dozen methods from JS::Class. r=Waldo.

Jason, is bug 893186 a possible regressor?
Blocks: 893186
Flags: needinfo?(jdemooij) → needinfo?(jorendorff)
It's possible, but the bug doesn't reproduce for me in tip. Building 77e2eaaf2fbb now...
Flags: needinfo?(jorendorff)
I can't reproduce it in 77e2eaaf2fbb either.

Whether or not we generate the third warning depends on whether we get an eval cache hit. So I think it's nondeterministic, and it would be good to skip this warning when fuzzing.

However I think it's better to delete this warning entirely now. Waldo, do you agree?
Flags: needinfo?(jwalden+bmo)
(In reply to Jason Orendorff [:jorendorff] from comment #3)
> I can't reproduce it in 77e2eaaf2fbb either.

I tested on Mac 10.8, and my configure options are as in comment 0. How about rev 3697f962bb7b as specified there?

It may have gone away though..
I think at this point we can just kill the warning.  We haven't shipped two-argument eval in years, now.
Flags: needinfo?(jwalden+bmo)
Attached patch v1Splinter Review
OK then.
Assignee: general → jorendorff
Attachment #803788 - Flags: review?(jwalden+bmo)
Comment on attachment 803788 [details] [diff] [review]
v1

Review of attachment 803788 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Eval.cpp
@@ -407,5 @@
> -WarnOnTooManyArgs(JSContext *cx, const CallArgs &args)
> -{
> -    if (args.length() > 1) {
> -        Rooted<JSScript*> script(cx, cx->currentScript());
> -        if (script && !script->warnedAboutTwoArgumentEval) {

You forgot about warnedAboutTwoArgumentEval in JSScript
Comment on attachment 803788 [details] [diff] [review]
v1

Review of attachment 803788 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, remove the deadweight JSScript field, too.
Attachment #803788 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/29458f6c07e5
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.