Page MenuHomePhabricator

Special:Log UnexpectedValueException after changes in HTMLTitleTextField and MapCacheLRU
Closed, ResolvedPublic

Description

https://en.wikipedia.beta.wmflabs.org/wiki/Special:Log is broken:

[W01XLwpEEj4AAA7ee@EAAAAF] /wiki/Special:Log UnexpectedValueException from line 144 of /srv/mediawiki/php-master/includes/libs/MapCacheLRU.php: MapCacheLRU::has called with invalid key. Must be string or integer.

Backtrace:

#0 /srv/mediawiki/php-master/includes/libs/MapCacheLRU.php(163): MapCacheLRU->has(NULL)
#1 /srv/mediawiki/php-master/includes/Title.php(318): MapCacheLRU->get(NULL)
#2 /srv/mediawiki/php-master/includes/htmlform/fields/HTMLTitleTextField.php(51): Title::newFromTextThrow(NULL)
#3 /srv/mediawiki/php-master/includes/htmlform/HTMLFormField.php(862): HTMLTitleTextField->validate(NULL, array)
#4 /srv/mediawiki/php-master/includes/htmlform/HTMLFormField.php(476): HTMLFormField->getErrorsAndErrorClass(NULL)
#5 /srv/mediawiki/php-master/includes/htmlform/HTMLForm.php(1668): HTMLFormField->getTableRow(NULL)
#6 /srv/mediawiki/php-master/includes/htmlform/HTMLForm.php(1250): HTMLForm->displaySection(array, string)
#7 /srv/mediawiki/php-master/includes/htmlform/HTMLForm.php(1040): HTMLForm->getBody()
#8 /srv/mediawiki/php-master/includes/htmlform/HTMLForm.php(1020): HTMLForm->getHTML(boolean)
#9 /srv/mediawiki/php-master/includes/logging/LogEventsList.php(168): HTMLForm->displayForm(boolean)
#10 /srv/mediawiki/php-master/includes/specials/SpecialLog.php(255): LogEventsList->showOptions(array, string, string, boolean, NULL, NULL, NULL, array, string, string)
#11 /srv/mediawiki/php-master/includes/specials/SpecialLog.php(147): SpecialLog->show(FormOptions, array)
#12 /srv/mediawiki/php-master/includes/specialpage/SpecialPage.php(566): SpecialLog->execute(NULL)
#13 /srv/mediawiki/php-master/includes/specialpage/SpecialPageFactory.php(569): SpecialPage->run(NULL)
#14 /srv/mediawiki/php-master/includes/MediaWiki.php(288): SpecialPageFactory::executePath(Title, RequestContext)
#15 /srv/mediawiki/php-master/includes/MediaWiki.php(867): MediaWiki->performRequest()
#16 /srv/mediawiki/php-master/includes/MediaWiki.php(524): MediaWiki->main()
#17 /srv/mediawiki/php-master/index.php(42): MediaWiki->run()
#18 /srv/mediawiki/w/index.php(3): include(string)
#19 {main}

Recent changes in HTMLTitleTextField and MapCacheLRU.


Other pages affected:

Event Timeline

Prtksxna created this task.Jul 17 2018, 2:59 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 17 2018, 2:59 AM
Prtksxna updated the task description. (Show Details)Jul 17 2018, 3:16 AM
Prtksxna updated the task description. (Show Details)Jul 17 2018, 4:37 AM
Krinkle added a comment.EditedJul 17 2018, 4:45 AM

The commit in question (correctly) enforces a restriction that was previously implied. The restriction being that: cache keys cannot be null. Many code paths were already enforcing this in their own code, but it seems we found one method used on Special:Log that wasn't accounting for this.

As such, it causes the exception to be thrown. We can temporarily revert that, but ultimately the commit can't be improved or done differently.

The HTMLTitleTextField class is passing null as a title to validate() which goes unfiltered and makes its way to Title::newFromTextThrow. That method happens to work by accident, because it is interpreting null as "Null" and treating that as if it was an article page name (e.g. [[Null]]), and says it is "valid".

That code is broken and needs to be fixed.

To confirm this I have restored my MediaWiki install to before f88a5a18155a (git checkout -b test 33912421), and confirmed Special:Log still works, and then cherry-picked 7a25cd388c8a (git cherry-pick 7a25cd388c8a), and confirmed Special:Log still works.

Change 446216 had a related patch set uploaded (by Prtksxna; owner: Prtksxna):
[mediawiki/core@master] HTMLTitleTextField: Also check is_null when checking if value is empty

https://gerrit.wikimedia.org/r/446216

Prtksxna updated the task description. (Show Details)Jul 17 2018, 5:09 AM

Change 446228 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] Revert "Convert Title::getTitleCache() to using MapCacheLRU"

https://gerrit.wikimedia.org/r/446228

Change 446228 merged by jenkins-bot:
[mediawiki/core@master] Revert "Convert Title::getTitleCache() to using MapCacheLRU"

https://gerrit.wikimedia.org/r/446228

Special:Log works for me, 446228 is merged, can this be resolved?

To fully fix the issue, we need to fix HTMLTitleTextField (https://gerrit.wikimedia.org/r/#/c/446216/) and then revert the revert of rMW7a25cd388c8a: Convert Title::getTitleCache() to using MapCacheLRU.

I think it is not blocking the deployment any more, we need to do some more work to clean up the tech debt, but master is in a stable state right now.

Change 446510 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] Revert "Revert "Convert Title::getTitleCache() to using MapCacheLRU""

https://gerrit.wikimedia.org/r/446510

Change 446510 abandoned by Bartosz Dziewoński:
Revert "Revert "Convert Title::getTitleCache() to using MapCacheLRU""

Reason:
Duplicate of I7fe8bbed3fb24240e47ba22de57da9dc8a9bea22

https://gerrit.wikimedia.org/r/446510

Change 446240 had a related patch set uploaded (by Krinkle; owner: Aaron Schulz):
[mediawiki/core@master] [DNM] Convert Title::getTitleCache() to using MapCacheLRU (again)

https://gerrit.wikimedia.org/r/446240

Change 446216 merged by jenkins-bot:
[mediawiki/core@master] HTMLTitleTextField, HTMLUserTextField: Check for null before using the value

https://gerrit.wikimedia.org/r/446216

Change 446240 merged by jenkins-bot:
[mediawiki/core@master] Convert Title::getTitleCache() to using MapCacheLRU (again)

https://gerrit.wikimedia.org/r/446240

matmarex closed this task as Resolved.Jul 19 2018, 6:39 PM
matmarex assigned this task to Prtksxna.
matmarex removed a project: Patch-For-Review.