hello
world
'); + +$elements = $doc->getElementsByTagName('p'); +$elements->item(0); // Activate item cache +$doc->loadHTML('A
B
C
'); +var_dump($elements); +var_dump($elements->item(0)->textContent); // First lookup +var_dump($elements->item(2)->textContent); // Uses cache +var_dump($elements->item(1)->textContent); // Does not use cache + +echo "-- Remove cached item test --\n"; + +$doc = new DOMDocument(); +$doc->loadHTML('hello
world
!
'); + +$elements = $doc->getElementsByTagName('p'); +$item = $elements->item(0); // Activate item cache +var_dump($item->textContent); +$item->remove(); +// Now element 0 means "world", and 1 means "!" +unset($item); +$item = $elements->item(1); +var_dump($item->textContent); + +echo "-- Removal of cached item in loop test --\n"; + +$doc = new DOMDocument; +$doc->loadXML( 'hello
world
!
'); + +$elements = $doc->getElementsByTagName('p'); +$item = $elements->item(0); // Activate item cache +var_dump($elements->length); // Length not cached yet, should still compute +$item->remove(); +// Now element 0 means "world", and 1 means "!" +unset($item); +var_dump($elements->length); +$item = $elements->item(1); +var_dump($item->textContent); +$item = $elements->item(1); +var_dump($item->textContent); +$item = $elements->item(0); +var_dump($item->textContent); +$item = $elements->item(1); +var_dump($item->textContent); + +?> +--EXPECT-- +int(3) +int(2) +string(1) "!" +string(1) "!" +string(5) "world" +string(1) "!" diff --git a/ext/dom/tests/DOMDocument_liveness_caching_invalidation.phpt b/ext/dom/tests/DOMDocument_liveness_caching_invalidation.phpt new file mode 100644 index 0000000000000..eaa600a666c55 --- /dev/null +++ b/ext/dom/tests/DOMDocument_liveness_caching_invalidation.phpt @@ -0,0 +1,26 @@ +--TEST-- +DOMDocument liveness caching invalidation by textContent +--EXTENSIONS-- +dom +--FILE-- +createElement( 'root' ); +$doc->appendChild( $root ); +for ( $i = 0; $i < 5; $i++ ) { + $root->appendChild( $doc->createElement( 'e' ) ); +} + +$i = 0; + +foreach ( $doc->getElementsByTagName( 'e' ) as $node ) { + if ($i++ == 2) { + $root->textContent = 'overwrite'; + } + var_dump($node->tagName); +} +?> +--EXPECTF-- +string(1) "e" +string(1) "e" +string(1) "e" diff --git a/ext/dom/tests/DOMElement_getElementsByTagName_without_document.phpt b/ext/dom/tests/DOMElement_getElementsByTagName_without_document.phpt new file mode 100644 index 0000000000000..cf6acf46162a3 --- /dev/null +++ b/ext/dom/tests/DOMElement_getElementsByTagName_without_document.phpt @@ -0,0 +1,14 @@ +--TEST-- +Node list cache should not break on DOMElement::getElementsByTagName() without document +--EXTENSIONS-- +dom +--FILE-- +getElementsByTagName('b') as $x); + +?> +Done +--EXPECT-- +Done diff --git a/ext/libxml/libxml.c b/ext/libxml/libxml.c index 71ed3f911cca7..5af3443069aba 100644 --- a/ext/libxml/libxml.c +++ b/ext/libxml/libxml.c @@ -1163,8 +1163,14 @@ PHP_LIBXML_API int php_libxml_increment_node_ptr(php_libxml_node_object *object, object->node->_private = private_data; } } else { + if (UNEXPECTED(node->type == XML_DOCUMENT_NODE || node->type == XML_HTML_DOCUMENT_NODE)) { + php_libxml_doc_ptr *doc_ptr = emalloc(sizeof(php_libxml_doc_ptr)); + doc_ptr->cache_tag.modification_nr = 1; /* iterators start at 0, such that they will start in an uninitialised state */ + object->node = (php_libxml_node_ptr *) doc_ptr; /* downcast */ + } else { + object->node = emalloc(sizeof(php_libxml_node_ptr)); + } ret_refcount = 1; - object->node = emalloc(sizeof(php_libxml_node_ptr)); object->node->node = node; object->node->refcount = 1; object->node->_private = private_data; diff --git a/ext/libxml/php_libxml.h b/ext/libxml/php_libxml.h index de9b49d2ce3b6..785fd64ed2403 100644 --- a/ext/libxml/php_libxml.h +++ b/ext/libxml/php_libxml.h @@ -69,6 +69,16 @@ typedef struct _php_libxml_node_ptr { void *_private; } php_libxml_node_ptr; +typedef struct { + size_t modification_nr; +} php_libxml_cache_tag; + +/* extends php_libxml_node_ptr */ +typedef struct { + php_libxml_node_ptr node_ptr; + php_libxml_cache_tag cache_tag; +} php_libxml_doc_ptr; + typedef struct _php_libxml_node_object { php_libxml_node_ptr *node; php_libxml_ref_obj *document; @@ -81,6 +91,20 @@ static inline php_libxml_node_object *php_libxml_node_fetch_object(zend_object * return (php_libxml_node_object *)((char*)(obj) - obj->handlers->offset); } +static zend_always_inline void php_libxml_invalidate_node_list_cache(php_libxml_doc_ptr *doc_ptr) +{ +#if SIZEOF_SIZE_T == 8 + /* If one operation happens every nanosecond, then it would still require 584 years to overflow + * the counter. So we'll just assume this never happens. */ + doc_ptr->cache_tag.modification_nr++; +#else + size_t new_modification_nr = doc_ptr->cache_tag.modification_nr + 1; + if (EXPECTED(new_modification_nr > 0)) { /* unsigned overflow; checking after addition results in one less instruction */ + doc_ptr->cache_tag.modification_nr = new_modification_nr; + } +#endif +} + #define Z_LIBXML_NODE_P(zv) php_libxml_node_fetch_object(Z_OBJ_P((zv))) typedef void * (*php_libxml_export_node) (zval *object); From 02c07618958968bafeb62373d272c4587f6ab071 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 29 May 2023 17:11:18 +0200 Subject: [PATCH 02/18] Use match any for ns too (note: contains bugfix for which I'll create PR) --- ext/dom/php_dom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/dom/php_dom.c b/ext/dom/php_dom.c index 926c3a60c09ff..1fbf6bb3c0d9f 100644 --- a/ext/dom/php_dom.c +++ b/ext/dom/php_dom.c @@ -1240,7 +1240,7 @@ xmlNode *dom_get_elements_by_tag_name_ns_raw(xmlNodePtr nodep, char *ns, char *l while (nodep != NULL && (*cur <= index || index == -1)) { if (nodep->type == XML_ELEMENT_NODE) { if (local_match_any || xmlStrEqual(nodep->name, (xmlChar *)local)) { - if (ns_match_any || (!strcmp(ns, "") && nodep->ns == NULL) || (nodep->ns != NULL && xmlStrEqual(nodep->ns->href, (xmlChar *)ns))) { + if (ns_match_any || (ns[0] == '\0' && nodep->ns == NULL) || (nodep->ns != NULL && xmlStrEqual(nodep->ns->href, (xmlChar *)ns))) { if (*cur == index) { ret = nodep; break; From ff807d6997511f1e255ee4bb20721845a26ba1e9 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 29 May 2023 17:12:19 +0200 Subject: [PATCH 03/18] Update comment --- ext/dom/dom_iterators.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ext/dom/dom_iterators.c b/ext/dom/dom_iterators.c index 48323332d7646..3d1fe629c8cf8 100644 --- a/ext/dom/dom_iterators.c +++ b/ext/dom/dom_iterators.c @@ -210,7 +210,8 @@ static void php_dom_iterator_move_forward(zend_object_iterator *iter) /* {{{ */ curnode = (xmlNodePtr)((php_libxml_node_ptr *)intern->ptr)->node; curnode = curnode->next; } else { - /* Nav the tree evey time as this is LIVE */ + /* The collection is live, we nav the tree from the base object if we cannot + * use the cache to restart from the last point. */ basenode = dom_object_get_node(objmap->baseobj); if (php_dom_is_cache_tag_stale_from_node(&iterator->cache_tag, basenode)) { if (basenode && (basenode->type == XML_DOCUMENT_NODE || From a75c85a59b9f2aa2abfa41544feb8f650a30781c Mon Sep 17 00:00:00 2001 From: nielsdos <7771979+nielsdos@users.noreply.github.com> Date: Mon, 29 May 2023 23:39:12 +0200 Subject: [PATCH 04/18] Comments --- ext/dom/nodelist.c | 7 ++++- ...ocument_getElementsByTagName_liveness.phpt | 26 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/ext/dom/nodelist.c b/ext/dom/nodelist.c index 81da629fed814..021637c5e5cc5 100644 --- a/ext/dom/nodelist.c +++ b/ext/dom/nodelist.c @@ -34,6 +34,7 @@ static zend_always_inline void objmap_cache_release_cached_obj(dom_nnodemap_object *objmap) { if (objmap->cached_obj) { + /* Since the DOM is a tree there can be no cycles. */ if (GC_DELREF(&objmap->cached_obj->std) == 0) { zend_objects_store_del(&objmap->cached_obj->std); } @@ -227,7 +228,11 @@ PHP_METHOD(DOMNodeList, item) DOM_RET_OBJ(itemnode, &ret, objmap->baseobj); if (cache_itemnode) { /* Hold additional reference for the cache, must happen before releasing the cache - * because we might be the last reference holder. */ + * because we might be the last reference holder. + * Instead of storing and copying zvals, we store the object pointer directly. + * This saves us some bytes because a pointer is smaller than a zval. + * This also means we have to manually refcount the objects here, and remove the reference count + * in reset_objmap_cache() and the destructor. */ dom_object *cached_obj = Z_DOMOBJ_P(return_value); GC_ADDREF(&cached_obj->std); /* If the tag is stale, all cached data is useless. Otherwise only the cached object is useless. */ diff --git a/ext/dom/tests/DOMDocument_getElementsByTagName_liveness.phpt b/ext/dom/tests/DOMDocument_getElementsByTagName_liveness.phpt index 2bd02bc87034b..2b4622d10d389 100644 --- a/ext/dom/tests/DOMDocument_getElementsByTagName_liveness.phpt +++ b/ext/dom/tests/DOMDocument_getElementsByTagName_liveness.phpt @@ -10,6 +10,32 @@ $root = $doc->documentElement; $i = 0; +/* Note that the list is live. The explanation for the output is as follows: + Before the loop we have the following (writing only the attributes): + 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 + + Now the loop starts, the current element is marked with a V. $i == 0: + V + 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 + 1 gets printed. $i == 0, which is even, so 1 gets removed, which results in: + V + 2 3 4 5 6 7 8 9 10 11 12 13 14 15 + Note that everything shifted to the left. + Because the list is live, the current element pointer still refers to the first index, which now corresponds to element with attribute 2. + Now the foreach body ends, which means we go to the next element, which is now 3 instead of 2. + V + 2 3 4 5 6 7 8 9 10 11 12 13 14 15 + 3 gets printed. $i == 1, which is odd, so nothing happens and we move on to the next element: + V + 2 3 4 5 6 7 8 9 10 11 12 13 14 15 + 4 gets printed. $i == 2, which is even, so 4 gets removed, which results in: + V + 2 3 5 6 7 8 9 10 11 12 13 14 15 + Note again everything shifted to the left. + Now the foreach body ends, which means we go to the next element, which is now 6 instead of 5. + V + 2 3 5 6 7 8 9 10 11 12 13 14 15 + 6 gets printed, etc... */ foreach ($doc->getElementsByTagName('e') as $node) { print $node->getAttribute('i') . ' '; if ($i++ % 2 == 0) From dccd29c03e71f6f1952ab036a6a931df5a0d1df3 Mon Sep 17 00:00:00 2001 From: nielsdos <7771979+nielsdos@users.noreply.github.com> Date: Wed, 31 May 2023 19:44:39 +0200 Subject: [PATCH 05/18] Fix staleness check --- ext/dom/nodelist.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ext/dom/nodelist.c b/ext/dom/nodelist.c index 021637c5e5cc5..11cab96e41a1a 100644 --- a/ext/dom/nodelist.c +++ b/ext/dom/nodelist.c @@ -185,11 +185,12 @@ PHP_METHOD(DOMNodeList, item) bool restart = true; int relative_index = index; if (index >= objmap->cached_obj_index && objmap->cached_obj && !php_dom_is_cache_tag_stale_from_node(&objmap->cache_tag, nodep)) { - nodep = dom_object_get_node(objmap->cached_obj); + xmlNodePtr cached_obj_xml_node = dom_object_get_node(objmap->cached_obj); + /* The node cannot be NULL if the cache is valid. If it is NULL, then it means we * forgot an invalidation somewhere. Take the defensive programming approach and invalidate * it here if it's NULL (except in debug mode where we would want to catch this). */ - if (UNEXPECTED(nodep == NULL)) { + if (UNEXPECTED(cached_obj_xml_node == NULL)) { #if ZEND_DEBUG ZEND_UNREACHABLE(); #endif @@ -197,6 +198,7 @@ PHP_METHOD(DOMNodeList, item) } else { restart = false; relative_index -= objmap->cached_obj_index; + nodep = cached_obj_xml_node; } } if (objmap->nodetype == XML_ATTRIBUTE_NODE || objmap->nodetype == XML_ELEMENT_NODE) { From 737f8be45a0162763fddfa59d416c6ec82cce5e6 Mon Sep 17 00:00:00 2001 From: nielsdos <7771979+nielsdos@users.noreply.github.com> Date: Wed, 31 May 2023 20:01:38 +0200 Subject: [PATCH 06/18] Fix SimpleXML invalidations Test was Co-authored-by: tstarling --- ...tElementsByTagName_liveness_simplexml.phpt | 28 +++++++++++++++++++ ext/simplexml/simplexml.c | 16 +++++++++++ 2 files changed, 44 insertions(+) create mode 100644 ext/dom/tests/DOMDocument_getElementsByTagName_liveness_simplexml.phpt diff --git a/ext/dom/tests/DOMDocument_getElementsByTagName_liveness_simplexml.phpt b/ext/dom/tests/DOMDocument_getElementsByTagName_liveness_simplexml.phpt new file mode 100644 index 0000000000000..d1eda9a2271f3 --- /dev/null +++ b/ext/dom/tests/DOMDocument_getElementsByTagName_liveness_simplexml.phpt @@ -0,0 +1,28 @@ +--TEST-- +DOMDocument::getElementsByTagName() liveness with simplexml_import_dom +--EXTENSIONS-- +dom +--FILE-- +loadXML( 'Hello
+xinclude: book.xml not found
+Hello
+xinclude: book.xml not found
+ +