magento2: PageCache: async rendering of blocks can corrupt layout cache

It meas, that I have fixed this issue and wrapped it up as a separate module. However as I said earlier, I won’t share it. Magento Community has helped me to find the root cause of the issue and that’s why I was able to fix it. That’s why I want to thank the community for this and share the root cause of it. However I will not share the fix, because if after my notes you cannot fix the issue like I fixed it, then you are useless as Magento specialists and will not be able to provide high quality assistance to your clients on Magento 2 platform. So the goal of my posts here is: to introduce an exercise, which is based on one of real problems, which faces a Magento developer almost every day. I want to make the community stronger. Just providing a final fix will not improve the level of community developers.

That is not how any of this works. If you really think you are setting the bar higher let me tell you that you are doing the complete opposite, leaving a bug open because you think people should be “smart” like you to fix it. This is how you make a community weaker because you say that if anyone is not as good as you it should not participate at all and that is completely false. But hey, this is OSS and we deal with this kind of cases every day so chances are someone will probably come and fix it. In the meantime @adrian-martinez-interactiv4 keep up the good work!

Yeah, there are many reasons of not sharing the code, one of them is - NDA of the company, where I am working for. We have fixed a lot of core bugs of M2 internally and this is one of the most difficult to find. And why have I posted a tip here is - because this issue number #8554 has also given me a tip about layout type such as “1column”, “empty”, etc. I was debugging it almost whole day and was starting to become a crazy, because I could not understand: why in the Layout object’s “_blocks” property the list of generated blocks are only a few of them in page_cache controller, however in CMS_index controller the list of blocks in “_blocks” property is big: about 79 blocks there. And at the same time the final XML in both actions is almost the same! The cause of that was: because Magento adds some blocks to the “_blocks” property manually in a separate method depending on what “pageLayout” is set. As the result: in page_cache action no page layout was passed and no page layout was set. I don’t need a validation of my fix 😃 It is on Production already and it works for us 100% 😃

The thread was cleaned from not-related to issue comments. Please refrain from negative messaging and be respectful to each other.

😄 Ok, continue adding new handlers for FPC, up to you! I also don’t see no reason in adding a new param to the request, until it was a core Magento approach how they build the final layout XML: they phisically set this layout type into Layout object and then even Create some blocks from PHP code, if a layout type was set. That’s why any other fixes, such as making CacheID unique, etc, do Not fix he issue Completely for ALL cases. I have successfully fixed it for our client in March and we continue shareing this fix internally from project to project. But you may continue having fun in fixing other bugs many times instead of fixing the root cause once. 😄

if one could see and read the fix, one can learn from it, trying to understand what the problem was and how the solution was build. For me thats a giant part of OSS. If you have a patch and don’t want to share it, okay, go your way, but please don’t brag (it’s nothing more to me) about it.

(And yes, it is about you, since you were implying, that you are good and if one can’t do one thing you can, he or she is useless)

A proprietary patch? When will this land in Magento EE? :trollface:

@bka - I have a solution to the issue. You can reference the fix in this commit: https://github.com/magento/magento2/commit/99bb170a805f3e9d0ab5c9b9bb801f739ac161c1

Try applying this patch to your environment and see if the issue goes away:

diff --git a/vendor/magento/module-ui/view/base/layout/default.xml b/vendor/magento/module-ui/view/base/layout/default.xml
index 7f2efbd..64d5f14 100644
--- a/vendor/magento/module-ui/view/base/layout/default.xml
+++ b/vendor/magento/module-ui/view/base/layout/default.xml
@@ -5,7 +5,7 @@
  * See COPYING.txt for license details.
  */
 -->
-<page xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" layout="admin-1column" xsi:noNamespaceSchemaLocation="urn:magento:framework:View/Layout/etc/page_configuration.xsd">
+<page xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:View/Layout/etc/page_configuration.xsd">
     <body>
         <referenceContainer name="after.body.start">
             <block class="Magento\Ui\Block\Logger" name="logger" template="Magento_Ui::logger.phtml"/>

If the issue does go away, I’d recommend closing this issue.

Ok, I did some debugging. It seems to only affect CMS pages and is related to the way how Magento_PageCache module works. This module renders a script into the DOM to fetch some blocks asynchronously.

<script type="text/x-magento-init">
    {
        "body": {
            "pageCache": {"url":"http:\/\/magento2.local\/index.php\/page_cache\/block\/render\/","handles":["default","cms_index_index","cms_page_view","cms_index_index_id_home"],"originalRequest":{"route":"cms","controller":"index","action":"index","uri":"\/index.php\/"},"versionCookieName":"private_content_version"}        }
    }
</script>

This is done in PageCache/view/frontend/web/js/page-cache.js. This call hits PageCache/Controller/Block/Render.php and PageCache/Controller/Block.php:_getBlocks triggers the loading of the layout:

$this->_view->loadLayout($handles, true, true, false);

However, because of the way how lib/internal/Magento/Framework/View/Model/Layout/Merge.php:_fetchRecursiveUpdates works, the pageLayout will be set to admin-1column defined in app/code/Magento/Theme/view/adminhtml/page_layout/admin-1column.xml. Unfortunately, in this case lib/internal/Magento/Framework/View/Page/Layout/Reader.php:read just gets an empty layout <layout/> for $xml.

public function read(Layout\Reader\Context $readerContext, $pageLayout)
{
    $this->getPageLayoutMerge()->load($pageLayout);
    $xml = $this->getPageLayoutMerge()->asSimplexml();
    $this->reader->interpret($readerContext, $xml);
}

I think the layout is not loaded because our area in this case is frontend not adminhtml. So this layout won’t be loaded in this case. And therefore there are some critical elements missing inside scheduledStructure afterwards, like e.g. the root container.

Now, the bad thing happens. This structure gets stored into the cache, if there is none. This is done in lib/internal/Magento/Framework/View/Layout.php:generateElements.

    $cacheId = 'structure_' . $this->getUpdate()->getCacheId();
    $result = $this->cache->load($cacheId);
    if ($result) {
        $this->readerContext = unserialize($result);
    } else {
        \Magento\Framework\Profiler::start('build_structure');
        $this->readerPool->interpret($this->getReaderContext(), $this->getNode());
        \Magento\Framework\Profiler::stop('build_structure');
        $this->cache->save(serialize($this->getReaderContext()), $cacheId, $this->getUpdate()->getHandles());
    }

The $cacheId will be the same as for a normal request because it is based on used handles. But remember, we have missing elements in the structure like root. Next time, somebody visits the page, he will get a blank page without body, because this broken layout structure has been loaded from the cache.

For normal requests on CMS pages, this is not happening, because the controller sets the pageLayout explicitly in app/code/Magento/Cms/Helper/Page.php:setLayoutType before the layout is loaded. This is usually 1column defined in app/code/Magento/Theme/view/frontend/page_layout/1column.xml and can then easily be loaded by lib/internal/Magento/Framework/View/Page/Layout/Reader.php.

I would really appreciate any advice on how to fix this.

Just to add some more information, I quickly looked in the git history and like @bka said, @erikhansen’s proposed solution was already executed on the develop branch in scope of MAGETWO-55341, see: https://github.com/magento/magento2/commit/f42472a3753c8b87969a8e2dde6645525bf797d8 (Not sure if this is the correct and definitive solution though, based on the above comments…)

@andreymoskvenkov C’mon man, this is opensource. Take a little, give a little 😉

All kidding aside, I appreciate your tip, but I’m sure your patch would be appreciated even more and it could be validated. If you can’t share it for whatever reason, I understand.