Follows-up I8520d8cb16 and Ib941c22d6b7ec5f1b9, and adds an internal
setter for use by Setup.php, and for wmf-config:rpc/RunSingleJob.php
which is a use case for setting it after Setup.php but before most
code execution. Interact with a component owner instead of directly
maintaining state and providing a shared API through the Config object.
Change-Id: I5c3d4b11f4e0eb3906ccb5b5fe3979e026459859
Allow callers of MWExceptionHandler::getStructuredExceptionData() and
jsonSerializeException() to explicitly control whether a backtrace is
included in the return value. This avoids the need to rely on the
LogExceptionBacktrace setting in static methods.
Bug: T294739
Change-Id: Ib941c22d6b7ec5f1b984bf5ded90652e42ad7b67
Rename canRecoverFromDisconnect() in order to better describe
its function. Make it use the transaction ID and query walltime
as arguments and return an ERR_* class constant instead of a bool.
Avoid retries of slow queries that yield lost connection errors.
Track session state errors caused by the loss of named locks or
temp tables (e.g. during connection loss). Such errors will prevent
further queries except for rollback() and flushSession(), which must
be issued to resolve the error.
Add flushPrimarySessions() methods to LBFactory/LoadBalancer
and use it in places where session state loss is meant to be
safely aknowledged.
Change-Id: I60532f86e629c83b357d4832d1963eca17752944
Make phan stricter about null types by setting null_casts_as_any_type to
false (the default in mediawiki-phan-config)
Remaining false positive issues are suppressed.
The suppression and the setting change can only be done together
Bug: T242536
Bug: T301991
Change-Id: I0f295382b96fb3be8037a01c10487d9d591e7e01
Rename canRecoverFromDisconnect() in order to better describe
its function. Make it use the transaction ID and query walltime
as arguments and return an ERR_* class constant instead of a bool.
Avoid retries of slow queries that yield lost connection errors.
Add methods and class constants to track session state errors
caused by the loss of named locks or temp tables. Such errors can
be resolved by a "session flush" method.
Make assertQueryIsCurrentlyAllowed() better distinguish ROLLBACK
queries from ROLLBACK TO SAVEPOINT queries. For some scenarios,
only full tranasction ROLLBACK queries should be allowed.
Add flushSession() method to Database and flushPrimarySessions()
methods to LBFactory/LoadBalancer.
Also:
* Rename wasKnownStatementRollbackError() and make it take the
error number as an argument, similar to wasConnectionError().
Add mysql error codes for query timeouts since they only cause
statement rollbacks.
* Rename wasConnectionError() and mark it as protected. This is an
internal method with no outside callers.
* Rename wasQueryTimeout(), remove some HHVM-specific code, and
simplify the arguments.
* Make executeQuery() use a for loop for the query retry logic
to reduce code duplication.
* Move the error state setting logic in executeQueryAttempt() up
in order to reduce code duplication.
* Move the beginIfImplied() call in executeQueryAttempt() up to the
retry loop in executeQuery(). This narrows the executeQueryAttempt()
concerns to sending a single query and updating tracking fields.
* Make closeConnection() and doHandleSessionLossPreconnect() in
DatabaseSqlite more consistent with the base class by releasing named locks.
* Mark trxStatus() as @internal.
Bug: T281451
Bug: T293859
Change-Id: I200f90e413b8a725828745f81925b54985c72180
The use of HTTP 400 dates back to T35646, which was to address
caching proxies and mobile browsers incorrectly caching bad titles
as valid content. However, this also means that caches in front of
MediaWiki, like Varnish, don't cache it either. Since we know that
these titles will always have no content, having these get cached
in Varnish is totally fine. Presumably the 404 will be enough to
tell other crawlers or scrapers that there's still no content on
these pages.
There's some room for debate on whether a HTTP 400 or 404 is more
technically correct here, but emitting a 404 seems like the more
pragmatic option.
Change-Id: I7b16f30ca6fd9a68f2a410692582692610f1f944
The functions returning null or the class property is set explict null
Found by phan strict checks
Change-Id: I4a271093fb6526564d8083a08249c64cb21f2453
Remove use of two configuration globals. This follows-up to
commit 47adb6d65a (I1a691f01cd82e60) which aimed to access all config
via MediaWikiServices, but that isn't safe during error handling.
These two were only used in the low-level "plain text" and
"basic HTML" error pages, which already can't have any branding,
skinning or localisation; and are not how most exceptions are rendered.
in those edge cases, we can use the name of the software instead of
(trying) to use the name of the site.
== Remove use of $wgSitename ==
In msg() and reportHTML(), remove use of $wgSitename in favour of a
generic fallback matching DefaulSettings.php. This does not affect
the common case of runtime fatals and timeouts, as we're only
changing the fallback after wfMessage() fails in msg(), or when
OutputPage is unavailable, which is typically only if services,
localisation or DB are also down (or not yet loaded). Most exceptions
happen when and after those have initialised fine.
Test case 1:
(Clean state) Edit ViewAction::show() to add `throw new RuntimeException();`
as its first statement, then try to view the main page.
This error page is unchanged. It is skinned, localised, and still uses
the configured sitename in the doc title.
Test case 2:
Edit index.php to call foo() instead of wfIndexMain(),
then try to view the main page. Before, this "minimal HTML" error page
would have a doc title of "Internal error - MyWiki", and now
"Internal error - MediaWiki".
Test case 3:
(Clean state) Edit Message::text() to add `throw new RuntimeException();`,
then try to view the main page. This results in a "plain text" error
page that doesn't even have an HTML doc, and is also unchanged.
== Remove use of $wgMimeType ==
In output(), remove use of $wgMimeType and remove the Content-Type
header that it was used for. This was redundant because the next
statements (reportHTML) already outputs Content-type. In reportHTML,
we sometimes delegate to OutputPage (if safe) and that honours
$wgMimeType already. In other cases, it is handled inline in with a
basic HTML error page, and that branch also sets the Content-Type
header already. In one case (reportOutageHTML) a header was not yet
set. For that one, I've added the missing header call and made it
explicitly text/html.
This is technically a bugfix, because our basic HTML error page is
HTML5, whereas $wgMimeType (which exists to allow enabling XHTML)
can be XHTML which we weren't following. In OutputPage and
Html::htmlHeader that would normally result in outputting `<?xml`.
Test case:
Edit ViewAction::show(), and add `throw new RuntimeException();`
as its first statement, then try to view the main page.
In devtools>network, there is still a proper Content-Type
and charset on the error page document.
Change-Id: I03cfa2b6155fb711582164852e7cab4c325a1b92
Partially reverts 47adb6d65a (I1a691f01cd82e60) as use of service wiring
is not safe in early code like entry points, Setup (incl WebStart/AutoLoader),
and error handling.
Change-Id: I021b8bf34dabe71ec196e89c957238247b1502d0
This reverts commit 0d453d915a.
Reason for revert: Trying to find a better approach than an abstract class
Change-Id: I69c0ec25cb920f41cb0eb6b3ad921d7b02290581
AbstractLocalizedException acts like a shim to ILocalizedException in preperation for refactoring
Bug: T287405
Change-Id: I01e40e82457d49487829d9a435b7a1198369ecf6
This helps phan to detect unreachable code and also impossible types
after the functions.
It helps phan to avoid false positives for array keys
when the keys are checked before
Bug: T240141
Change-Id: I895f70e82b3053a46cd44135b15437e6f82a07b2
Log INormalizedException messages in a structured way, allowing
the logging infrastructure to group them better.
Change-Id: I877909d1113ab93b4b8a115af5bb0fe039ea32d6
In a few places where a PermissionManager is used
but only GroupPermissionsLookup is needed
Also update references to the class in PermissionManager
that referred to it as GroupPermissionLookup
Change-Id: I5d7a13900852a38768a106aeee1ce012c3a04ea2
In order to allow Authority to know about user blocks,
we need a narrow interface to represent such blocks.
This deprecates some methods on AbstractBlocks in favor
of new methods on the Block interface that avoid binding to
the User class.
Bug: T271494
Change-Id: I7bb950533970984a014de0434518fbbefb695131
Follows-up f2543d442a, in which I accidentally removed the prefix.
Also, while at it, drop the "$class: " prefix that we normally
add to exceptions. For errors we internally construct ErrorException,
but this is something a clas name rarely seen by PHP developers and
has sometimes led to confusions about whether this is an exception
or not. We did still have "PHP Notice" and "PHP Warning" right after
it (except for deprecations) which should act as confidence indicator
that they are in fact not exceptions. We also have the channel
name ("error" instead of "exception"). But removing this will help
boost that confidence further, plus it had no added value anyhow, so
less is more?
Change-Id: Ib93f48c94d642f519558aab40d143b43e1d9ed16
This is micro-optimization of closure code to avoid binding the closure
to $this where it is not needed.
Created by I25a17fb22b6b669e817317a0f45051ae9c608208
Change-Id: I0ffc6200f6c6693d78a3151cb8cea7dce7c21653
* Add $wgRequestTimeLimit. If it is non-null, it starts a time limit,
using Excimer with a fallback to set_time_limit().
* Add pretty formatting of timeout exceptions thrown by the library.
Related refactoring.
* Expose the library's critical section feature in MediaWikiServices
* In Setup.php, call warnIfHeadersSent() before sending session headers.
This helped to debug a related issue I had.
* In wfTransactionalTimeLimit() use the new library, and respect the
existing library time limit if it is larger than
$wgTransactionalTimeLimit.
Bug: T269326
Depends-On: I6409ad8a5cba775c27b0d5a79d3300c4dac4c91a
Change-Id: I2e6f6351c451407c06cc7e20932548f7b62e36b6
This key predates MediaWiki's standardised notion of request IDs,
and of PSR-3 and Monolog usage.
Bug: T199607
Change-Id: Ibdd5bf12591761ab45be12ba72943e9d94f678cb
This should make error logs easier to work with through a couple
of ways:
* The stack trace is now complete, instead of missing the first
crucial step, which is often the one used for filtering
purposes and for identifying errors within a given deployed
version of MediaWiki. (E.g. when filtering out an error that is
expected to be fixed by the next release and/or when checking
how prominent an error currently is).
* Logstash reports that report message + trace will not need to be
edited by hand to include the file+line.
* The workflow for Logstash generally follows one of two patterns.
The default is to filter by exception.file (including line number),
which is very sure to catch all possible variants thrown from
the same code, regardless of any variables in the message, but
has the downside of not matching week-over-week consistency due to
file paths (at least for WMF) containing the deployment version.
The other option is to filter by message, which has the risk of
possibly excluding too much if there are multiple unrelated ways
to trigger the issue, but is a sensible second option. This is
usually done by filtering on normalized_message for non-exception
errors, but doesn't work well for exceptions because they contain
the file paths and do so in-between the class and message words,
and thus are not compatible with Logstash's default substring/term
match. The alternative of exception.message is then considered but
is lacking the class/type, which can be fragile.
With this change applied, no editing is needed, and no multiple
approaches need to be considered with the same option.
Either filtering by exception.file as-is, or filtering by
normalized_message as-is, regardless of whether it is an exception
error or other message in another channel, will both work.
Bug: T271496
Change-Id: I5908ed53f9b97b3c9cde126aca89ab6fc197c845
This reverts commit 900c6663b0.
Reason for revert: breaks Parsoid CI. Instead of fixing the test in presence of the FlaggedRevs hook, I would rather convert DefaultPreferencesFactory to Authority and fix the tests once instead of doing it twice.
Change-Id: Iaa440a9804c9ed97339e737162ef64ccf29ceb51
Taint check checks for possible security issues by tracking html
escaping and more by using phan.
This slows done the phan-job a bit and requires more ram
Keep the DoubleEscaped issues out to make reviewer easier
Adds suppression for false positives
Adds taint-annotation to help taint-check
Removes suppression for code phan now understand better by the tracking
of keys in taint-check
Fix some small issues by adding int cast or htmlspecialchars calls
Bug: T216348
Bug: T268920
Change-Id: I849ac4f120fd15b483e8939d4db45c98dc351259
A PHP fatal error from MWExceptionRenderer::output() causes the
exception handler to be re-entered, with the original exception thrown
away. So, catch all throwables. That way we can see both error messages,
neither is thrown away.
Bug: T263911
Change-Id: Ie98438cbdd328fe295c9b5202d79edb0c8fb41c5
This does not help to avoid the suppression inside the class,
but it avoids the suppression for all callers
Remove unneeded local vars while looking at the code
Bug: T269895
Bug: T216348
Change-Id: I5ce9a2dddca2b3bb556eb7adb5305178db09a71a
$file and $line are reported as possibly undefined in static code
analyzer, but the $level = 0 does not pass self::$fatalErrorTypes,
just return earlier
Value 0 is not used:
https://www.php.net/manual/en/errorfunc.constants.php
Change-Id: Ib8431d6ed496ee50447c8c976afe17ebac03e156
* Always use trigger_error for deprecation warnings, not just in development.
They are still silent from the run-time perspective (not thrown as
exceptions).
Previously this code path was only called when $wgDevelopmentWarnings
is set to true. For most dev environments and for CI, this means
nothing much changes given that DevelopmentSettings.php set this to true.
* In the code path that handles native PHP warnings, when setting the $file
and $line attribution that Logstash/Kibana report as "exception.file"
use the same offset as the one that wfDeprecated() has computed from
the back trace. This means it no longer (wrongly/uselessly) attributes
all deprecation warnings to MWDebug.php.
* Trim the message suffix from "Called from <method> in <file>" to
just "Called from <method>". This reduces noise and makes the message
more stable over multiple MW branches. The stack trace is still there
like before.
== Before (only with $wgDevelopmentWarnings) ==
> PHP Deprecated: Use of wfGetScriptUrl was deprecated in MediaWiki 1.35.
> [Called from MediaWiki::__construct in /var/mediawiki/includes/MediaWiki.php at line 67]
>
> Error from line 393 of /var/mediawiki/includes/debug/MWDebug.php
>
> #0 [internal function]: MWExceptionHandler::handleError()
> #1 /var/mediawiki/includes/debug/MWDebug.php(393): trigger_error()
> #2 /var/mediawiki/includes/debug/MWDebug.php(297): MWDebug::sendMessage()
> #3 /var/mediawiki/includes/debug/MWDebug.php(270): MWDebug::sendRawDeprecated()
> #4 /var/mediawiki/includes/GlobalFunctions.php(1032): MWDebug::deprecated()
> #5 /var/mediawiki/includes/GlobalFunctions.php(2548): wfDeprecated()
> #6 /var/mediawiki/includes/MediaWiki.php(67): wfGetScriptUrl(string)
> #7 /var/mediawiki/load.php(50): MediaWiki->__construct()
== After (always) ==
> Use of wfGetScriptUrl was deprecated in MediaWiki 1.35. [Called from MediaWiki::__construct]
>
> Error from line 67 of /var/mediawiki/includes/MediaWiki.php
>
> #0 [internal function]: MWExceptionHandler::handleError()
> #1 /var/mediawiki/includes/debug/MWDebug.php(293): trigger_error()
> #2 /var/mediawiki/includes/debug/MWDebug.php(270): MWDebug::sendRawDeprecated()
> #3 /var/mediawiki/includes/GlobalFunctions.php(1038): MWDebug::deprecated()
> #4 /var/mediawiki/includes/GlobalFunctions.php(2548): wfDeprecated()
> #5 /var/mediawiki/includes/MediaWiki.php(67): wfGetScriptUrl(string)
> #6 /var/mediawiki/load.php(50): MediaWiki->__construct()
Bug: T252923
Change-Id: I1d4a166b6dff8b0e19fce3fab409f4a89e734ee6
