Closed Bug 1297304 Opened 3 years ago Closed 3 years ago

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

Categories

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

23 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: d, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(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:

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
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.
Blocks: 855971
Status: UNCONFIRMED → NEW
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: 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.
Status: NEW → RESOLVED
Closed: 3 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 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?
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 <cam@mcc.id.au>
  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 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.
See Also: → 1297411
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4b998ebc50
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
Status: REOPENED → ASSIGNED
Note that this patch implements the spec behavior after https://github.com/heycam/webidl/pull/174
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 bzbarsky@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/3c60fe237a35
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 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.