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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
Attachments
(1 file)
32.46 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter 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 1•13 years ago
|
||
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+
Assignee | ||
Comment 2•13 years ago
|
||
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
Comment 3•13 years ago
|
||
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 → ---
Comment 4•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Comment 5•13 years ago
|
||
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 6•13 years ago
|
||
Comment 7•13 years ago
|
||
This was merged to m-c as part of merge changeset d871849ac3a3 on April 27. Should this now be FIXED?
Assignee | ||
Comment 9•13 years ago
|
||
Hm, yes.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•