1563606 - Error showing error pages: Too few arguments to function PhutilErrorHandler::handleError()

Status: RESOLVED FIXED

(Conduit :: Phabricator, defect, P2)

Description

[Māris Fogels [:mars] (please needinfo)]

When viewing a Phabricator error page I received the following page in my browser as unformatted plain text (no Phabricator UI, no styling).

For the URL https://phabricator.services.mozilla.com/api/differential.revision.search, requested on 2019/07/04 at approximately 19:00 UTC, I received this exception info:

[Core Exception/ArgumentCountError] Too few arguments to function PhutilErrorHandler::handleError(), 4 passed in /app/phabricator/externals/extensions/sentry/sentry/src/ErrorHandler.php on line 361 and exactly 5 expected

The error doesn't show up in Sentry. However, going by the exception text the error is in the Sentry error handler code itself.

Steps to Reproduce:

1. Log into Phabricator
2. Run an operation using the Conduit API such as https://phabricator.services.mozilla.com/api/differential.revision.search. You should see a valid API result page.
3. Wait until API search results expire? I was refreshing the results page after a 24 hour break and after restarting my browser (nightly).

Expected result:
There should be a nicely formatted HTML Conduit page.

Actual result:
Plain text page (no HTML, no styling) showing the aforementioned ArgumentCountError exception text and exception line number.

To help find the request that generated the error in the server logs:
URL: https://phabricator.services.mozilla.com/api/differential.revision.search
Timestamp: 2019/07/04 at approximately 19:00 UTC
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:69.0) Gecko/20100101 Firefox/69.0

Comment 1

[Māris Fogels [:mars] (please needinfo)]

There is a better reproduction in bug 1584288 comment 0:

I got a broken error message when trying to land a commit with a broken bug # on it. Specifically, I tried to push patches for a bug which I did not have access to.

The error message I got looked like this:

(debug) Response: {"result": null, "error_code": "ArgumentCountError", "error_info": "Too few arguments to function PhutilErrorHandler::handleError(), 4 passed in /app/phabricator/externals/extensions/sentry/sentry/src/ErrorHandler.php on line 361 and exactly 5 expected"}

I believe that the problem is that an error handler registered by libphutil expects 5 arguments, and the sentry-php handler passes down only 4.

• libphutil: https://github.com/phacility/libphutil/blob/f39a03df2bc52777f7e5572baa9bb8df2a9b016c/src/error/PhutilErrorHandler.php#L183
• sentry-php: https://github.com/getsentry/sentry-php/blob/5896f9f0a00a7525797764ba42091dfbde22a746/src/ErrorHandler.php#L386

Comment 5

[:glob ✱]

This is a bug in the php sentry library (https://github.com/getsentry/sentry-php/issues/891), fallout from the php7 upgrade.

https://www.php.net/manual/en/migration71.incompatible.php

Previously, a warning would be emitted for invoking user-defined functions with too few arguments. Now, this warning has been promoted to an Error exception. This change only applies to user-defined functions, not internal functions.

Manually applying the corrected PR to the sentry lib fixed it:

--- ErrorHandler.php	2019-10-02 15:24:19.831098763 +0800
+++ ErrorHandler-patched.php	2019-10-02 15:23:55.088408601 +0800
@@ -368,7 +368,7 @@
      *
      * @throws \Throwable
      */
-    private function handleError(int $level, string $message, string $file, int $line): bool
+    private function handleError(int $level, string $message, string $file, int $line, array $errcontext): bool
     {
         if (0 === error_reporting()) {
             $errorAsException = new SilencedErrorException(self::ERROR_LEVELS_DESCRIPTION[$level] . ': ' . $message, 0, $level, $file, $line);
@@ -383,7 +383,7 @@
         $this->invokeListeners($this->errorListeners, $errorAsException);

         if (null !== $this->previousErrorHandler) {
-            return false !== \call_user_func($this->previousErrorHandler, $level, $message, $file, $line);
+            return false !== \call_user_func($this->previousErrorHandler, $level, $message, $file, $line, $errcontext);
         }

         return false;

I also needed the following to avoid calling array_walk_recursive on a null value:

diff --git a/extensions/src/logging/SentryLoggerPlugin.php b/extensions/src/logging/SentryLoggerPlugin.php
index 17ec748..96902b6 100644
--- a/extensions/src/logging/SentryLoggerPlugin.php
+++ b/extensions/src/logging/SentryLoggerPlugin.php
@@ -78,7 +78,9 @@ class SentryLoggerPlugin extends Phobject {
           $item = '********';
         }
       };
-      array_walk_recursive($request['data'], $sanitize, $fields_re);
+      if ($request['data']) {
+        array_walk_recursive($request['data'], $sanitize, $fields_re);
+      }

       // Sanitize query string
       $query_data = self::parse_query_str($request['query_string']);

Given this issue is masking actual errors being returned to callers in addition to preventing capturing of errors in sentry we should probably disable sentry integration until the sentry php extension has been fixed.

dkl - thoughts?

Comment 6

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› 🎈 from comment #5)

Given this issue is masking actual errors being returned to callers in addition to preventing capturing of errors in sentry we should probably disable sentry integration until the sentry php extension has been fixed.

dkl - thoughts?

We can also just manually patch the lib in the Dockerfile until the PR is accepted upstream.

dkl

Comment 7

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #6)

(In reply to Byron Jones ‹:glob› 🎈 from comment #5)

Given this issue is masking actual errors being returned to callers in addition to preventing capturing of errors in sentry we should probably disable sentry integration until the sentry php extension has been fixed.

dkl - thoughts?

We can also just manually patch the lib in the Dockerfile until the PR is accepted upstream.

dkl

This has been fixed by https://github.com/getsentry/sentry-php/pull/892/files and will be in the next release whenever that is. We can manually patch it til then or wait til the next release and we will get it during our next update to Phabricator.

Comment 8

[:glob ✱]

Let's patch with the change from the PR - the current state of returning the PHP error for normal errors (eg. selecting a review which doesn't exist on Phabricator) is not ideal.

Comment 9

[David Lawrence [:dkl]]

Attachment: phabricator-extensions: Bug 1563606 - Error showing error pages: Too few arguments to function PhutilErrorHandler::handleError()

When viewing a Phabricator error page I received the following page in my browser as unformatted plain text (no Phabricator UI, no styling).
For the URL https://phabricator.services.mozilla.com/api/differential.revision.search, requested on 2019/07/04 at approximately 19:00 UTC, I received this exception info:

[Core Exception/ArgumentCountError] Too few arguments to function PhutilErrorHandler::handleError(), 4 passed in /app/phabricator/externals/extensions/sentry/sentry/src/ErrorHandler.php on line 361 and exactly 5 expected

The error doesn't show up in Sentry. However, going by the exception text the error is in the Sentry error handler code itself.

Comment 10

[David Lawrence [:dkl]]

Merged. Will be in the next Phabricator update.

Comment 11

[Justin Lebar (not reading bugmail)]

Is there a workaround until the next phab update?

Comment 12

[David Lawrence [:dkl]]

(In reply to Justin Lebar (not reading bugmail) from comment #11)

Is there a workaround until the next phab update?

Unfortunately no. But on the bright side the next phab update will be tomorrow morning.