Closed Bug 745944 Opened 13 years ago Closed 13 years ago

Accessorize PropDesc, make those accessors assert correct use (that attributes are present before being checked)

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
In the spec, the internal Property Descriptor structure can have [[Writable]], [[Configurable]], [[Value]], etc. fields -- but those fields need not actually be present. And, indeed, semantics sometimes vary if fields are absent, versus present with their default values. So PropDesc must contain has-attribute fields to verify this. And it only makes sense to check the values of the attributes if those attributes are present. This is pretty much a perfect use case for C++ accessors that assert correct access, so let's convert the existing code over to them, so we know we're using property descriptors correctly. This passes JS/JIT/JSAPI tests, although I wouldn't be surprised if a fuzzer picks out a bug or two more. But it's a start in the right direction, definitely.
Attachment #615463 - Flags: review?(jorendorff)
Comment on attachment 615463 [details] [diff] [review] Patch >- * Since [[Configurable]] defaults to false, we don't need to check >- * blah blah blah blah blah blah blah blah blah fnord blah >- * field is not present. Perfectly pellucid logic, eh? Er, yes. Thank you for deleting this. >+PropDesc::unwrapInto(JSContext *cx, Debugger *dbg, JSObject *obj, PropDesc *unwrapped) const >+{ >+ new (unwrapped) PropDesc(*this); No bug, but since the caller passes a pointer to an already-constructed PropDesc, it seems better to say: *unwrapped = *this; I like these better as static functions in Debugger.cpp than as methods of PropDesc in some other file. If you can see a nice easy way to support that, please do. Offhand, you might have a member function template template <class Fn> bool PropDesc::mapValues(JSContext *cx, Fn fn, bool regenerateDescObject, PropDesc *outp) const; but maybe it's a pain to use such a thing. Anyway, if you don't see an easy fix, that's ok. While you're renaming these two functions, please rename unwrapInto to unwrapDebuggerObjectsInto or something equally hideous. I should have named them so as to avoid that false sense of symmetry in the first place; the two methods are not opposites at all. >+ * Rewrap *idp and the fields of *desc for the current compartment. Also: >+ * defining a property on a proxy requires the pd field to contain a descriptor >+ * object, so reconstitute desc->pd if needed. Perhaps change pd to pd_ or pd().
Attachment #615463 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/91b9cba098f8 Assigning directly does seem nicer than placement-new, changed that. The Debugger bits I'm not super-happy about, but I think those should get much nicer as direct proxies and other work progresses, so I'm inclined to leave it as-is for now. (Except for the renaming, which I did.) Changed to pd_ since it's working on the field, and pd() is just an accessor that direct proxies will be killing soon, I believe.
Target Milestone: --- → mozilla15
Push backed out for win debug bustage, which persisted even after the original partial backout: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=718ced982de1 https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb6904fa2cf
Target Milestone: mozilla15 → ---
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This was merged to m-c as part of merge changeset d871849ac3a3 on April 27. Should this now be FIXED?
Thoughts on comment 7?
Hm, yes.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: