Differential Testing: Different output message involving Object.freeze

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
major
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: gkw, Unassigned)

Tracking

(Blocks 2 bugs, {testcase})

Trunk
mozilla52
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(5 attachments)

Reporter

Description

3 years ago
x = [];
try {
    Array.prototype.unshift.apply(x, [2]);
    x.toSource = function() {
        print("z");
    }
    Object.freeze(x);
    x.sort(function() {});
} catch (e) {}
try {
    Array.prototype.sort.call(x, function() {});
} catch (e) {}

$ ./js-dbg-64-dm-clang-darwin-560b2c805bf7 --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js

$ ./js-dbg-64-dm-clang-darwin-560b2c805bf7 --fuzzing-safe --no-threads --ion-eager testcase.js
z
Tested this on m-c rev 560b2c805bf7.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin14.5.0 --disable-jemalloc --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 560b2c805bf7

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/26e6632de510
user:        Leo Gaspard
date:        Thu Aug 25 16:28:31 2016 -0700
summary:     Bug 1283334 - Part 3: Do not sparsify dense arrays when freezing - Ion. r=nbp

Leo, is bug 1283334 a likely regressor?
Flags: needinfo?(leo)
Flags: needinfo?(nicolas.b.pierron)
Just for the record, since I got distracted looking into this, and I guess this would be helpful, I think it's an inlining bug, based on:

 * The test case still passes with --ion-eager but without --no-threads.
 * Fails: --ion-eager --ion-offthread-compile=off
 * Passes: --ion-eager --ion-offthread-compile=off --ion-inlining=off

I assume with --no-threads ion does more aggressive inlining?

Sorry for not being able to provide more details, but it's the first time I've ever looked at ion. I did a bit more investigation, but I'm still grasping the code and I don't have a clear idea of where the bug may come from exactly.

As a funny note, if you don't catch the last exception in the test case, apart from the "z", you get a |TypeError: undefined is read-only| error, instead of the expected |0 is read-only|, so I guess something is really messed up with the generated code... Sorry I can't help more :(
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> if you don't catch the last exception in the test case,
> apart from the "z", you get a |TypeError: undefined is read-only| error,
> instead of the expected |0 is read-only|, so I guess something is really
> messed up with the generated code...

The mechanism we use to produce error messages that contain a string representation of most values that are on the JS stack is thoroughly unprincipled.  (Sometimes we copy values off the stack, so mere existence on the JS stack at *some* point isn't enough to hit the unprincipled mechanism.)  It works sometimes, maybe even often.  But it's basically trivial to deliberately make it generate absurdly mistaken reprs, and it's not that much harder to do by accident.

So generally, you should take goofy-looking parts of error messages with a huge grain of salt, because they're very likely wrong and not all that representative of what was actually done.  I didn't bother looking at the testcase, but odds are you're just hitting this broken mechanism, not finding something "really messed up" with the generated code.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> So generally, you should take goofy-looking parts of error messages with a
> huge grain of salt, because they're very likely wrong and not all that
> representative of what was actually done.  I didn't bother looking at the
> testcase, but odds are you're just hitting this broken mechanism, not
> finding something "really messed up" with the generated code.

Fair enough, I shouldn't have said that without knowing more.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> > So generally, you should take goofy-looking parts of error messages with a
> > huge grain of salt, because they're very likely wrong and not all that
> > representative of what was actually done.  I didn't bother looking at the
> > testcase, but odds are you're just hitting this broken mechanism, not
> > finding something "really messed up" with the generated code.
> 
> Fair enough, I shouldn't have said that without knowing more.

(That being said, the first try block exception message is ok, but the second (which throws at exactly the same operation over the same array isn't)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)

>  * The test case still passes with --ion-eager but without --no-threads.
>  * Fails: --ion-eager --ion-offthread-compile=off
>  * Passes: --ion-eager --ion-offthread-compile=off --ion-inlining=off
> 
> I assume with --no-threads ion does more aggressive inlining?

When we do not have --no-threads, we are compiling off the main thread, which might let enough time to let the function work pass the point where it might fail if it were in IonMonkey.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> Fair enough, I shouldn't have said that without knowing more.

No!  :-)  Conjecture, speculate, etc. as it's how you learn when you're wrong.  I'm just telling you the story so you know it for the future.  Good habit to say at least one wrong thing a day, or something like that.  ;-)

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> (That being said, the first try block exception message is ok, but the
> second (which throws at exactly the same operation over the same array isn't)

Still not investigating the bug report, so this could be not an explanation for what you're seeing.

But.  The basic idea of stacked-value decompilation is that we look backward from the top of the stack for the value to be decompiled, and when we find it, we rerun bytecode up to the current point to determine what operation placed that value there.  If it's the wrong stack offset, the wrong operation was presumed to place the value there, so a decompilation string for a wholly different operation is used.  Thus you can get correct *or* incorrect decompilation for an error at a particular point in the code, depending what value is being decompiled, and where it exists and does not exist on the stack.
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> But.  The basic idea of stacked-value decompilation is that we look backward
> from the top of the stack for the value to be decompiled, and when we find
> it, we rerun bytecode up to the current point to determine what operation
> placed that value there.

Thanks for pointing to stack-value decompilation Jeff.

So I posted a patch that prevents the call to ToSource and fixes the error message. Then another patch because IMO if the array is single-element we shouldn't be setting anything into it in the first place.

Still, there are a few subtleties that I'm suspicious of.

So this was throwing in the self-hosted MergeSort (concretely in MoveHoles). I don't think we should be throwing there given the script wasn't in script mode. I guess self-hosted code runs in strict mode, so the opcode generated was STRICTSETELEM, which makes us throw for frozen objects.

It seems to me that this may be harder to fix that what I initially though due to this... Any idea?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> I don't think we should be throwing there given the script wasn't in script
> mode.

Strict mode, that is.
Comment hidden (mozreview-request)

Comment 12

3 years ago
mozreview-review
Comment on attachment 8799230 [details]
Bug 1304638 - Return early from sorting if the array is single-element or empty.

https://reviewboard.mozilla.org/r/84508/#review83110

::: js/src/builtin/Array.js:203
(Diff revision 2)
>      /* Step 2. */
>      var len = ToLength(O.length);
>  
> +    if (len <= 1)
> +      return this;

The spec does not explictly add such case in the Step 2.  On the other hand, it defines that:

> Perform an implementation-dependent sequence of calls to the [[Get]] and [[Set]] internal methods of obj

So, I guess, no calls to any of these method is an implementation-dependent sequence.

This is also what is done by the MergeSort call below, after some extra checks for unwrapping TypeArrays, and also and extra temporary allocation.
Attachment #8799230 - Flags: review?(nicolas.b.pierron) → review+

Comment 13

3 years ago
mozreview-review
Comment on attachment 8799229 [details]
Bug 1304638 - Don't return "(intermediate value)" while decompiling SETELEM/STRICTSETELEM.

https://reviewboard.mozilla.org/r/84506/#review83122
Attachment #8799229 - Flags: review?(nicolas.b.pierron) → review+
> Bug 1304638 - Don't return "(intermediate value)" while decompiling
> SETELEM/STRICTSETELEM.

Driveby nit, we need tests for this. See this one for an example: http://searchfox.org/mozilla-central/source/js/src/jit-test/tests/basic/expression-autopsy.js
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

3 years ago
mozreview-review
Comment on attachment 8799439 [details]
Bug 1304638 - Tests for decompilation of SETELEM/STRICTSETELEM.

https://reviewboard.mozilla.org/r/84612/#review83168

::: js/src/jit-test/tests/basic/expression-autopsy.js:75
(Diff revision 1)
> -check("this.x");
> +check("this.x", "this.x", false); // FIXME: This one is known to fail in strict mode.
>  check("ieval(undef)", "ieval(...)");

nit: Open a bug, and insert the bug number in this comment.

::: js/src/jit-test/tests/basic/expression-autopsy.js:101
(Diff revision 1)
> +check_one("0", function() {
> +  "use strict";
> +  var o = [0];
> +  Object.freeze(o);
> +  o[0] = "foo";
> +}, " is read-only");

Should this be "o[0]" instead of "0"?

Comment 18

3 years ago
mozreview-review
Comment on attachment 8799440 [details]
Bug 1304638 - Remove redundant call to offsetForStackOperand.

https://reviewboard.mozilla.org/r/84614/#review83172

Good catch!
Attachment #8799440 - Flags: review?(nicolas.b.pierron) → review+
Flags: needinfo?(nicolas.b.pierron)

Comment 19

3 years ago
mozreview-review
Comment on attachment 8799439 [details]
Bug 1304638 - Tests for decompilation of SETELEM/STRICTSETELEM.

https://reviewboard.mozilla.org/r/84612/#review83180

(cancel review; re-ask for review after answering the question about check_one first argument)
Attachment #8799439 - Flags: review?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #17) 
> Should this be "o[0]" instead of "0"?

Nope, I'll try to explain why. I'm still looking into how should that be unified/fixed properly (didn't plan to close this bug without that changed).

So that test doesn't test decompilation (and thus I'll need to change it, good catch). Consider the following test-case:

"use strict";

for (var i = 0; i < 2; ++i) {
  try {
    var to = [0, 1];
    var from = [5, 1];
    Object.freeze(to);
    for (var j = 0; j < 1; ++j) {
      to[0] = from[0];
    }
  } catch (e) {
    print(e);
  }
}

(Note: The fact that the statement to[0] = from[0] is in a useless one-iteration loop is important).

So this will print two different error messages (with --ion-eager --no-threads), the first being "0 is read-only", and the second being "to[0]" is read-only. After a bit of debugging I discovered that the first one is not using decompilation (the first is going through js::jit::DoSetElemFallback, while the second one is going through ThrowReadOnlyError).

It's still not clear to me why this difference among both iterations (and that's why my previous comment about the error message difference comes from, btw). I'd expect both iterations to go through ThrowReadOnlyError, I'm still looking into why this happens, though my JIT debugging abilities are not precisely over the roof (improving, though :P).

Nicolas, do you have any insight in why this might be happening?
Flags: needinfo?(nicolas.b.pierron)

Comment 21

3 years ago
mozreview-review-reply
Comment on attachment 8799439 [details]
Bug 1304638 - Tests for decompilation of SETELEM/STRICTSETELEM.

https://reviewboard.mozilla.org/r/84612/#review83168

> nit: Open a bug, and insert the bug number in this comment.

This is not a bug, sorry. In strict mode `this` is undefined.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #20)
> Nicolas, do you have any insight in why this might be happening?

So apparently this enters on the baseline path, then on ion. Assuming we're fine with it... I think this is ready. Also, I remember seeing other similar case while testing (something that threw a different exception message after the first iteration), but I've lost that test-case, sorry.

Assuming this is expected to happen (and I guess it is given the ifdefs for JS_MORE_DETERMINISTIC in jsopcode.cpp), I think this is pretty much ready. Clearing the ni? since I think this is somewhat expected.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #24)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #20)
> > Nicolas, do you have any insight in why this might be happening?
> 
> So apparently this enters on the baseline path, then on ion. Assuming we're
> fine with it... I think this is ready. Also, I remember seeing other similar
> case while testing (something that threw a different exception message after
> the first iteration), but I've lost that test-case, sorry.
> 
> Assuming this is expected to happen (and I guess it is given the ifdefs for
> JS_MORE_DETERMINISTIC in jsopcode.cpp), I think this is pretty much ready.
> Clearing the ni? since I think this is somewhat expected.

Usually we use JS_MORE_DETERMINISTIC when Ion does not have enough information to display useful information, we rarely do it when Ion has more information than baseline as this is not supposed to happen.

I would definitely prefer if we could fix this issue as "0 is read-only" is weird, and I would expect to have one of the following error message instead:

  0 is a read-only property.
  o[0] is read-only.
Comment on attachment 8799739 [details]
Bug 1304638 - Improve error message for JSMSG_READ_ONLY when we're not in C++ code.

Forwarding the review, as I am not familiar with this code.
Nice finding.
Attachment #8799739 - Flags: review?(nicolas.b.pierron) → review?(jwalden+bmo)
Once you are done with the last patch, don't forget to update the MDN page about this error message [1], to include the object name as part of the error message:

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Read-only
See Also: → 1310714

Comment 29

3 years ago
mozreview-review
Comment on attachment 8799439 [details]
Bug 1304638 - Tests for decompilation of SETELEM/STRICTSETELEM.

https://reviewboard.mozilla.org/r/84612/#review85702

::: js/src/jsopcode.cpp:1271
(Diff revision 2)
>                 quote(prop, '\0');
>        }
>        case JSOP_SETELEM:
>        case JSOP_STRICTSETELEM:
> +        // NOTE: We don't show the right hand side of the operation because
> +        // it's used on error messages like: "a[0] is not readable".

"used in error messages"
What is the status of this bug? Is there anything blocking progress?
(In reply to Hannes Verschore [:h4writer] from comment #30)
> What is the status of this bug? Is there anything blocking progress?

Hmm, last time Waldo was unsure about the one-off that I used for the last patch (basically, decompiling all the time _except_ when the last frame was a function call).

I think that shouldn't be a blocker, and that we can land the rest of changes, mark this bug as fixed, and move the last patch changing the error message for the interpreter to another bug.

Does that seem reasonable?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #31)
> Does that seem reasonable?

Yes that seems reasonable to me.

Comment 33

3 years ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27f76213fa87
Don't return "(intermediate value)" while decompiling SETELEM/STRICTSETELEM. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/107238b89fcc
Return early from sorting if the array is single-element or empty. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8bb58ecb17d
Remove redundant call to offsetForStackOperand. r=nbp
See Also: → 1313392
Flags: needinfo?(leo)
we see a perf win with this patch set:
== Change summary for alert #3917 (as of October 27 2016 08:41 UTC) ==

Improvements:

  2%  tcanvasmark summary linux64 opt e10s     9274.83 -> 9461.12

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=3917
Attachment #8799739 - Flags: review?(jwalden+bmo) → review?(nicolas.b.pierron)

Comment 36

2 years ago
mozreview-review
Comment on attachment 8799439 [details]
Bug 1304638 - Tests for decompilation of SETELEM/STRICTSETELEM.

https://reviewboard.mozilla.org/r/84612/#review141010

Thanks for adding more tests :)
Attachment #8799439 - Flags: review?(nicolas.b.pierron) → review+

Comment 37

2 years ago
mozreview-review
Comment on attachment 8799739 [details]
Bug 1304638 - Improve error message for JSMSG_READ_ONLY when we're not in C++ code.

https://reviewboard.mozilla.org/r/84872/#review141054

(canceling review) This patch moved to Bug 1313392.
Attachment #8799739 - Flags: review?(nicolas.b.pierron)
You need to log in before you can comment on or make changes to this bug.