Fix PHP 7 testbot failures

Drupal\shortcut\Tests\ShortcutSetsTest passes locally for me (on RC6)

So running Drupal\basic_auth\Tests\Authentication\BasicAuthTest

* thread #1: tid = 0x0000, 0x00000001055c6264 php`_emalloc_56 + 36 at zend_alloc.c:1291, stop reason = signal SIGSTOP
    frame #0: 0x00000001055c6264 php`_emalloc_56 + 36 at zend_alloc.c:1291
   1288
   1289		if (EXPECTED(heap->free_slot[bin_num] != NULL)) {
   1290			zend_mm_free_slot *p = heap->free_slot[bin_num];
-> 1291			heap->free_slot[bin_num] = p->next_free_slot;
   1292			return (void*)p;
   1293		} else {
   1294			return zend_mm_alloc_small_slow(heap, bin_num ZEND_FILE_LINE_RELAY_CC ZEND_FILE_LINE_ORIG_RELAY_CC);

With a backtrace of

  * frame #0: 0x00000001055c6264 php`_emalloc_56 + 36 at zend_alloc.c:1291
    frame #1: 0x00000001055c6240 php`_emalloc_56 + 16 at zend_alloc.c:2361
    frame #2: 0x0000000105601f36 php`zend_array_dup(source=0x0000000106d208c0) + 22 at zend_hash.c:1716
    frame #3: 0x000000010566b840 php`ZEND_ASSIGN_DIM_SPEC_VAR_UNUSED_HANDLER(execute_data=0x000000010681aee0) + 112 at zend_vm_execute.h:18975
    frame #4: 0x0000000105630298 php`execute_ex(ex=<unavailable>) + 40 at zend_vm_execute.h:417
    frame #5: 0x00000001056306be php`zend_execute(op_array=<unavailable>, return_value=<unavailable>) + 1038 at zend_vm_execute.h:458
    frame #6: 0x00000001055ef3ab php`zend_execute_scripts(type=8, retval=0x0000000000000000, file_count=3) + 315 at zend.c:1428
    frame #7: 0x000000010558b7f4 php`php_execute_script(primary_file=0x00007fff5a9f82b0) + 852 at main.c:2471
    frame #8: 0x0000000105682b60 php`do_cli(argc=<unavailable>, argv=<unavailable>) + 4048 at php_cli.c:974
    frame #9: 0x0000000105681a17 php`main(argc=<unavailable>, argv=<unavailable>) + 1399 at php_cli.c:1345
    frame #10: 0x00007fff8a9e65c9 libdyld.dylib`start + 1

So what's interesting is that we're crashing in the test runner parent and not in the site under test. Going to try reporting this to bugs.php.net and see what happens. If I took another test from the segfault list (Drupal\taxonomy\Tests\TermTest) - it crashes at exactly the same spot with the same backtrace - which leads me to hope we have one thing fix here.

Ran it 5 times and Drupal\page_cache\Tests\PageCacheTest passes locally for me.

Created https://bugs.php.net/bug.php?id=70805

Drupal\basic_auth\Tests\Authentication\BasicAuthTest passed locally for me with RC6, --class and --repeat 5.

I found the bug with the graph edge, it is GC related, which also explains why it happens later, because we leak memory elsewhere / or in this graph.

So by disabling the GC during compilation we can easily work-around that and also probably gain some speed.

Recommended reading how this has impacted composer (on another graph operation):

- http://blog.blackfire.io/performance-impact-of-the-php-garbage-collector...
- http://blog.ircmaxell.com/2014/12/what-about-garbage.html

Attached patch disables gc during container compilation.

There's so much wrong it makes my head hurt. We have a graph implementation which to the best of my knowedge doesn't crash PHP 7.

More importantly, if this is critical, does it mean we are blocked on releasing D8.0.0 until PHP7 bugs are fxed / worked around?

More importantly, if this is critical, does it mean we are blocked on releasing D8.0.0 until PHP7 bugs are fxed / worked around?

No if we're blocked on issues in PHP 7 itself, we'll have to do #2576745: Add a hook_requirements() warning when running on PHP7 then require 7.0.4 or whatever version allows us to work. But it's worth trying to not do that if we can.

Okay, we need to collect the cycles else PHP 5.5 runs out of memory.

Lets try that.

> // The service graph implementation is prone to corruption during GC.

Once more: can we switch to the graph implementation?

#14: We could do that, but we would need to write a proxy for the ServiceReferenceGraph* classes, which is possible.

Another option might be to explicitly GC the graph. It is a very simple implementation, which should not fail in the way it does ...

---

For the 1 remaining test failure I filed:

https://bugs.php.net/bug.php?id=70808

TL;DR is:

Under certain circumstances if you have an array and you do an unset() for an item at the end and then an array_merge_recursive(), then array_merge_recursive() will append to the end and the old unset items will in many cases (but not all) corrupt the memory.

e.g.

PHP5 always re-indexes the array on array_merge_recursive(), so this bug cannot occur - or maybe it just does see better what is unset and what is not.

Anyway an important bug for PHP to resolve before their release ...

PHP7 fixed the bug in #15 already after just 5 hours (took 3 hours to resolve it seems)! - My bug report must have been good :p

(https://bugs.php.net/bug.php?id=70808)

Wow, nice :)

And just to confirm #16, it is fixed locally now, too when running the test suite.

So once Drupal-CI has build new, this will be fixed.

One caveat though:

- The new branch that has the fix is PHP-7.0, not PHP-7.0.0. I think someone mixed that up, upstream in PHP team.

Or it means this really simple bug fix is just considered for 7.0.1 at this point, which would be sad.

@neclimdul: Maybe you can ask your contacts about that?

So I've tried to abuse Symfony's AnalyzeServiceReferencesPassTest to cause the segfault in a simpler way than running Drupal 8 tests - unfortunately no luck yet.

So lol... in the PHPUnit bootstrap.inc for the dependency injection component in Symfony there is the following line of code...

// Disabling Zend Garbage Collection to prevent segfaults with PHP5.4+
// https://bugs.php.net/bug.php?id=53976
gc_disable();

Funnily enough though running the tests with PHP 7, 5.6 and 5.5 does not produce any segfaults unfortunately.

Alright, let's all not stop shaking our head for a while.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -821,7 +821,13 @@ protected function initializeContainer() {
+      gc_disable();
       $container = $this->compileContainer();
+      gc_enable();

So there is a trade-off to do this - we will end using more memory. I guess the questions are (a) will we get a fix in PHP7 before we launch. (b) what is the memory cost of doing this.

If we don't get a fix in PHP7 - scheduled to be release on Nov 12th and the memory cost of doing #13 is acceptable then I think this is a better workaround than making PHP7 support experimental.

If we have to we can do a version check before the gc change.

Also we should get that simpletest memory patch in now that pifr is off.

#23: Please see #13, where we do collect the cycles before disabling the GC.

So we have all memory freed before rebuilding the container, which makes sense and should not take long.

And yes we can add a version check.

re #25 so even with gc_disable() it still stores roots - see https://secure.php.net/manual/en/features.gc.collecting-cycles.php

I think we don't even need to disable the GC, just collecting cycles before compiling the container should be enough to not trigger the GC during the graph operation.

I added tracing to zend_gc_collect_cycles() and it is pretty much always called. Of notice is that also the crash happens after zend_gc_collect_cycles().

The GC is called during container compilation anyway and the reason is that the GC buffer can only hold 10'000 potential roots, so if lots of objects are created and destroyed, then this easily gets over this limit, so PHP7 must call the GC.

The GC is called during container compilation anyway and the reason is that the GC buffer can only hold 10'000 potential roots

Interesting!

#18:

- The new branch that has the fix is PHP-7.0, not PHP-7.0.0. I think someone mixed that up, upstream in PHP team.

Or it means this really simple bug fix is just considered for 7.0.1 at this point, which would be sad.

@neclimdul: Maybe you can ask your contacts about that?

@rasmus tweeted about this fix (https://twitter.com/rasmus/status/659953498293858304) and so I took a bit of a deeper look at PHP's git repo. It seems they periodically merge the 7.0 branch into 7.0.0 (http://git.php.net/?p=php-src.git;a=commit;h=40142c084a28a6266b7b781778c...). OTOH, the 7.0 branch got a commit saying "7.0.1 is next". So I'm not at all certain. Hence I simply asked Rasmus on Twitter: https://twitter.com/wimleers/status/660008466925428736.

Rasmus answered: https://twitter.com/rasmus/status/660092777523314688

the RM will cherry-pick critical fixes into the release branch

So, we're good :)

Based on the fact that the last fails are all related to https://bugs.php.net/bug.php?id=70805 and we still have #2454439: [META] Support PHP 7 to track any issues I'm downgrading this to major and postponing - since we'll only need to do something here if PHP fail to fix that bug before release - given the attention that seems unlikely. If we do need to do something this will be an rc target so mark as such.

Who knows which tag to use :)

@catch said that we should have a green PHP7 green and commit a patch doing PHP7 only fixes to make regressions easier to spot. Seems like a good idea. The patch attached is green on PHP7 RC6 for me.

Here's the gc only patch - so we can have separate commits for each fix.

Committed/pushed to 8.0.x, thanks! (As two separate commits so we can do a clean revert individually).

Marking this issue fixed since we're at 100% pass.

We can now post revert patches on #2454439: [META] Support PHP 7 for retesting as PHP 7 builds include the fixes - that can stay critical and maybe postponed to have a fully green PHP 7 test run with no hacks.

D8 Accelerate sponsored neclimdul and me for 10 hours (5 each), so re-adding tag for that.

Nice work!!!