Closed Bug 1297304 Opened 7 years ago Closed 7 years ago

Deleting named properties that have been previously set by a user doesn't appear to work correctly


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

23 Branch
Not set



Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed


(Reporter: d, Assigned: bzbarsky)


(Blocks 1 open bug)


(Keywords: regression)


(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160604131506

Steps to reproduce:

Actual results:

Logs foo, foo, foo

Expected results:

Logs foo, foo, HTMLImageElement. My reasoning is as follows:

- Defining:
  - We eventually reach step 4 and define the property using OrdinaryDefineOwnProperty
- Accessing (the first time):
  - The named property visibility algorithm returns false since O has an own property named P
  - Thus we fall back to OrdinaryGetOwnProperty
- Accessing (the second time): same as before
- Deleting:
  - The named property visibility algorithm returns false since O has an own property named P
  - The property is configurable, so step 3.2 applies, removing the property! I guess this means OrdinaryDelete.
- Accessing (the third time)
  - Now we're back in but we get different answers
  - The named property visibility algorithm should return true
  - "Determine the value of a named property" should get the image element
  - We should get back a prop desc containing the image element
Here's regression range:

apparently a regression from bug 855971 (or perhaps the spec was changed after that?), or bug 820657 part.

Looks like it's an issue in either HTMLDocumentBinding.cpp or nsHTMLDocument.cpp.

In HTMLDocumentBinding.cpp, `HTMLDocumentBinding::DOMProxyHandler::delete_` calls `nsHTMLDocument::NamedGetter` before calling `dom::DOMProxyHandler::delete_`.

> bool
> DOMProxyHandler::delete_(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id, JS::ObjectOpResult& opresult) const
> {
> ...
>     self->NamedGetter(cx, NonNullHelper(Constify(name)), found, &result, rv);
> ...
>   deleteSucceeded = !found;
>   if (found) {
>     return deleteSucceeded ? opresult.succeed() : opresult.failCantDelete();
>   }
>   return dom::DOMProxyHandler::delete_(cx, proxy, id, opresult);

`nsHTMLDocument::NamedGetter` doesn't check own property at all, and it finds an element named "imgname", and returns with `found==true`.
> void
> nsHTMLDocument::NamedGetter(JSContext* cx, const nsAString& aName, bool& aFound,
>                             JS::MutableHandle<JSObject*> aRetval,
>                             ErrorResult& rv)
> {
>   nsWrapperCache* cache;
>   nsISupports* supp = ResolveName(aName, &cache);
>   if (!supp) {
>     aFound = false;
>     aRetval.set(nullptr);
>     return;
>   }
> ...
>   aFound = true;
> nsISupports*
> nsHTMLDocument::ResolveName(const nsAString& aName, nsWrapperCache **aCache)
> {
>   nsIdentifierMapEntry *entry = mIdentifierMap.GetEntry(aName);
> ...
>   if (length > 0) {
>     if (length == 1) {
>       // Only one element in the list, return the element instead of returning
>       // the list.
>       nsIContent *node = list->Item(0);
>       *aCache = node;
>       return node;
>     }

So `HTMLDocumentBinding::DOMProxyHandler::delete_` doesn't reach `dom::DOMProxyHandler::delete_` if there's an element with same name.
Blocks: 855971
Component: Untriaged → DOM
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
See Also: → 820657
Flags: needinfo?(peterv)
As far as I can tell our behavior is following the spec.  The reasoning in comment 0 is incorrect at this point:

>- Deleting:
>  - The named property visibility algorithm returns false since O has an own property named P

The named property visibility algorithm returns true in step 4, because Document is [OverrideBuiltins].  Step 5, which looks at own properties, is never reached.

So step 2 is entered, there is no named property deleter, so [[Delete]] returns false (though the "return" before false seems to have gotten lost somewhere; I filed on that).  Step 3 and in particular 3.2 is never reached.
Closed: 7 years ago
Flags: needinfo?(peterv)
Resolution: --- → INVALID
That said, something here is fishy.  Per spec as currently written, the second and third log statements should log HTMLImageElement, it looks like.  That's because lands in and that will also call and get true.  And then the image should be returned.

That is in fact the behavior I see in Safari, but not what we implement....  Chrome does something that matches neither Gecko nor Safari (showing the named prop at the second log statement, but undefined at the third one).  Edge does the "expected results" from comment 0, which corresponds to what would happen if steps 4 and 5 of were switched.

Cameron, do you recall what the intent was here in terms of interaction of own props and named props in the overridebuiltins case?
Looks to me like the current ordering in the named property visibility algorithm has been the same since this commit:

  commit 9fb3b63070a79baf6ba9ea7333dcb941929c3dac
  Author: Cameron McCormack <>
  Date:   Fri Jun 17 12:35:49 2011 +1000

    Make indexed and named properties be exposed on objects through [[GetOwnProperty]].

which is basically ever since we had the thing....  Needinfo'ing Cameron for real, in case he does recall what the thinking was.
Flags: needinfo?(cam)
I filed bug 1297411 on the fact that gets on [OverrideBuiltins] interfaces in Gecko don't match the spec.
Apologies, I missed the [OverrideBuiltins]. Glad I found something interesting though.

This came up in the course of trying to align Chrome's document bindings with the spec, FYI.
Reopening, because I've come to the conclusion that the spec is broken.  See

Once that's sorted out, either this bug or bug 1297411 will be invalid, but it's not clear which one yet.
See Also: → 1297411
Resolution: INVALID → ---
Pushed by
Add operator== overloads for comparing HandleRefPtrs to their raw Handles. r=bholley
(Er, that was completely the wrong bug.)
You should put the right bug in here, so people coming from the commit will find it...
Assignee: nobody → bzbarsky
Note that this patch implements the spec behavior after
Comment on attachment 8789880 [details] [diff] [review]
Fix our named property DOM proxy code to handle deletion correctly when expandos are involved

Review of attachment 8789880 [details] [diff] [review]:

Thanks for fixing this up and writing the tests.
Attachment #8789880 - Flags: review?(peterv) → review+
Pushed by
Fix our named property DOM proxy code to handle deletion correctly when expandos are involved.  r=peterv
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Flags: needinfo?(cam)
Version: 47 Branch → 23 Branch
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.