Closed
Bug 1397026
Opened 7 years ago
Closed 7 years ago
Differential Testing: Different output message involving Object.defineProperty
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: gkw, Assigned: jandem)
References
Details
(Keywords: testcase)
Attachments
(1 file, 1 obsolete file)
6.49 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
function f(x) { Object.defineProperty(this, "setUint8", ({ value: true, writable: true, configurable: true, enumerable: true })); if (x) { Object.seal(this); } } for (let z of [0, /x/, 0, 0]) { try { f(z); } catch (e) { print("FOO"); } } $ ./js-dbg-64-dm-linux-0afabd3e5c27 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js FOO $ ./js-dbg-64-dm-linux-0afabd3e5c27 --fuzzing-safe --no-threads --ion-eager testcase.js FOO FOO Tested this on m-c rev 0afabd3e5c27. My configure flags are: AR=ar sh ./configure --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 0afabd3e5c27 autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/a14fc8f5babc user: André Bargull date: Mon Aug 28 14:19:34 2017 +0200 summary: Bug 1303335: Move parts of Object.getOwnProperty and Object.defineProperty to self-hosted code. r=till Andre, is bug 1303335 a likely regressor?
Flags: needinfo?(andrebargull)
Comment 1•7 years ago
|
||
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0) > autoBisect shows this is probably related to the following changeset: > > The first bad revision is: > changeset: https://hg.mozilla.org/mozilla-central/rev/a14fc8f5babc > user: André Bargull > date: Mon Aug 28 14:19:34 2017 +0200 > summary: Bug 1303335: Move parts of Object.getOwnProperty and > Object.defineProperty to self-hosted code. r=till > > Andre, is bug 1303335 a likely regressor? Yes. Slightly reduced test case: --- var o = {}; for (var i = 0; i < 4; ++i) { var desc = { value: i, writable: true, configurable: true, enumerable: true }; try { Object.defineProperty(o, "p", desc); } catch (e) { } if (i === 1) { Object.defineProperty(o, "p", {configurable: false}); } print(o.p); } --- Expected: Prints "0 1 1 1" Actual: Prints "0 1 1 3" Jan, it looks _DefineDataProperty, when optimized as INIT_ELEM, doesn't actually perform [[DefineOwnProperty]], but merely [[Set]]. Is this reasoning correct and how can we fix it to perform a proper [[DefineOwnProperty]]?
Flags: needinfo?(andrebargull) → needinfo?(jdemooij)
Assignee | ||
Comment 2•7 years ago
|
||
Good catch. This fixes it and is the only place that needs fixing AFAICS.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8904957 -
Flags: review?(andrebargull)
Comment 3•7 years ago
|
||
Comment on attachment 8904957 [details] [diff] [review] Patch Review of attachment 8904957 [details] [diff] [review]: ----------------------------------------------------------------- There is still an issue where [[Enumerable]] is not set correctly. Test case for [[Enumerable]], still not passing (with --baseline-eager). Expected: Prints "true" four times. Actual: Prints "true" three times and then "false" --- var o = {}; for (var i = 0; i < 4; ++i) { var desc = { value: i, writable: true, configurable: true, enumerable: true }; try { Object.defineProperty(o, "p", desc); } catch (e) { } print(Object.getOwnPropertyDescriptor(o, "p").enumerable); if (i > 0) { Object.defineProperty(o, "p", {enumerable: false}); } } --- Test case which failed before bug 1303335 (passes with the attached patch): --- var currentOptions; var options = { get month() { currentOptions = this; currentOptions.year = "two-digit"; if (i > 5) { Object.defineProperty(currentOptions, "year", {configurable: false}); } } }; var date = new Date(); for (var i = 0; i < 10; ++i) { try { date.toLocaleString(undefined, options); } catch (e) {} print(currentOptions.year); } ---
Attachment #8904957 -
Flags: review?(andrebargull) → review-
Assignee | ||
Comment 4•7 years ago
|
||
How about this one? We check for configurable/writable/enumerable now so I think that should cover everything?
Attachment #8904957 -
Attachment is obsolete: true
Attachment #8904990 -
Flags: review?(andrebargull)
Comment 5•7 years ago
|
||
Comment on attachment 8904990 [details] [diff] [review] Patch Review of attachment 8904990 [details] [diff] [review]: ----------------------------------------------------------------- Yes, that should work. Thanks!
Attachment #8904990 -
Flags: review?(andrebargull) → review+
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/509c46b010b6 Make sure JSOP_INIT* IC behavior matches [[DefineOwnProperty]] instead of [[Set]]. r=anba
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/509c46b010b6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•