User story: N/A
We have this:
Everything is failing with a generic "error" message
We want this:
UploadWizard should function as normal.
Screenshots (if possible):
Acceptance Criteria:
- UploadWizard actually works on Beta
• Ramsey-WMF | |
May 31 2019, 6:17 PM |
F29283127: Screen Shot 2019-05-30 at 7.00.05 PM.png | |
May 31 2019, 6:17 PM |
User story: N/A
We have this:
Everything is failing with a generic "error" message
We want this:
UploadWizard should function as normal.
Screenshots (if possible):
Acceptance Criteria:
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Open | None | T231112 Rethink slow filters and profiling after enabling CachingParser | |||
Resolved | Nullzero | T231536 CachingParser::checkSyntax should only try to build the AST without evaluating it | |||
Resolved | Daimona | T234427 Improve CachingParser performance (2019) | |||
Resolved | Krinkle | T156095 Re-enable AbuseFilterCachingParser once we are sure it's safe | |||
Duplicate | BUG REPORT | None | T224746 [Beta Commons] All attempts to upload with UploadWizard fail |
API response is an internal fatal:
Argument 1 passed to AbuseFilterCachingParser::evalNode() must be an instance of AFPTreeNode, null given </div>
I'll check it. Anyway, AbuseFilterCachingParser is only used in test wikis, whereas production uses AbuseFilterParser. Thus, it's a beta-only issue. Of note, CachingParser is currently very bugged (T156095), and we're working on it these days.
I think it was enabled to test it and see what errors (like this one) could come out. The config patch is https://gerrit.wikimedia.org/r/#/c/operations/mediawiki-config/+/314604/, which doesn't have a real reason.
Since we're here, I guess we should first try to fix this specific bug, unless 1-it turns out to be complicated 2-it's absolutely necessary that it doesn't happen on beta commons anymore, in which case we can simply make beta commons use the usual parser.
Do we have a more detailed stacktrace? Is it available on beta logstash?
I could actually find it on beta logstash, we have
PHP Fatal Error: Argument 1 passed to AbuseFilterCachingParser::evalNode() must be an instance of AFPTreeNode, null given
#0 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/parser/AbuseFilterCachingParser.php(123): array_map(array, array) #1 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/parser/AbuseFilterCachingParser.php(230): AbuseFilterCachingParser->evalNode(AFPTreeNode) #2 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/parser/AbuseFilterCachingParser.php(66): AbuseFilterCachingParser->evalNode(AFPTreeNode) #3 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php(190): AbuseFilterCachingParser->intEval(string) #4 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/AbuseFilter.php(541): AbuseFilterParser->parse(string) #5 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/AbuseFilter.php(699): AbuseFilter::checkConditions(string, AbuseFilterVariableHolder, boolean, string) #6 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/AbuseFilter.php(600): AbuseFilter::checkFilter(stdClass, AbuseFilterVariableHolder, Title, string, string) #7 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/AbuseFilter.php(1195): AbuseFilter::checkAllFilters(AbuseFilterVariableHolder, Title, string, string) #8 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/AbuseFilterHooks.php(832): AbuseFilter::filterAction(AbuseFilterVariableHolder, Title, string, User) #9 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/AbuseFilterHooks.php(744): AbuseFilterHooks::filterUpload(string, UploadFromFile, User, array, NULL, NULL, NULL) #10 /srv/mediawiki/php-master/includes/Hooks.php(174): AbuseFilterHooks::onUploadStashFile(UploadFromFile, User, array, NULL) #11 /srv/mediawiki/php-master/includes/Hooks.php(202): Hooks::callHook(string, array, array, NULL) #12 /srv/mediawiki/php-master/includes/upload/UploadBase.php(1081): Hooks::run(string, array) #13 /srv/mediawiki/php-master/includes/upload/UploadBase.php(1061): UploadBase->runUploadStashFileHook(User) #14 /srv/mediawiki/php-master/includes/api/ApiUpload.php(315): UploadBase->tryStashFile(User, boolean) #15 /srv/mediawiki/php-master/includes/api/ApiUpload.php(162): ApiUpload->performStash(string, array) #16 /srv/mediawiki/php-master/includes/api/ApiUpload.php(135): ApiUpload->getStashResult(array) #17 /srv/mediawiki/php-master/includes/api/ApiUpload.php(104): ApiUpload->getContextResult() #18 /srv/mediawiki/php-master/includes/api/ApiMain.php(1595): ApiUpload->execute() #19 /srv/mediawiki/php-master/includes/api/ApiMain.php(531): ApiMain->executeAction() #20 /srv/mediawiki/php-master/includes/api/ApiMain.php(502): ApiMain->executeActionWithErrorHandling() #21 /srv/mediawiki/php-master/api.php(87): ApiMain->execute() #22 /srv/mediawiki/w/api.php(3): include(string) #23 {main}
Re-voicing from Daimona, I agree it'd be useful to keep the caching parser enabled in Beta so as to keep finding new issues and work on them.
But, if a specific issue we find is non-trivial to fix or we're unavailable, should be fine to disable during those dips, and then re-enable afterwards.
I forgot to say that I took a quick look at the issue. Unfortunately, we don't know what's the faulty filter (the parser is unaware), and the triggering line doesn't tell much. All I know ATM is that, during a function call, an argument is evaluated to null. We could use some heuristics to dig through the parser and try to figure out the exact code path, but I don't think it's worthwhile. Instead, I believe we should first try to find the faulty filter on beta commons, and then dig through the parser with a failing sample at hand. Note that I also manually hit "Check syntax" for all enabled filters, but none of them failed. So this is probably another example of T214643. Plus, given that it's related to function calls, I also used the filter search to see if any filter has dangling commas in function calls (which would be T153251). The answer is, apparently, no.
The first idea that comes to mind is, given that the error can consistently be reproduced, to bisect existing filters; i.e., disable half of them and try again, etc. Maybe this operation could be sped up by changing values in the DB directly. But even then, it won't be that quick. Suggestions are welcome.
@Ramsey-WMF As I said above, we first need a minimal test case. Beta Commons, being production-like, has several filters enabled, so it's not easy to find the culprit one. To summarize, I see three different ways to find the offending filter:
Whatever is better for you, I can help. For 1., I can start right now if you provide me the upload info (or we can coordinate e.g. on IRC so that I edit filters and you try to upload the file).
Hello @Daimona
Regarding the quoted section below:
We disable ~5 filters at a time, re-trying the faulty upload every time. I could do that myself (I'm already g-sysop on BC), but I'd need to know what to upload, with what title and comment, etc. in order to trigger the error (or someone else could do that while I disable filters)
The error is happening on Beta Commons on ALL uploads, so there's no particular title or comment or anything to add. Just try to upload any image to Beta Commons and you should get the error triggered.
OK, thanks. So I tried as above, and found that the culprit is filter 31, which is now disabled. Now it's also clear that this bug is a duplicate of T153251, which for some reason didn't have a stack trace.