Closed Bug 1125624 Opened 10 years ago Closed 10 years ago

Merge DefineProperty with StandardDefineProperty

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox-esr38 - ---

People

(Reporter: evilpies, Assigned: jorendorff)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

part 1 - Implement ValidateAndApplyPropertyDescriptor steps 3-4, so that (once the corresponding code in StandardDefineProperty is deleted) freezing an already-frozen object with an addProperty class hook will not call the hook
6.87 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
part 2 - Change js::StandardDefineProperty to forward to js::DefineProperty
17.38 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
part 3 - Remove js::StandardDefineProperty and js::DefineOwnProperty
18.31 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
Everything else might be slightly wrong. Also not the ProperyDescriptor overload, but PropDesc version.
I'm taking over this bug. The answer is to only have one thing rather than two things.
Summary: Use StandardDefineProperty everywhere → Merge DefineProperty with StandardDefineProperty and PropDesc with PropertyDescriptor
This will follow some error-handling work.
Depends on: 1113369
Blocks: 1107443
Depends on: 1125437
Blocks: CVE-2015-4516
Depends on: 1133081
Depends on: 1142784
Depends on: 1142828
Summary: Merge DefineProperty with StandardDefineProperty and PropDesc with PropertyDescriptor → Merge DefineProperty with StandardDefineProperty
Depends on: 1144819
Depends on: 1143810
Depends on: 1147660
Depends on: 1138499
Depends on: 1148652
Depends on: 1148750
Depends on: 1140482
No longer blocks: CVE-2015-4478
Calling the hook is bad because sometimes the object is an XPConnect object with an addProperty hook that always throws. This patch contains two hacks I'm unsure about, both related to the same situation: NativeDefineProperty is called from JSOP_INITPROP after JSOP_NEWOBJECT. 1. In GetExistingPropertyValue, avoid calling GetExistingProperty in a case where that would trigger an assertion. An alternative might be to weaken the assertion so that it is effectively skipped if obj's group has Addendum_PreliminaryObjects. 2. In NativeDefineProperty, when redundant is true, we update the type even though neither the shape nor the value is changing! Without this hack, we get assertion failures in Ion later.
Attachment #8613185 - Flags: review?(jwalden+bmo)
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Attachment #8613188 - Flags: review?(jwalden+bmo)
Comment on attachment 8613185 [details] [diff] [review] part 1 - Implement ValidateAndApplyPropertyDescriptor steps 3-4, so that (once the corresponding code in StandardDefineProperty is deleted) freezing an already-frozen object with an addProperty class hook will not call the hook Review of attachment 8613185 [details] [diff] [review]: ----------------------------------------------------------------- Adding r?bhackett specifically for the "two hacks I'm unsure about" (see last 3 paragraphs of comment 9).
Attachment #8613185 - Flags: review?(bhackett1024)
Comment on attachment 8613185 [details] [diff] [review] part 1 - Implement ValidateAndApplyPropertyDescriptor steps 3-4, so that (once the corresponding code in StandardDefineProperty is deleted) freezing an already-frozen object with an addProperty class hook will not call the hook Review of attachment 8613185 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/NativeObject.cpp @@ +1212,5 @@ > + // Avoid failing a too-strict assertion in GetExistingProperty. > + if (shape->hasSlot() && shape->hasDefaultGetter()) { > + vp.set(obj->getSlot(shape->slot())); > + return true; > + } I think this is effectively neutering that early assertion in GetExistingProperty. What are the circumstances under which the assert is failing? Addendum_PreliminaryObjects shouldn't be involved here (it just indicates the object is one of the first ones created with the given group). In JSOP_NEWOBJECT objects the new objects are created with an initial set of properties which have undefined values. Those initial undefined values are not accounted for by TI, which is I guess why the assert is failing. Script will never be able to observe the values of these properties before they are assigned to, so we allow this, but it sounds like the assertion should be weakened for some calling context where the VM reads the value of the property when it is first assigned to? @@ +1376,5 @@ > + if (!IsImplicitDenseOrTypedArrayElement(shape) && desc.hasValue()) { > + if (!UpdateShapeTypeAndValue(cx, obj, shape, desc.value())) > + return false; > + } > + return result.succeed(); I would guess this is probably also involved with the issue described above. If the INITPROP is initializing a property with |undefined| then because of the initial properties on the NEWOBJECT object the DefineProperty will seem to be redundant, but the types on the ObjectGroup have not been updated to reflect that undefined value, so this should indeed update type information. Do you see this happening with any values other than |undefined|?
Attachment #8613185 - Flags: review?(bhackett1024)
Attachment #8613188 - Attachment is obsolete: true
Attachment #8613188 - Flags: review?(jwalden+bmo)
Blocks: 1170216
(In reply to Brian Hackett (:bhackett) from comment #13) > What are the circumstances under which the assert is > failing? This happens under JSOP_INITPROP, targeting an object created by JSOP_NEWOBJECT. JSOP_INITPROP calls js::NativeDefineProperty. This patch makes that call DefinePropertyIsRedundant, which calls GetExistingPropertyValue, which triggers the assertion. (I would rather not be doing any of this. Unfortunately, DefinePropertyIsRedundant is not just a particularly dumb optimization. See the first 2 paragraphs of comment 9. :-P) > Addendum_PreliminaryObjects shouldn't be involved here (it just > indicates the object is one of the first ones created with the given group). OK, then it just happens to be set because when we fail the assertion, it's the first time through. Red herring, sorry. > it sounds like the assertion should be > weakened for some calling context where the VM reads the value of the > property when it is first assigned to? Right. That's sort of what I'm trying to do here ... I don't see a better way to do it by e.g. adding a condition to the MOZ_ASSERT_IF. ...Maybe I could make the JSOP_NEWOBJECT machinery initialize the fields to a magic value rather than undefined, and skip the assertion when the slot contains that magic value. (?) > Do you see this happening with any values other than |undefined|? No. I agree with everything you wrote, and I think it only happens in the exact case you're describing.
Flags: needinfo?(bhackett1024)
(In reply to Jason Orendorff [:jorendorff] from comment #15) > (In reply to Brian Hackett (:bhackett) from comment #13) > > What are the circumstances under which the assert is > > failing? > > This happens under JSOP_INITPROP, targeting an object created by > JSOP_NEWOBJECT. > > JSOP_INITPROP calls js::NativeDefineProperty. This patch makes that call > DefinePropertyIsRedundant, which calls GetExistingPropertyValue, which > triggers the assertion. > > (I would rather not be doing any of this. Unfortunately, > DefinePropertyIsRedundant is not just a particularly dumb optimization. See > the first 2 paragraphs of comment 9. :-P) OK, how about adding the '// Avoid failing a too-strict assertion ...' block or similar code to DefinePropertyIsRedundant itself, to run as an alternative to GetExistingPropertyValue if possible. Then other callers to GetExistingPropertyValue will benefit from the assertion, and the assertion itself won't need to be changed.
Flags: needinfo?(bhackett1024)
OK, I'll do that. And improve the comment.
Comment on attachment 8613185 [details] [diff] [review] part 1 - Implement ValidateAndApplyPropertyDescriptor steps 3-4, so that (once the corresponding code in StandardDefineProperty is deleted) freezing an already-frozen object with an addProperty class hook will not call the hook Review of attachment 8613185 [details] [diff] [review]: ----------------------------------------------------------------- I kinda like the MagicValue(JS_UNINITIALIZED_OBJECT_PROPERTY) idea, maybe slightly more than the alternative thing. Seems like an uninitialized property sentinel could have more value than in just this one weird assertion case. But comment 16 is also okay enough. ::: js/src/vm/NativeObject.cpp @@ +1233,5 @@ > + if (desc.hasConfigurable() && desc.configurable() != ((shapeAttrs & JSPROP_PERMANENT) == 0)) > + return true; > + if (desc.hasEnumerable() && desc.enumerable() != ((shapeAttrs & JSPROP_ENUMERATE) != 0)) > + return true; > + if (desc.hasWritable() || desc.hasValue()) { Wasn't there an isDataDescriptor() method to abbreviate this? @@ +1246,5 @@ > + > + // The specification calls for SameValue here, but it seems to be a > + // bug. See <https://bugs.ecmascript.org/show_bug.cgi?id=3508>. > + if (desc.value() != currentValue) > + return true; Bah. Stab typed arrays repeatedly.
Attachment #8613185 - Flags: review?(jwalden+bmo) → review+
Still 2 more parts to land.
Keywords: leave-open
Attachment #8613187 - Flags: review?(jwalden+bmo) → review+
Attachment #8613582 - Flags: review?(jwalden+bmo) → review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1189744
Depends on: 1194282
Jason, do you want to request uplift for this to esr ? It could still make it into esr 38.3.0.
Flags: needinfo?(jorendorff)
No, this is way too big to uplift.
Flags: needinfo?(jorendorff)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: