Closed Bug 1304643 Opened 3 years ago Closed 3 years ago

Differential Testing: Different output message involving Object.create

Categories

(Core :: JavaScript Engine: JIT, defect, P1, major)

x86_64
All
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- affected
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: gkw, Assigned: h4writer)

References

(Blocks 2 open bugs)

Details

(Keywords: testcase)

Attachments

(1 file)

var x = Object.create(this);
var y = '1';
for (var i = 0; i < 3; ++i) {
    y += x.y;
}
print(y);

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

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

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/6b1e076dbcb7
user:        Hannes Verschore
date:        Tue May 24 07:43:20 2016 +0200
summary:     Bug 1269313: IonMonkey - Use TI to break alias between instructions, r=jandem

Hannes, is bug 1269313 a likely regressor?
Flags: needinfo?(hv1989)
Flags: needinfo?(hv1989)
Priority: -- → P1
Flags: needinfo?(hv1989)
Attached patch patchSplinter Review
This solves the issue. For MGetPropertyCache the property we want to get could be on the prototype. For now disable testing TI of the objects to know if they are the same. If we could get the prototype where this property lives we could use that to test! 
Maybe an optimization we can do lateron. Find the correct prototype and remember that so we can test that.
Assignee: nobody → hv1989
Flags: needinfo?(hv1989)
Attachment #8802098 - Flags: review?(jdemooij)
Comment on attachment 8802098 [details] [diff] [review]
patch

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

Please add a test that fails without the patch, and we should probably backport this.

::: js/src/jit/AliasAnalysisShared.cpp
@@ +165,5 @@
>      return object;
>  }
>  
> +
> +

Nit: rm blank lines
Attachment #8802098 - Flags: review?(jdemooij) → review+
Pushed by hv1989@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2faa2e745b8e
IonMonkey: Only mark as not aliasing if object owns property, r=jandem
Comment on attachment 8802098 [details] [diff] [review]
patch

Approval Request Comment

[Feature/regressing bug #]:
bug 1269313

[User impact if declined]:
Possible wrong results when executing JS code.

[Describe test coverage new/current, TreeHerder]:
Passes jit-tests and green on inbound

[Risks and why]: 
Not really risky w.r.t. crashing or introducing bugs. The change now takes a well tested path. The only issue could be a regression. Though I assume there is none and if there is, we probably optimized something incorrectly.

[String/UUID change made/needed]:
/
Attachment #8802098 - Flags: approval-mozilla-beta?
Attachment #8802098 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/2faa2e745b8e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8802098 [details] [diff] [review]
patch

Let's stabilize this on Aurora51 and then uplift to Beta50 before we gtb 50.0b10 on Monday.
Attachment #8802098 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8802098 [details] [diff] [review]
patch

Even though this has stabilized a bit on Nightly52 and Aurora51, given how late it is in Beta50 cycle (a week away from RC) and the fact that this bug exists since 48, I have decided to not take it in 50.
Attachment #8802098 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.