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

Status: RESOLVED FIXED

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

Description

[Domenic Denicola]

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:

http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4410


Actual results:

Logs foo, foo, foo


Expected results:

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

- Defining: https://heycam.github.io/webidl/#defineownproperty
  - We eventually reach step 4 and define the property using OrdinaryDefineOwnProperty
- Accessing (the first time): https://heycam.github.io/webidl/#getownproperty-guts
  - 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: https://heycam.github.io/webidl/#delete
  - 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 https://heycam.github.io/webidl/#getownproperty-guts 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

Comment 1

[Tooru Fujisawa [:arai]]

Here's regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c8e47b184aba&tochange=b109e2dbf03b

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`.

https://dxr.mozilla.org/mozilla-central/rev/24763f58772d45279a935790f732d80851924b46/dom/html/nsHTMLDocument.cpp#2226
> 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;


https://dxr.mozilla.org/mozilla-central/rev/24763f58772d45279a935790f732d80851924b46/dom/html/nsHTMLDocument.cpp#2188
> 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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

As far as I can tell our behavior is following the spec.  The reasoning in comment 0 is incorrect at this point:

>- Deleting: https://heycam.github.io/webidl/#delete
>  - The named property visibility algorithm returns false since O has an own property named P

The named property visibility algorithm returns true in http://heycam.github.io/webidl/#dfn-named-property-visibility step 4, because Document is [OverrideBuiltins].  Step 5, which looks at own properties, is never reached.

So http://heycam.github.io/webidl/#delete 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 https://github.com/heycam/webidl/issues/149 on that).  Step 3 and in particular 3.2 is never reached.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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 http://heycam.github.io/webidl/#getownproperty lands in http://heycam.github.io/webidl/#getownproperty-guts and that will also call http://heycam.github.io/webidl/#dfn-named-property-visibility 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 http://heycam.github.io/webidl/#dfn-named-property-visibility 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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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 <[e-mail]>
  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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I filed bug 1297411 on the fact that gets on [OverrideBuiltins] interfaces in Gecko don't match the spec.

Comment 6

[Domenic Denicola]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Reopening, because I've come to the conclusion that the spec is broken.  See https://github.com/heycam/webidl/issues/152

Once that's sorted out, either this bug or bug 1297411 will be invalid, but it's not clear which one yet.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4b998ebc50
Add operator== overloads for comparing HandleRefPtrs to their raw Handles. r=bholley

Comment 9

[Cameron McCormack (:heycam)]

(Er, that was completely the wrong bug.)

Comment 10

[Boris Zbarsky [:bzbarsky]]

You should put the right bug in here, so people coming from the commit will find it...

Comment 11

[Tooru Fujisawa [:arai]]

comment #8 was for bug 1294161

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix our named property DOM proxy code to handle deletion correctly when expandos are involved

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=773a12803335

Comment 14

[Boris Zbarsky [:bzbarsky]]

Note that this patch implements the spec behavior after https://github.com/heycam/webidl/pull/174

Comment 15

[Peter Van der Beken [:peterv]]

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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c60fe237a35
Fix our named property DOM proxy code to handle deletion correctly when expandos are involved.  r=peterv

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c60fe237a35