Closed Bug 864535 Opened 7 years ago Closed 7 years ago

CGDOMJSProxyHandler_delete needs changes in response to bug 858677

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: Waldo, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

There are a bunch of "XXX we should throw if Throw is true" comments with various wordings, now.  After bug 858677, they're no longer accurate: if deletion should throw when Throw is true, that should be signaled by setting |*bp = false|.  The JS engine is now responsible for throwing an error when the property being deleted was un-deletable.

Much of this, I think, can be solved by just deleting comments.  But I don't quite know enough of the WebIDL deletion semantics to fix this myself, without bogging down somewhat in spec language.
The semantics are as follows, though note that the spec might be wrong on this stuff:

When [[Delete]] is invoked with a property name that is a supported property index, if there is no indexed deleter on the interface, or if the deleting operation failed, or if the deleting operation was declared with a boolean return value and returned false, then Reject.

When [[Delete]] was invoked with a property name that is not an index and there is no named deleter or deletion failed or the deleting operation has a boolean retval and returned false, Reject.

Reject is defined as "If Throw is true, then throw a TypeError exception, otherwise return false".

Note that in ES5 there's a return value for [[Delete]] and also a Throw argument....

How do those map onto the SpiderMonkey signature after bug 858677?
Flags: needinfo?(jwalden+bmo)
This is one of those cases where it's unfortunate WebIDL tracks ES5 and not ES6.  (Although mostly that'd be its own unstable-foundation barrel of fun.)  In ES6, the return value and Throw parameter are smashed into one -- because in ES5, whenever a Reject happens during deletion, the fallthrough behavior is to return false.

Basically any place you see "Reject", just return false.  And any place you see "return true", keep doing that.  So I think the semantics should be:

""""
When [[Delete]] is invoked with a property name that is a supported property index, if there is no indexed deleter on the interface, or if the deleting operation failed, or if the deleting operation was declared with a boolean return value and returned false, then return false.

When [[Delete]] was invoked with a property name that is not an index and there is no named deleter or deletion failed or the deleting operation has a boolean retval and returned false, return false.
"""
Flags: needinfo?(jwalden+bmo)
All the comments are in places where *bp is getting set to false as needed already
Attachment #740619 - Flags: review?(jwalden+bmo)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Comment on attachment 740619 [details] [diff] [review]
This should do the trick

Review of attachment 740619 [details] [diff] [review]:
-----------------------------------------------------------------

In a sane world you could cut down on over-bracing a bit here, but unfortunately Gecko is not a sane world.  ;-)
Attachment #740619 - Flags: review?(jwalden+bmo) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/be8219e1ae5f
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/be8219e1ae5f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
So does that mean we throw in strict more now? Test for that would be nice.
Flags: in-testsuite- → in-testsuite?
Afaict we now throw in both modes on attempts to delete a named property when there is no named deleter.  At least based on code inspection; I have not tried the experiment.  ;)
That doesn't sound right.  If there's no deleter then you're supposed to Reject.  That returns false in non-strict mode code, which means |delete objectSupportingNamedPropertiesWithNoDeleter.prop| should evaluate to false in non-strict code, not throw.  Or am I mis-parsing somehow?
Oh, maybe I was misreading the JS side of this.  The binding code just returns false either way, and I thought that caused the JS side to always throw, but that might have been only true in ES5.
Both ES5 and ES6 effectively say that in non-strict code, deletion won't throw.  (Unless the delete op itself throws something unusual.)  The Throw/Reject stuff changed, but not in a way that introduced an semantic differences.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.