HTML Processor may error processing nested HTML structures

[jonsurrell]

The HTML Processor (not the Tag Processor) may throw an error when processing HTML with very deep (>100 element) nesting:

<i><!-- …99 <i> tags… --><i>

It will throw Exception: could not allocate bookmark.

[dmsnell]

Is this specifically about nesting depth? Or it could occur also for documents with certain specific kinds of tags. I think a reasonable option in any case is to catch and abort in these cases, but we might want to examine the throwing behavior of the underlying Tag Processor 🤷‍♂️

There are discussions about ​capping nesting depth in the spec itself, so we should try to align over time with that. According to this discussion, there are already limits in Safari, Firefox, and Chrome. We could potentially adjust our arbitrary limit to match those.

Chrome&Firefox the max depth is 513, while in Safari it is 512

Also interesting from that issue is this note

at some point it stops nesting elements, and instead adds them at sibling. In all browser, there is some MAX_DEPTH, and if the stack of open elements has already a length of MAX_DEPTH then before pushing a new element to the stack they'll pop the previous one

Of course this isn’t specified behavior, but it certainly establishes a baseline for expectations we could follow.

[jonsurrell]

Nesting is the most obvious way for the error to happen. Internally, each element on the stack of open elements gets a bookmark. When nesting causes the stack to have more bookmarks than are allowed, this error is triggered.

Internal and user bookmarks are shared, so if many bookmarks are being set externally it would lower the nesting threshold for triggering this error.

There are also bookmarks associated with virtual tokens, but that's a form of nesting. For example <table><td> is apparently 2 tags, but when processed via ::create_fragment() this actually consumes a maximum of 6 bookmarks I believe. 2 bookmarks for the root and context (HTML > BODY) then 4 more elements (TABLE > TBODY (virtual) > TR (virtual) > TD).

Bookmark exhaustion, typically from deep nesting, can cause the HTML Processor to throw an Exception.

The Exception is thrown by a private method, handle the exception and return false to indicate a failure to process.

Trac ticket: https://core.trac.wordpress.org/ticket/64394

[dmsnell]

For example <table><td> is apparently 2 tags, but when processed via ::create_fragment() this actually consumes a maximum of 6 bookmarks

This would correspond with the actual DOM nesting though, so I think that’s fine.

Even if we leave some buffer, for example, setting a MAX_BOOKMARKS count of something like 600 or 1000, we will then have plenty of opportunity to parse all documents a browser would parse.

The bookmark limit was always meant to be tunable and mostly serve to prevent infinite loops and needlessly wasteful code. There is no intrinsic constraint they are guarding.

[jonsurrell]

I agree, it seems like MAX_BOOKMARKS can be increased. However, I'd keep that work separate from this ticket.

Bookmark exhaustion has been observed on fairly standard WordPress post content. This is partially related to ​WordPress/gutenberg#70542 which may nest formatting elements to extreme depths.

The initial limit of 100 bookmarks was very conservative. A significant increase in bookmarks seems to perform well without excessive memory usage.

Related to ​#10616.

I definitely don’t love how pervasive and deep our error-checking has become here.

Me neither 😕

did you attempt to use ->bail()?

I did try using bail(). That transforms a bookmark exhaustion error into an unsupported error, which seemed confusing. When bookmarks are exhausted, an error is set. Bail overrides with an unsupported error code.

We could consider having bail leave the last error in case it's already been set.

what if we continue to throw inside bookmark_token() but trap that and handle aborting the same way we handle all deep control flow issues?

I have that in an earlier commit. It wasn't as pervasive (at b02d6793b86f8c982401cb367ce5cd1d90ce5831):

​https://github.com/WordPress/wordpress-develop/blob/b02d6793b86f8c982401cb367ce5cd1d90ce5831/src/wp-includes/html-api/class-wp-html-processor.php#L1043-L1050
​https://github.com/WordPress/wordpress-develop/blob/b02d6793b86f8c982401cb367ce5cd1d90ce5831/src/wp-includes/html-api/class-wp-html-processor.php#L1157-L1169

I have that in an earlier commit. It wasn't as pervasive

Why did you change it @sirreal? did I ask you to? 🙃 🤦‍♂️

No, I was just iterating and exploring. The method actually already had a …|false return type annotation, suggesting it may have intended to return false on failure.

I can't say I like the general Exception catching either, but it works. I don't have a strong preference, would you prefer that other version @dmsnell?

What about increasing \WP_HTML_Tag_Processor::MAX_BOOKMARKS as well?

The issue is more relevant on the HTML Processor because it creates its own bookmarks for each token as it processes them. The Tag Processor has no reason to do so, so a small limit is a reasonable guard for developers who create bookmarks in a loop, e.g. $processor->set_bookmark( "item-{$i}" );. If they need more they can subclass.

We want to make sure we can properly parse the HTML documents in the HTML Processor, which requires more internal bookmarks. @sirreal did explore bypassing this issue entirely by creating what is effectively a separate namespace for internal bookmarks. Given that we chose this value somewhat arbitrarily and saw in practice that it was simply too low, I think it’s pragmatic to update only here.

@sirreal is there a copy of the exception-catching version available?

what do you not like about it, or prefer about adding |false to all of the methods?

is there a copy of the exception-catching version available?

@dmsnell ​This diff should give an idea

what do you not like about it, or prefer about adding |false to all of the methods?

I don't feel strongly one way or the other.

Relying on error handling does reduce the checks required. My biggest concern is that it depends on methods being called inside of try/catch structures, which is easy to overlook. Handling a false return also requires some discipline, but at least the check is local and doesn't require understanding a call stack.

Some background to consider (all considering trunk):

• bookmark_token() claims to have both behaviors (throw + return false) suggesting confusion or oversight. It's one or the other, currently throw. ​https://github.com/WordPress/wordpress-develop/blob/f4f33f7eb8bb283de2b5f985c3c119f7e2d0c19d/src/wp-includes/html-api/class-wp-html-processor.php#L5097-L5107
• insert_html_element() and insert_foreign_element() never fail because the token already exists with an allocated bookmark. ​https://github.com/WordPress/wordpress-develop/blob/f4f33f7eb8bb283de2b5f985c3c119f7e2d0c19d/src/wp-includes/html-api/class-wp-html-processor.php#L6242-L6244
• insert_virtual_node() allocates a bookmark in order to construct a token. Virtual tokens are not "discovered" in the document, but arise from HTML parsing rules. ​https://github.com/WordPress/wordpress-develop/blob/f4f33f7eb8bb283de2b5f985c3c119f7e2d0c19d/src/wp-includes/html-api/class-wp-html-processor.php#L6295-L6303

---

The main thing I disliked was the general Exception catching that was required. It's possible for the last_error to be set and another Exception to arise:

​https://github.com/WordPress/wordpress-develop/blob/b02d6793b86f8c982401cb367ce5cd1d90ce5831/src/wp-includes/html-api/class-wp-html-processor.php#L1043-L1050
​https://github.com/WordPress/wordpress-develop/blob/b02d6793b86f8c982401cb367ce5cd1d90ce5831/src/wp-includes/html-api/class-wp-html-processor.php#L1157-L1169

Perhaps that check should have compared the known message:
​https://github.com/WordPress/wordpress-develop/blob/b02d6793b86f8c982401cb367ce5cd1d90ce5831/src/wp-includes/html-api/class-wp-html-processor.php#L5125

I also considered introducing another HTML API Exception class, but I'm reluctant to introduce another exception class for this.

@sirreal did explore bypassing this issue entirely by creating what is effectively a separate namespace for internal bookmarks

One clarification, I don't believe that was in this codebase but in the Rust port.

I do like the idea of keeping internal and external bookmarks separate. Internal bookmarks could then also use numeric arrays and likely most bookmark operations would be push/pop operations on a stack with potential performance implications. It's a rainy day idea 🙂

:shipit: getting this in will practically eliminate the need for #10616. while it’s good to continue on that one, it will be better to have some time to think about it without worrying that stuff is crashing

The exception-throwing and catching version definitely suits me more, notably because it avoids exporting these details to the callers.

I also think this is a middle-path to excluding internal bookmarks, so spreading out the protection throughout the class seems like it’s going to be code we then have to find and remove later (though maybe the recent PHPStan changes will help surface those cases).

which is easy to overlook

If our goal is to never fail _under normal usage_ and the failed bookmarks are only there to prevent infinite loops or memory-abuse, then perhaps we add a test to ensure that setting the bookmark fails and also that it doesn’t throw.

[desrosj]

In 61726:

Build/Test Tools: Exclude gutenberg directory when running PHPCS.

The relevant files will be scanned within src (provided the build script has been run).

See #64394.

[desrosj]

Apologies, [61726] was intended for #64393.

The exception-throwing and catching version definitely suits me more, notably because it avoids exporting these details to the callers.

I've restored the throw/catch version of these change.

perhaps we add a test to ensure that setting the bookmark fails and also that it doesn’t throw.

The PR includes tests like that. They test the processing behavior without testing the bookmark functions directly.

@dmsnell Thanks, I've reworked some of the testing based on your feedback.

I do plan to land ​https://github.com/WordPress/wordpress-develop/pull/10820 to increase the max bookmarks as well.

[jonsurrell]

In 61755:

HTML API: Prevent bookmark exhaustion from throwing.

When bookmark exhaustion occurs during processing, return false instead of throwing an Exception.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/10616.

Props jonsurrell, westonruter, dmsnell.
Fixes #64394.

[jonsurrell]

In 61756:

HTML API: Increase HTML Processor bookmark limit to 10,000.

The limit was overly conservative and bookmark exhaustion may occur on realistic documents. The increased limit allows documents with much greater depth to be safely processed.

The limit is increased from 100 to 10,000.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/10820.

Follow-up to [61755].

Props jonsurrell, dmsnell, westonruter.
See #64394.

Merged in r61756.