Declare dynamic properties in ext/dom

[kocsismate]

No description provided.

[kocsismate]

I've just tried to properly declare the dynamic properties. I'm not exactly sure when I should set explicit default values. For now, I only did it for untyped property which are not nullable.

[kocsismate]

I found the cause of the assertion errors (ZEND_ASSERT(info & (1 << type)); in ext/opcache/jit/zend_jit_trace.c:340) during the tracing JIT test run: a lot of property reader functions call php_dom_throw_error(INVALID_STATE_ERR, 0); but despite the function name, only a warning is emitted when the 2nd parameter is 0, so I had to make a bunch of properties nullable.

Now, only 2 tests fail:

• ext/dom/tests/bug28721.phpt: Assertion failed: (ret > 2), function zend_accel_get_type_map_ptr, file ./ext/opcache/zend_persist.c, line 326.
• ext/dom/tests/bug43364.phpt: Leaks when JIT is turned on :S

I appreciate any help!

[kocsismate]

Now, magically only the ext/dom/tests/bug43364.phpt JIT test fails with:

[Sun Feb 28 21:09:34 2021]  Script:  '/home/vsts/work/1/s/ext/dom/tests/bug43364.php'
/home/vsts/work/1/s/Zend/zend_objects_API.h(91) :  Freeing 0x00007f386c45eaf0 (80 bytes), script=/home/vsts/work/1/s/ext/dom/tests/bug43364.php
Last leak repeated 10 times
=== Total 11 memory leaks detected ===

Fortunately, I found some trace: the leak only occurs when the DOMNode::$childNodes property is typed, there is no issue otherwise.

[nikic]

The leak should be fixed by 82622d7.

[nikic]

I found the cause of the assertion errors (ZEND_ASSERT(info & (1 << type)); in ext/opcache/jit/zend_jit_trace.c:340) during the tracing JIT test run: a lot of property reader functions call php_dom_throw_error(INVALID_STATE_ERR, 0); but despite the function name, only a warning is emitted when the 2nd parameter is 0, so I had to make a bunch of properties nullable.

I think you can elevate all INVALID_STATE_ERR to use strict_error=1 (maybe best as a separate PR). If I understood correctly, this basically corresponds to the usual "uninitialized object" condition, e.g. when you try to create certain DOM objects without going through the DOM API. In line with usual guidelines, it's fine to always throw in that case.

[kocsismate]

Thanks for the fix!

I think you can elevate all INVALID_STATE_ERR to use strict_error=1 (maybe best as a separate PR). If I understood correctly, this basically corresponds to the usual "uninitialized object" condition, e.g. when you try to create certain DOM objects without going through the DOM API.

Yes, I think so as well.

In line with usual guidelines, it's fine to always throw in that case.

I'm happy to do it! I thought that the ship had already sailed for promoting these errors, so I didn't propose this :) I'll provide a separate PR after this one is merged.

[kocsismate]

This one is also fairly ready. @beberlei Do you have time for a longer review? ^^

[beberlei]

@kocsismate I skimmed it and am delighted that it works even though DOM properties are "virtual" and not real, so if all tests pass from me 👍

[nikic]

Looks reasonable, though I haven't checked all the types in detail.

For the read-write cases, we should probably introduce the same type checks we did elsewhere (mysqli/snmp).

[kocsismate]

For the read-write cases, we should probably introduce the same type checks we did elsewhere (mysqli/snmp).

Yeah, I want to add this along with the migration to typed properties in a follow-up PR.