fix: ensure that property is writable and configurable #1238

gajus · 2016-09-18T16:25:54Z

A property thats writable but not configurable cannot be overwitten.
Attempting to overwrite a non-configurable property will result in
error equivalent to:

TypeError: Cannot redefine property: stack

A property thats writable but not configurable cannot be overwitten. Attempting to overwrite a non-configurable property will result in error equivalent to: TypeError: Cannot redefine property: stack

gajus · 2016-09-18T16:27:02Z

I have changed the formatting of the condition too, because as it is, the current condition is neither correct (it returns on the first positive condition, either writable or configurable), neither readable.

gajus · 2016-09-18T16:40:33Z

To clarify:

17:35 not-an-aardvark What issue are were you having that is fixed by the change?
17:39 gajus I have described that in the github issue
17:39 gajus an error thrown with 'stack' property configured to {writable: true, configurable: false}
17:39 gajus will cause Bluebird to swallow the error and fail silently with the original promise never being resolved [or rejected]

This is the stack trace:

  unhandled-rejection Got unhandledRejection +50ms
  unhandled-rejection Emitting unhandledRejection... +0ms
unhandledRejection TypeError: Cannot redefine property: stack
    at Object.defineProperty (native)
    at Object.notEnumerableProp (/node_modules/bookshelf/node_modules/bluebird/js/release/util.js:102:11)
    at CapturedTrace.attachExtraTrace (/node_modules/bookshelf/node_modules/bluebird/js/release/debuggability.js:785:10)
    at Promise.longStackTracesAttachExtraTrace [as _attachExtraTrace] (/node_modules/bookshelf/node_modules/bluebird/js/release/debuggability.js:406:19)
    at Promise._rejectCallback (/node_modules/bookshelf/node_modules/bluebird/js/release/promise.js:472:10)
    at reject (/node_modules/bookshelf/node_modules/bluebird/js/release/thenables.js:79:17)
    at run (/node_modules/core-js/modules/es6.promise.js:87:22)
    at /node_modules/core-js/modules/es6.promise.js:100:28
    at flush (/node_modules/core-js/modules/_microtask.js:18:9)
    at _combinedTickCallback (internal/process/next_tick.js:67:7)
    at process._tickDomainCallback (internal/process/next_tick.js:122:9)

gajus · 2016-09-18T17:14:40Z

To replicate the issue:

import Promise from 'bluebird';

const error = new Error('test');

Object.defineProperty(error, 'stack', {
  value: 'test',
  writable: true,
  enumerable: false,
  configurable: false
});

new Promise(() => {
  throw error;
});

which will produce:

/node_modules/bluebird/js/release/util.js:97
    es5.defineProperty(obj, name, descriptor);
        ^

TypeError: Cannot redefine property: stack
    at Object.defineProperty (native)
    at Object.notEnumerableProp (/node_modules/bluebird/js/release/util.js:97:9)
    at Promise.longStackTracesAttachExtraTrace [as _attachExtraTrace] (/node_modules/bluebird/js/release/debuggability.js:409:18)
    at Promise._rejectCallback (/node_modules/bluebird/js/release/promise.js:472:10)
    at Promise._resolveFromExecutor (/node_modules/bluebird/js/release/promise.js:490:17)
    at new Promise (/node_modules/bluebird/js/release/promise.js:77:14)
    at Object.<anonymous> (/src/bin/server.js:12:1)
    at Module._compile (module.js:556:32)
    at loader (/node_modules/babel-register/lib/node.js:146:5)
    at Object.require.extensions.(anonymous function) [as .js] (/node_modules/babel-register/lib/node.js:156:7)
    at Module.load (module.js:473:32)
    at tryModuleLoad (module.js:432:12)
    at Function.Module._load (module.js:424:3)
    at Function.Module.runMain (module.js:590:10)
    at /node_modules/babel-cli/lib/_babel-node.js:151:24
    at Object.<anonymous> (/node_modules/babel-cli/lib/_babel-node.js:152:7)

darthtrevino · 2016-09-26T17:58:23Z

🙏

gajus · 2016-10-17T10:40:16Z

@petkaantonov Is there a reason for not merging this?

petkaantonov · 2016-10-17T11:21:18Z

src/es5.js

+                return true;
+            }
+
+            if (descriptor.set) {


What happens if descriptor.configurable is false even if there is a setter?

It is not possible. You will get an error:

TypeError: Invalid property descriptor. Cannot both specify accessors and a value or writable attribute, #(anonymous function) ..

I don't get such error:

Object.defineProperty({}, "asd", {set: function(){}, get: function(){}, configurable: false})

I think I misunderstood the purpose of a setter and distinction between having a setter and the purpose of the writable and configurable configs in this context.

It appears that descriptor.set presence is irrelevant for determining whether property is writable/ configurable.

petkaantonov · 2016-10-17T11:21:41Z

src/es5.js

+                return true;
+            }
+
+            if (descriptor.writable !== true) {


if (!descriptor.writable)

As far as I remember, the reason I have used strict equality here is because it isn't clear reading the spec whether writable (and other optional properties) are even going to be set if they are not explicitly configured.

However, as far as my simple testing goes on Chrome, default values are always populated, e.g.

const error = new Error('test'); Object.defineProperty(error, 'stack', { value: 'test' }); Object.getOwnPropertyDescriptor(error, 'stack');

Object {value: "test", writable: false, enumerable: false, configurable: false}

I will make the change.

petkaantonov · 2016-10-17T11:22:00Z

src/es5.js

+                return false;
+            }
+
+            if (descriptor.configurable !== true) {


if (!descriptor.configurable)

petkaantonov · 2016-10-17T11:26:03Z

perhaps it's better to use propertyIsConfigurable separately?

gajus · 2016-10-17T11:44:16Z

perhaps it's better to use propertyIsConfigurable separately?

I am not certain whats that in reference to.

petkaantonov · 2016-10-17T11:49:15Z

Using Object.defineProperty only needs configurable true or undefined descriptor, so for this code propertyIsConfigurable would be more appropriate check

fix: ensure that property is writable and configurable

43d7e4b

A property thats writable but not configurable cannot be overwitten. Attempting to overwrite a non-configurable property will result in error equivalent to: TypeError: Cannot redefine property: stack

darthtrevino mentioned this pull request Sep 26, 2016

resolve() hangs with rejected bluebird Promises graphql/graphql-js#488

Closed

darthtrevino mentioned this pull request Sep 26, 2016

Add 'configurable: true' to GraphQLError stack property graphql/graphql-js#505

Merged

petkaantonov reviewed Oct 17, 2016

View reviewed changes

src/es5.js

return false;

}

if (descriptor.configurable !== true) {

Copy link

Owner

petkaantonov Oct 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (!descriptor.configurable)

gajus added 2 commits October 17, 2016 12:33

style: remove strict equality

5ef7495

fix: remove setter presence check

8e056d6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ensure that property is writable and configurable #1238

fix: ensure that property is writable and configurable #1238

gajus commented Sep 18, 2016

gajus commented Sep 18, 2016

gajus commented Sep 18, 2016 •

edited

gajus commented Sep 18, 2016

darthtrevino commented Sep 26, 2016

gajus commented Oct 17, 2016

petkaantonov Oct 17, 2016

gajus Oct 17, 2016

petkaantonov Oct 17, 2016

gajus Oct 17, 2016

petkaantonov Oct 17, 2016

gajus Oct 17, 2016

petkaantonov Oct 17, 2016

petkaantonov commented Oct 17, 2016

gajus commented Oct 17, 2016

petkaantonov commented Oct 17, 2016

fix: ensure that property is writable and configurable #1238

Are you sure you want to change the base?

fix: ensure that property is writable and configurable #1238

Conversation

gajus commented Sep 18, 2016

gajus commented Sep 18, 2016

gajus commented Sep 18, 2016 • edited

gajus commented Sep 18, 2016

darthtrevino commented Sep 26, 2016

gajus commented Oct 17, 2016

petkaantonov Oct 17, 2016

Choose a reason for hiding this comment

gajus Oct 17, 2016

Choose a reason for hiding this comment

petkaantonov Oct 17, 2016

Choose a reason for hiding this comment

gajus Oct 17, 2016

Choose a reason for hiding this comment

petkaantonov Oct 17, 2016

Choose a reason for hiding this comment

gajus Oct 17, 2016

Choose a reason for hiding this comment

petkaantonov Oct 17, 2016

Choose a reason for hiding this comment

petkaantonov commented Oct 17, 2016

gajus commented Oct 17, 2016

petkaantonov commented Oct 17, 2016

gajus commented Sep 18, 2016 •

edited