[PATCH 0/2] MR6970: jscript: Move property allocation to update_external_prop.

[Jacek Caban (＠jacek)]

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6970

[PATCH 1/2] jscript: Move property allocation to update_external_prop.

[Jacek Caban]

From: Jacek Caban <jacek(a)codeweavers.com>

---
 dlls/jscript/dispex.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c
index 4e40cdf1bea..6569fdc8421 100644
--- a/dlls/jscript/dispex.c
+++ b/dlls/jscript/dispex.c
@@ -268,10 +268,15 @@ static dispex_prop_t *lookup_dispex_prop(jsdisp_t *obj, unsigned hash, const WCH
     return NULL;
 }
 
-static HRESULT update_external_prop(jsdisp_t *obj, dispex_prop_t *prop, const struct property_info *desc)
+static HRESULT update_external_prop(jsdisp_t *obj, const WCHAR *name, dispex_prop_t *prop, const struct property_info *desc, dispex_prop_t **ret)
 {
     HRESULT hres;
 
+    if(desc->name)
+        name = desc->name;
+    if(!prop && !(prop = alloc_prop(obj, name, PROP_DELETED, 0)))
+        return E_OUTOFMEMORY;
+
     if(!desc->iid) {
         prop->type = PROP_EXTERN;
         prop->u.id = desc->id;
@@ -305,6 +310,7 @@ static HRESULT update_external_prop(jsdisp_t *obj, dispex_prop_t *prop, const st
     }
 
     prop->flags = desc->flags & PROPF_ALL;
+    *ret = prop;
     return S_OK;
 }
 
@@ -327,12 +333,8 @@ static HRESULT find_external_prop(jsdisp_t *This, const WCHAR *name, BOOL case_i
                     return S_OK;
                 }
             }
-            if(!prop && !(prop = alloc_prop(This, desc.name ? desc.name : name, PROP_DELETED, 0)))
-                return E_OUTOFMEMORY;
 
-            hres = update_external_prop(This, prop, &desc);
-            *ret = prop;
-            return hres;
+            return update_external_prop(This, name, prop, &desc, ret);
         }else if(prop && prop->type == PROP_EXTERN) {
             prop->type = PROP_DELETED;
         }
@@ -456,13 +458,8 @@ static HRESULT ensure_prop_name(jsdisp_t *This, const WCHAR *name, DWORD create_
         if(This->builtin_info->lookup_prop) {
             struct property_info desc;
             hres = This->builtin_info->lookup_prop(This, name, fdexNameEnsure, &desc);
-            if(hres == S_OK) {
-                hres = update_external_prop(This, prop, &desc);
-                if(FAILED(hres))
-                    return hres;
-                *ret = prop;
-                return S_OK;
-            }
+            if(hres == S_OK)
+                return update_external_prop(This, name, prop, &desc, ret);
         }
 
         hres = S_OK;
@@ -765,10 +762,7 @@ static HRESULT fill_props(jsdisp_t *obj)
 
             prop = lookup_dispex_prop(obj, string_hash(desc.name), desc.name, FALSE);
             if(!prop) {
-                prop = alloc_prop(obj, desc.name, PROP_DELETED, 0);
-                if(!prop)
-                    return E_OUTOFMEMORY;
-                hres = update_external_prop(obj, prop, &desc);
+                hres = update_external_prop(obj, desc.name, NULL, &desc, &prop);
                 if(FAILED(hres))
                     return hres;
             }
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/6970

[PATCH 2/2] jscript: Add support for deleting host properties.

[Jacek Caban]

From: Jacek Caban <jacek(a)codeweavers.com>

---
 dlls/jscript/dispex.c    | 14 +++++++---
 dlls/mshtml/tests/es5.js | 55 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c
index 6569fdc8421..d5ed3adf4fb 100644
--- a/dlls/jscript/dispex.c
+++ b/dlls/jscript/dispex.c
@@ -274,12 +274,16 @@ static HRESULT update_external_prop(jsdisp_t *obj, const WCHAR *name, dispex_pro
 
     if(desc->name)
         name = desc->name;
-    if(!prop && !(prop = alloc_prop(obj, name, PROP_DELETED, 0)))
-        return E_OUTOFMEMORY;
 
     if(!desc->iid) {
+        if(!prop && !(prop = alloc_prop(obj, name, PROP_DELETED, 0)))
+            return E_OUTOFMEMORY;
         prop->type = PROP_EXTERN;
         prop->u.id = desc->id;
+    }else if(prop) {
+        /* If a property for a host non-volatile already exists, it must have been deleted. */
+        *ret = prop;
+        return S_OK;
     }else if(desc->flags & PROPF_METHOD) {
         jsdisp_t *func;
 
@@ -287,7 +291,8 @@ static HRESULT update_external_prop(jsdisp_t *obj, const WCHAR *name, dispex_pro
         if(FAILED(hres))
             return hres;
 
-        prop->type = PROP_JSVAL;
+        if(!(prop = alloc_prop(obj, name, PROP_JSVAL, 0)))
+            return E_OUTOFMEMORY;
         prop->u.val = jsval_obj(func);
     }else {
         jsdisp_t *getter, *setter = NULL;
@@ -304,7 +309,8 @@ static HRESULT update_external_prop(jsdisp_t *obj, const WCHAR *name, dispex_pro
             }
         }
 
-        prop->type = PROP_ACCESSOR;
+        if(!(prop = alloc_prop(obj, name, PROP_ACCESSOR, 0)))
+            return E_OUTOFMEMORY;
         prop->u.accessor.getter = getter;
         prop->u.accessor.setter = setter;
     }
diff --git a/dlls/mshtml/tests/es5.js b/dlls/mshtml/tests/es5.js
index e15e3f379cd..40458c103c9 100644
--- a/dlls/mshtml/tests/es5.js
+++ b/dlls/mshtml/tests/es5.js
@@ -2971,3 +2971,58 @@ sync_test("prototypes", function() {
     ok(document.body instanceof Element, "body is not an instance of Element");
     ok(document.body instanceof Node, "body is not an instance of Node");
 });
+
+sync_test("prototypes_delete", function() {
+    function check_prop(name) {
+        var orig = Object.getOwnPropertyDescriptor(Element.prototype, name);
+        ok(orig != undefined, "Could not get " + name + " descriptor");
+        var is_func = "value" in orig;
+
+        function check(obj, has_own, has_prop, has_enum, todo_enum) {
+            var r = obj.hasOwnProperty(name);
+            ok(r === has_own, obj + ".hasOwnProperty(" + name + ") returned " + r);
+            r = name in obj;
+            ok(r === has_prop, name + " in " + obj + " returned " + r);
+            r = check_enum(obj, name);
+            todo_wine_if(todo_enum).
+            ok(r === has_enum, "enumerating " + name + " in " + obj + "returned " + r);
+        }
+
+        check(document.body, false, true, true, is_func);
+        check(Element.prototype, true, true, true, is_func);
+        check(Node.prototype, false, false, false);
+
+        delete Element.prototype[name];
+        check(document.body, false, false, false);
+        check(Element.prototype, false, false, false);
+        check(Node.prototype, false, false, false);
+
+        Element.prototype[name] = -2;
+        Node.prototype[name] = -3;
+        ok(document.body[name] === -2, "document.body[" + name + "] = " + Element.prototype[name]);
+
+        check(document.body, false, true, true);
+        check(Element.prototype, true, true, true);
+        check(Node.prototype, true, true, true);
+
+        delete Element.prototype[name];
+        ok(document.body[name] === -3, "document.body[" + name + "] = " + Element.prototype[name]);
+        check(document.body, false, true, true);
+        check(Element.prototype, false, true, true);
+        check(Node.prototype, true, true, true);
+
+        delete Node.prototype[name];
+        check(document.body, false, false, false);
+        check(Element.prototype, false, false, false);
+        check(Node.prototype, false, false, false);
+
+        /* Restore the prop */
+        Object.defineProperty(Element.prototype, name, orig);
+        check(document.body, false, true, true, is_func);
+        check(Element.prototype, true, true, true, is_func);
+        check(Node.prototype, false, false, false);
+    }
+
+    check_prop("scrollLeft"); /* accessor prop */
+    check_prop("getBoundingClientRect"); /* function prop */
+});
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/6970

[PATCH 2/2] jscript: Add support for deleting host properties.

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=150263

Your paranoid android.


=== w8 (32 bit report) ===

mshtml:
script.c:4275: Test failed: OnResponse failed: 80004004

=== w1064v1507 (64 bit report) ===

mshtml:
script.c:1224: Test failed: L"/index.html?dom.js:document_lastModified: lastModified too far from navigationStart: 1323"

=== debian11b (64 bit WoW report) ===

user32:
win.c:4070: Test failed: Expected active window 0000000002E4017C, got 0000000000000000.
win.c:4071: Test failed: Expected focus window 0000000002E4017C, got 0000000000000000.

[PATCH 0/2] MR6970: jscript: Add support for deleting host properties.

[Gabriel Ivăncescu]

This looks fine for now, pretty neat way to fix it.

But I still don't think we can escape a revamp at some point. The 2 "largest" volatile objects I tested before were the storage, and doc/window, and they function differently. On doc and window, the volatile props (stuff like accessing DOM elements via props) are looked up after the prototypes, while on storage, they are looked up first, before even the prop on the object.

For example:

- localStorage.getItem("test") returns null (no volatile item).
- Object.defineProperty(localStorage, "test", ...) creates normal prop on it.
- localStorage.setItem("test", "blah") now it is `blah` (only way to retrieve the defined prop underneat is getOwnPropertyDescriptor)
- localStorage.removeItem("test") now reveals the original defined prop

Worse is that it also hijacks deletion, so delete acts like removeItem, but if you call delete now, it won't delete the defineProperty prop (since there's no setItem so nothing to remove).

For doc and window it's the opposite and if anything in the prototype chain has the name of an element id, it will be returned instead.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6970#note_89883

[PATCH 0/2] MR6970: jscript: Add support for deleting host properties. - approved

[Gabriel Ivăncescu]

This merge request was approved by Gabriel Ivăncescu.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6970