120314 2013-08-26 10:03:46 -0700 Object.defineProperty should be able to create a PropertyDescriptor where m_attributes == 0 2013-08-26 14:11:03 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 mhahnenberg mhahnenberg darin oldest_to_newest 921052 0 mhahnenberg 2013-08-26 10:03:46 -0700 Currently with the way that defineProperty works, we leave a stray low bit set in PropertyDescriptor::m_attributes in the following code: var o = {}; Object.defineProperty(o, 100, {writable:true, enumerable:true, configurable:true, value:"foo"}); This is due to the fact that the lowest non-zero attribute (ReadOnly) is represented as 1 << 1 instead of 1 << 0. We then calculate the default attributes as ((1 << 3) << 1) - 1, which is 0xF, but only the top three bits mean anything. Even in the case above, the top three bits are set to 0 but the bottom bit remains set, which causes us to think m_attributes is non-zero. 921057 1 209660 mhahnenberg 2013-08-26 10:09:43 -0700 Created attachment 209660 Patch 921061 2 209660 darin 2013-08-26 10:14:27 -0700 Comment on attachment 209660 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209660&action=review > Source/JavaScriptCore/ChangeLog:17 > + This is due to the fact that the lowest non-zero attribute (ReadOnly) is represented as 1 << 1 > + instead of 1 << 0. We then calculate the default attributes as ((1 << 3) << 1) - 1, which is 0xF, > + but only the top three bits mean anything. Even in the case above, the top three bits are set > + to 0 but the bottom bit remains set, which causes us to think m_attributes is non-zero. I think the real problem is in the code that calculates the default attributes. Where is that code? Is there some reason it has to do "((1 << 3) << 1) - 1" instead of just or'ing together the specific attributes? 921064 3 mhahnenberg 2013-08-26 10:20:17 -0700 (In reply to comment #2) > (From update of attachment 209660 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209660&action=review > > > Source/JavaScriptCore/ChangeLog:17 > > + This is due to the fact that the lowest non-zero attribute (ReadOnly) is represented as 1 << 1 > > + instead of 1 << 0. We then calculate the default attributes as ((1 << 3) << 1) - 1, which is 0xF, > > + but only the top three bits mean anything. Even in the case above, the top three bits are set > > + to 0 but the bottom bit remains set, which causes us to think m_attributes is non-zero. > > I think the real problem is in the code that calculates the default attributes. Where is that code? Is there some reason it has to do "((1 << 3) << 1) - 1" instead of just or'ing together the specific attributes? It's in PropertyDescriptor.cpp: unsigned PropertyDescriptor::defaultAttributes = (DontDelete << 1) - 1; I agree that it would be better style to or the default attributes together. I'll make that change. 921075 4 mhahnenberg 2013-08-26 10:33:09 -0700 (In reply to comment #2) > (From update of attachment 209660 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209660&action=review > > > Source/JavaScriptCore/ChangeLog:17 > > + This is due to the fact that the lowest non-zero attribute (ReadOnly) is represented as 1 << 1 > > + instead of 1 << 0. We then calculate the default attributes as ((1 << 3) << 1) - 1, which is 0xF, > > + but only the top three bits mean anything. Even in the case above, the top three bits are set > > + to 0 but the bottom bit remains set, which causes us to think m_attributes is non-zero. > > I think the real problem is in the code that calculates the default attributes. Where is that code? Is there some reason it has to do "((1 << 3) << 1) - 1" instead of just or'ing together the specific attributes? Hmm, a couple of these constants are exposed as part of the JavaScriptCore framework's C API in JSObjectRef.h (e.g. kJSPropertyAttributeReadOnly). Perhaps it would be better to just change how the default attributes are calculated internally and leave the other values alone. 921132 5 mhahnenberg 2013-08-26 13:00:52 -0700 Committed r154630: <http://trac.webkit.org/changeset/154630> 921156 6 darin 2013-08-26 14:11:03 -0700 (In reply to comment #4) > Hmm, a couple of these constants are exposed as part of the JavaScriptCore framework's C API in JSObjectRef.h (e.g. kJSPropertyAttributeReadOnly). Please add a COMPILE_ASSERT to keep these in sync. 209660 2013-08-26 10:09:43 -0700 2013-08-26 10:14:27 -0700 Patch bug-120314-20130826100943.patch text/plain 2482 mhahnenberg SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTU0NjA5KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIzIEBA CisyMDEzLTA4LTI2ICBNYXJrIEhhaG5lbmJlcmcgIDxtaGFobmVuYmVyZ0BhcHBsZS5jb20+CisK KyAgICAgICAgT2JqZWN0LmRlZmluZVByb3BlcnR5IHNob3VsZCBiZSBhYmxlIHRvIGNyZWF0ZSBh IFByb3BlcnR5RGVzY3JpcHRvciB3aGVyZSBtX2F0dHJpYnV0ZXMgPT0gMAorICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTIwMzE0CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQ3VycmVudGx5IHdpdGggdGhlIHdh eSB0aGF0IGRlZmluZVByb3BlcnR5IHdvcmtzLCB3ZSBsZWF2ZSBhIHN0cmF5IGxvdyBiaXQgc2V0 IGluIAorICAgICAgICBQcm9wZXJ0eURlc2NyaXB0b3I6Om1fYXR0cmlidXRlcyBpbiB0aGUgZm9s bG93aW5nIGNvZGU6CisKKyAgICAgICAgdmFyIG8gPSB7fTsKKyAgICAgICAgT2JqZWN0LmRlZmlu ZVByb3BlcnR5KG8sIDEwMCwge3dyaXRhYmxlOnRydWUsIGVudW1lcmFibGU6dHJ1ZSwgY29uZmln dXJhYmxlOnRydWUsIHZhbHVlOiJmb28ifSk7CisgICAgICAgIAorICAgICAgICBUaGlzIGlzIGR1 ZSB0byB0aGUgZmFjdCB0aGF0IHRoZSBsb3dlc3Qgbm9uLXplcm8gYXR0cmlidXRlIChSZWFkT25s eSkgaXMgcmVwcmVzZW50ZWQgYXMgMSA8PCAxIAorICAgICAgICBpbnN0ZWFkIG9mIDEgPDwgMC4g V2UgdGhlbiBjYWxjdWxhdGUgdGhlIGRlZmF1bHQgYXR0cmlidXRlcyBhcyAoKDEgPDwgMykgPDwg MSkgLSAxLCB3aGljaCBpcyAweEYsIAorICAgICAgICBidXQgb25seSB0aGUgdG9wIHRocmVlIGJp dHMgbWVhbiBhbnl0aGluZy4gRXZlbiBpbiB0aGUgY2FzZSBhYm92ZSwgdGhlIHRvcCB0aHJlZSBi aXRzIGFyZSBzZXQgCisgICAgICAgIHRvIDAgYnV0IHRoZSBib3R0b20gYml0IHJlbWFpbnMgc2V0 LCB3aGljaCBjYXVzZXMgdXMgdG8gdGhpbmsgbV9hdHRyaWJ1dGVzIGlzIG5vbi16ZXJvLgorCisg ICAgICAgICogcnVudGltZS9Qcm9wZXJ0eVNsb3QuaDoKKwogMjAxMy0wOC0yNCAgRmlsaXAgUGl6 bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAgICAgIEZsb2F0VHlwZWRBcnJheUFkYXB0b3I6 OnRvSlNWYWx1ZSBzaG91bGQgYWxtb3N0IGNlcnRhaW5seSBub3QgdXNlIGpzTnVtYmVyKCkgc2lu Y2UgdGhhdCBhdHRlbXB0cyBpbnQgY29udmVyc2lvbnMKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0 Q29yZS9ydW50aW1lL1Byb3BlcnR5U2xvdC5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2Ny aXB0Q29yZS9ydW50aW1lL1Byb3BlcnR5U2xvdC5oCShyZXZpc2lvbiAxNTQ2MDgpCisrKyBTb3Vy Y2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9Qcm9wZXJ0eVNsb3QuaAkod29ya2luZyBjb3B5KQpA QCAtMzcsMTEgKzM3LDExIEBAIGNsYXNzIEdldHRlclNldHRlcjsKIC8vIFByb3BlcnR5IGF0dHJp YnV0ZXMKIGVudW0gQXR0cmlidXRlIHsKICAgICBOb25lICAgICAgICAgPSAwLAotICAgIFJlYWRP bmx5ICAgICA9IDEgPDwgMSwgIC8vIHByb3BlcnR5IGNhbiBiZSBvbmx5IHJlYWQsIG5vdCB3cml0 dGVuCi0gICAgRG9udEVudW0gICAgID0gMSA8PCAyLCAgLy8gcHJvcGVydHkgZG9lc24ndCBhcHBl YXIgaW4gKGZvciAuLiBpbiAuLikKLSAgICBEb250RGVsZXRlICAgPSAxIDw8IDMsICAvLyBwcm9w ZXJ0eSBjYW4ndCBiZSBkZWxldGVkCi0gICAgRnVuY3Rpb24gICAgID0gMSA8PCA0LCAgLy8gcHJv cGVydHkgaXMgYSBmdW5jdGlvbiAtIG9ubHkgdXNlZCBieSBzdGF0aWMgaGFzaHRhYmxlcwotICAg IEFjY2Vzc29yICAgICA9IDEgPDwgNSwgIC8vIHByb3BlcnR5IGlzIGEgZ2V0dGVyL3NldHRlcgor ICAgIFJlYWRPbmx5ICAgICA9IDEgPDwgMCwgLy8gcHJvcGVydHkgY2FuIGJlIG9ubHkgcmVhZCwg bm90IHdyaXR0ZW4KKyAgICBEb250RW51bSAgICAgPSAxIDw8IDEsIC8vIHByb3BlcnR5IGRvZXNu J3QgYXBwZWFyIGluIChmb3IgLi4gaW4gLi4pCisgICAgRG9udERlbGV0ZSAgID0gMSA8PCAyLCAv LyBwcm9wZXJ0eSBjYW4ndCBiZSBkZWxldGVkCisgICAgRnVuY3Rpb24gICAgID0gMSA8PCAzLCAv LyBwcm9wZXJ0eSBpcyBhIGZ1bmN0aW9uIC0gb25seSB1c2VkIGJ5IHN0YXRpYyBoYXNodGFibGVz CisgICAgQWNjZXNzb3IgICAgID0gMSA8PCA0LCAvLyBwcm9wZXJ0eSBpcyBhIGdldHRlci9zZXR0 ZXIKIH07CiAKIGNsYXNzIFByb3BlcnR5U2xvdCB7Cg==