Problem/Motivation
FlattenException attempts to provide a displayable trace and goes into a depth of 10 levels for arrays. When we throw an exception within a Form, where $form is a method argument in the stack trace, it results in a huge amount of memory usage and then chokes on it, resulting in a fatal error instead of a proper error.
Proposed resolution
We actually have our own code for parsing and displaying the backtrace and we don't use FlattenException. While we could look into using Symfony instead or using a framework like whoops (thereis an issue open for it), for now, let's just drop the usage of FlattenException, which actually allows us to remove a duplicated method.
Remaining tasks
User interface changes
API changes
Original report by @whiplashomega
I copied a large section of code from an existing website's HTML into the source editor in the ckeditor when making a test page. This code contained the unicode character U+DDAB. When I saved the page I got the following error:
Fatal error: Allowed memory size of 536870912 bytes exhausted (tried to allocate 32 bytes) in C:\xampp\htdocs\dcfinternet\core\vendor\symfony\debug\Symfony\Component\Debug\Exception\FlattenException.php on line 223
As you can see, I have no shortage of memory allocated to PHP(half a gig). Removing the character from the code fixed the problem. Since that particular character's presence was an error created by a change in encoding(it was supposed to be a 'tt' in the word 'getting') this wasn't a serious issue in this case, but I imagine this issue extends to other atypical unicode characters as well.
Tagging this with D8MI as well, since this issue could affect language support for non-roman languages, such as Chinese.
Comment | File | Size | Author |
---|---|---|---|
#27 | interdiff-24-27.txt | 1.37 KB | hussainweb |
#27 | 2386571-27.patch | 5.07 KB | hussainweb |
#24 | 2386571-24.patch | 4.35 KB | dawehner |
#24 | interdiff.txt | 1.15 KB | dawehner |
#21 | 2386571-21.patch | 3.95 KB | dawehner |
Comments
Comment #1
Wim LeersWOW! WOW!
I unfortunately can't reproduce this though. Posting this:
does not trigger that crash you're seeing. At least not on the latest Drupal 8 version. Note that you're using beta 2, whereas we're already at beta 3.
Comment #2
whiplashomega CreditAttribution: whiplashomega commentedApparently this is an issue in Drupal 7 as well, when I try to paste the character that caused the error into a comment it gives me an error, although not a fatal one, just a 'problem with your comment' error. The character that caused the error is, however, not U+DDAB as originally reported, that was just what jEdit thought it was when I was trying to save a file containing it that was encoded with Cp1252. I looked it up in the utf-8 table and apparently it is in fact character U+1001AB, way down in the supplemental sets. Support for those characters is probably not important to Drupal, but there should probably be some sort of error handling path for this.
Comment #3
whiplashomega CreditAttribution: whiplashomega commentedUpdated to beta3, issue still exists.
Comment #4
Wim LeersNeither #2 nor #3 answer #1.
I'm now suspecting that your LAMP stack is the root cause of this problem, that you're hitting a PHP bug.
Which PHP version are you using?
Comment #5
whiplashomega CreditAttribution: whiplashomega commentedI'm actually running on a WAMP stack, a recent version of XAMPP running on a Windows machine. From phpinfo() I've got the following information:
PHP Version 5.5.15
Windows Server 2008 R2 Standard Edition Service Pack 1
Apache 2.0 Handler
Drupal Version 8.0.0-beta3, no modules installed beyond Core, running a custom theme on the front end, seven for administration.
Enabled Extensions:
bcmath
bz2
calendar
Core
ctype
curl
date
dom
ereg
exif
fileinfo
filter
ftp
gd
gettext
hash
iconv
json
libxml
mbstring
mcrypt
mhash
mysql
mysqli
mysqlnd
odbc
openssl
pcre
PDO
pdo_mysql
pdo_sqlite
Phar
Reflection
session
SimpleXML
soap
sockets
SPL
sqlite3
standard
tokenizer
uploadprogress
wddx
xml
xmlreader
xmlrpc
xmlwriter
xsl
zip
zlib
Comment #6
Wim LeersPlease do the two following things:
Comment #7
whiplashomega CreditAttribution: whiplashomega commentedThis does not resolve the issue, I disabled my theme, and ran on Bartik, issue still appeared. Then I reinstalled stock beta3 in a new database, added nothing, and the issue still exists. Some additional notes for trying to recreate the error:
1. Previewing a page works fine, the error message only appears when I save the page (published or unpublished).
2. I can recreate the error by copying and pasting any of the Unicode characters on this page into the text of the body: http://www.utf8-chartable.de/unicode-utf8-table.pl The latest tests I did, it was the only character in the body, on the only page on the site. In fact the error occurs with any uncode character higher than U+FFFF, so starting with U+10000. This matches the point at which utf-8 switches from using 3 bytes to store characters, to 4 bytes. This range of characters does include a few actual languages, if only ancient and/or highly esoteric ones.
3. Content type does not affect the issue. The error crops up in custom block creation, articles, basic pages, and I imagine just about anything else.
4. If you are unable to recreate on Linux, I suspect it may be due to poor utf-8 support on windows, particularly for C dependent programs (such as PHP and MySQL). When I attempt to insert one of these characters directly into a table in phpmyadmin it gives me a warning, and replaces it with a ?(but does not fail entirely). When I insert it using the MySQL command line, there is no error and no warning, but it still replaces it with a ?.
Comment #8
Wim LeersThat definitely sounds like a problem with your AMP stack then. There's nothing Drupal can do to help you, I'm afraid. I can't reproduce this locally nor with http://simplytest.me/project/drupal/8.0.x. Could you please try reproducing it with http://simplytest.me/project/drupal/8.0.x, just to make sure? Thanks :)
Comment #9
whiplashomega CreditAttribution: whiplashomega commentedGetting the same error at simplytest.me
Here are links to screenshots:
https://drive.google.com/file/d/0B8hs5jMXGj5iUVB4dThSNjBRcDQ/view?usp=sh...
https://drive.google.com/file/d/0B8hs5jMXGj5iX2oyYmdtSDV1Nmc/view?usp=sh...
I don't expect Drupal to have support for these highly non-standard characters. (to the point that Windows doesn't appear to have a single default font that supports them) Simply to not fail so spectacularly when they are put in, stretch goal to have a helpful error message letting a content editor or site owner identify what caused the error.
Another note that the alt+code method of inserting a special character has an upper limit that prevents these characters from being typed in. I've only been able to do this by copying and pasting from a UTF-8 character table, or from another source that already contains the character.
Comment #10
Wim LeersWow, if you can reproduce it on simplytest.me, it's not an AMP stack thing probably.
I just wonder why I haven't been able to reproduce it using #1. Tagging as a critical bug, because no content should be able to crash Drupal like this.
Comment #11
swentel CreditAttribution: swentel commentedRelated to #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols) and #2382707: UTF8MB4 for MySQL
Oh those lovely emoji. See https://www.drupal.org/node/2043439
Comment #12
swentel CreditAttribution: swentel commentedSo yes, probably duplicate and we can close one other two as well I guess.
Comment #13
Wim LeersI think we should bump #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols) to critical and close the other one and this one as a duplicate. Do you agree?
Comment #14
BerdirI don't think it is directly related to the Unicode character. It's just that there seems to be a loop in symfony's FlattenException implementation, I've seen this a few times myself for various, different reasons.
So I think this is a separate bug. We should fix the actual error, but we also need to prevent FlattenException from blowing up badly if an exception happens.
Comment #15
Wim LeersBut something must trigger that exception, right?
Comment #16
BerdirAbsolutely, and in this case, it is definitely that other issue. But this is a more generic problem. We need to fix both the specific reported problem (which also exists in D7 and has for a long time) and the fact that some exceptions seem to lead to recursion in FlattenException. That must not happen, no matter how bad an exception is.
Comment #17
Wim LeersOh, agreed then :)
Comment #18
chx CreditAttribution: chx commentedI am leaving this tag despite I did update the IS but it needs fact checking.
Comment #19
chx CreditAttribution: chx commentedComment #20
dawehnerIn case someone wants to reproduce it: Get anything from http://apps.timwhitlock.info/emoji/tables/unicode
The problem is really basically that the protection of FlattenException against long stuff, works pretty great for deep structures,
but as we all know, Drupal use some form structures which are both deep and wide at the same time.
https://github.com/symfony/symfony/issues/13003
Comment #21
dawehnerFrom my point of view there is no point in using the FlattenedException ... given that \Drupal\Core\Utility\Error::formatBacktrace() already exists ...
and outputs it in the same exact way as the flattened exception with the flatten specific method.
Comment #22
Wim LeersComment #23
BerdirIssue summary still needs work, but here's a better issue title.
Comment #24
dawehnerLet's get rid of the variable name as well.
Comment #25
BerdirManually tested that this is working well (as in, it does now actually display the exception, which needs to be resolved in one of the referenced issues).
Updated the issue summary.
I'm not sure about tests, we would have have to generate a large array, pass it to a method that then throws an exception and verify it does not explode. Seems a pretty weird thing to write an automated test for me.
Comment #26
BerdirComment #27
hussainwebNot a big change. I am just removing
use
statements that referred to theFlattenException
class, since we are not using it anymore.Comment #28
BerdirOk, discussed with @alexpott, he agreed that we don't need test coverage here.
Comment #29
catchCommitted/pushed to 8.0.x, thanks!