Changing component doesn't seem related to mediawiki ui

More details at https://www.mediawiki.org/wiki/Talk:HHVM#Big_Pages_and_Lua_Timeout

(and I'm going to trim out the CC's of the old component.)

• Bug 71515 has been marked as a duplicate of this bug. ***

Possibly related: https://bugzilla.wikimedia.org/show_bug.cgi?id=71519

I couldn't reproduce with an anonymous test script or with my own user account, but with Ori's test script, including login cookies, I reproduced it and got a gdb backtrace. It is infinite (or at least very deep) recursion in serialize(), causing a segfault in HHVM.

The issue is that the peculiar structure of a PPNode_Hash_Tree has references nested very deeply, potentially hundreds of thousands of levels. In the case of

https://de.wikipedia.org/w/index.php?curid=5622355

the resulting PPNode_Hash_Tree for the article itself has nesting at least 13500 levels deep. serialize() was observed to crash after 8584 levels, and unserialize() was observed to crash after about 12500 levels, in both cases due to exhaustion of the 8MB thread stack.

The solution options that occur to me are:

a) Increase the thread stack size with ulimit -Ss. This is tested and works for the above test case, but to work for any conceivable test case would require a very large stack, perhaps 2GB.
b) Introduce a depth limit to serialize() and unserialize(). Probably sensible in any case, but potentially tricky to sell the PR to upstream, and would still leave us with an error message delivered to the user.
c) Rewrite serialize() and unserialize() to be non-recursive. That would be quite a lot of work.
d) Use a different data structure in PPNode_Hash_Tree, for example using integer keys to break references, perhaps in an HHVM-specific subclass.
e) Use a custom serializer in PPNode_Hash_Tree, which can traverse the tree without recursion.

I upped the stack size soft limit to 32MiB, which doesn't fix the problem, but certainly makes it a lot more esoteric.

I tried (e), using XML instead of unserialize(): parsing XML was slower than just parsing the original wikitext, and 9 times slower than unserialize().

Are these still occurring in production or can we resolve fixed while working on a better solution? I've not been able to reproduce this with any of the cited pages.

It's much more esoteric but reproducable:

https://de.wikipedia.org/w/index.php?title=Benutzer:Boshomi/Test2&action=submit

add a little bit => 503

On my original testpage https://de.wikipedia.org/wiki/Benutzer:Boshomi/testurl I receive much more 15 sec timeouts, compared with php.

(In reply to Erik Moeller from comment #9)

Are these still occurring in production or can we resolve fixed while
working on a better solution? I've not been able to reproduce this with any
of the cited pages.

Impact on users is effectively nil, in that the bug is now only reproducible with specially-contrived degenerate input. (I should note that it is possible to make Zend crash this way, too.)

It's not good to segfault on user input, so there is still a bit of work to do before we mark this resolved. I think the sanest approach would be (b) (cf comment 6). From the user's perspective the only thing that would change would be the phrasing of the error message we present.

(In reply to Ori Livneh from comment #11)

It's not good to segfault on user input, so there is still a bit of work to
do before we mark this resolved. I think the sanest approach would be (b)
(cf comment 6). From the user's perspective the only thing that would change
would be the phrasing of the error message we present.

A depth limit on serialize() wouldn't completely fix the problem though. Try something like this and you will get a segfault in both PHP and HHVM:

$a = null;
for ( $i = 0; $i < 100000; ++$i ) {

$a = (object)array( $a );

}
$a = null;

In the above example, the segfault happens on the last line, when the objects are recursively destroyed and the stack overflows. So I think (d) should be considered.

Someone's already doing option (b) internally, which will at least turn segfaults into fatal errors. I'll let you know when the fix makes it out to master (it's pretty simple). He's also going to look at (c) but that might not end up happening if it's too complex or makes things slower.

For the record, I just tested this and it's still not solved in HHVM 3.12; I guess we'll keep our hack for the time being.

It's been 8 years since I wrote Preprocessor_Hash, and reviewing the code now gives me a new perspective. I think the rationale for using singly-linked lists does not hold up. I think I did it for the following reasons:

• O(1) list merge, e.g. line 500. This is cute but doesn't give you a better overall time order than say array_splice() to append to a child node array, since we spent O(N) time already creating the list, and the list merge is only done once.
• O(1) list truncation, line 605-628. Again, replacing with array_splice() would apparently not affect the overall time order.
• Using a DOM-like API it made it easier to port the code from Preprocessor_DOM and to provide a common interface. For example we have a public function PPNode::getNextSibling() which is easier to implement if you have a linked list. An alternative is a cached array index , possibly stored in a temporary object. PPNode_DOM is a temporary object, created when it is needed by the external API, so PPNode_Hash could be similar. Note that the public API is read-only.

I'm going to try to modify the data structure and see how that goes.

Change 299710 had a related patch set uploaded (by Tim Starling):
Preprocessor_Hash: use child arrays instead of linked lists

https://gerrit.wikimedia.org/r/299710

Change 299710 merged by jenkins-bot:
Preprocessor_Hash: use child arrays instead of linked lists

https://gerrit.wikimedia.org/r/299710

I used a reproduction case from T135483 and it is apparently all fixed. Seems https://gerrit.wikimedia.org/r/299710 solved it.

I have opened a few of the URL from this ask description and they all render.

@tstarling @ori I think you can now close this task.