Page MenuHomePhabricator
Create Task
Maniphest T299583

Wikimedia\Assert\PreconditionException: Precondition failed: Range is not collapsed
Closed, ResolvedPublicPRODUCTION ERROR
Actions

Description

Error
normalized_message
[{reqId}] {exception_url}   Wikimedia\Assert\PreconditionException: Precondition failed: Range is not collapsed
exception.trace
from /srv/mediawiki/php-1.38.0-wmf.18/vendor/wikimedia/assert/src/Assert.php(49)
#0 /srv/mediawiki/php-1.38.0-wmf.18/extensions/DiscussionTools/includes/CommentUtils.php(525): Wikimedia\Assert\Assert::precondition(boolean, string)
#1 /srv/mediawiki/php-1.38.0-wmf.18/extensions/DiscussionTools/includes/ThreadItem.php(133): MediaWiki\Extension\DiscussionTools\CommentUtils::getRangeFirstNode(MediaWiki\Extension\DiscussionTools\ImmutableRange)
#2 /srv/mediawiki/php-1.38.0-wmf.18/extensions/DiscussionTools/includes/HeadingItem.php(109): MediaWiki\Extension\DiscussionTools\ThreadItem->getTranscludedFrom()
#3 /srv/mediawiki/php-1.38.0-wmf.18/extensions/DiscussionTools/includes/ApiDiscussionTools.php(52): MediaWiki\Extension\DiscussionTools\HeadingItem->getTranscludedFrom()
#4 /srv/mediawiki/php-1.38.0-wmf.18/includes/api/ApiMain.php(1889): MediaWiki\Extension\DiscussionTools\ApiDiscussionTools->execute()
#5 /srv/mediawiki/php-1.38.0-wmf.18/includes/api/ApiMain.php(868): ApiMain->executeAction()
#6 /srv/mediawiki/php-1.38.0-wmf.18/includes/api/ApiMain.php(839): ApiMain->executeActionWithErrorHandling()
#7 /srv/mediawiki/php-1.38.0-wmf.18/api.php(90): ApiMain->execute()
#8 /srv/mediawiki/php-1.38.0-wmf.18/api.php(45): wfApiMain()
#9 /srv/mediawiki/w/api.php(3): require(string)
#10 {main}
Impact

~45 occurrences within 45 min of deploying to group1

Notes

Details

Request URL
https://meta.wikimedia.org/w/api.php?action=discussiontools&format=*&formatversion=*&uselang=*&paction=*&page=*&oldid=*
SubjectRepoBranchLines +/-
Prevent assertion failure caused by empty headingsmediawiki/extensions/DiscussionToolswmf/1.38.0-wmf.18+5 -0
Prevent assertion failure caused by empty headingsmediawiki/extensions/DiscussionToolsmaster+5 -0
Customize query in gerrit

Related Objects
Search...

Event Timeline

jeena created this task.Jan 19 2022, 8:43 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 19 2022, 8:43 PM
Lucas_Werkmeister_WMDE added a project: DiscussionTools.Jan 20 2022, 1:20 PM
ppelberg added a project: Editing-team (Kanban Board).Jan 20 2022, 4:05 PM
ppelberg moved this task from Incoming to Ready to Be Worked On on the Editing-team (Kanban Board) board.
ppelberg moved this task from Backlog to Triaged on the DiscussionTools board.
thcipriani added a parent task: T293959: 1.38.0-wmf.18 deployment blockers.Jan 20 2022, 4:11 PM
thcipriani triaged this task as Unbreak Now! priority.Jan 20 2022, 4:18 PM
thcipriani subscribed.

Adding this as a train blocker as it's:

  1. A new error with wmf.18
  2. reproducible by going to the Main Page of metawiki.
Zabe subscribed.Jan 20 2022, 4:19 PM
Krinkle added a project: Performance-Team (Radar).EditedJan 20 2022, 4:22 PM
Krinkle subscribed.

It appears the DT javascript layload is making API requests on all page views. E.g. viewing pages like https://www.mediawiki.org/wiki/ResourceLoader results in an uncached and unconditional background request. That seems likely unintentional and could become a performance problem if widespread.

I'm noticing it actual talk pages as well, e.g. https://meta.wikimedia.org/wiki/User_talk:Krinkle, and for logged-out users as well, where it's making three different API queries, each one private/uncacheable.

I'm not sure what this is for and whether it's by design that this is needed during the critical path. If not, it should be defered until interaction. Otherwise, if it is critical for the initial binding of click handlers, then it seems like this information may be worth including into the page metadata directly from the server-side.

Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.Jan 20 2022, 4:22 PM
matmarex claimed this task.Jan 20 2022, 6:11 PM
matmarex added a comment.Jan 20 2022, 6:50 PM

The issue occurs because some of the headings on the main page on Meta-Wiki have no content in the Parsoid HTML. This shouldn't happen.

Normal read modeParsoid HTML
https://meta.wikimedia.org/wiki/Main_Pagehttps://meta.wikimedia.org/api/rest_v1/page/html/Main_Page
image.png (2×3 px, 550 KB)
image.png (2×3 px, 476 KB)

We have some code that depends on the headings… existing, and this page is tripping this assertion.

I don't think this is likely to cause any real problems for users, and I don't think it's a train blocker. But I'll prepare a patch so that our code doesn't spam the logs.

gerritbot added a comment.Jan 20 2022, 6:55 PM

Change 755752 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/DiscussionTools@master] Prevent assertion failure caused by empty headings

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

gerritbot added a project: Patch-For-Review.Jan 20 2022, 6:55 PM
matmarex mentioned this in T299685: Some headings are missing on the main page on Meta-Wiki in Parsoid rendering.Jan 20 2022, 7:10 PM
matmarex added a comment.Jan 20 2022, 7:20 PM
In T299583#7636971, @Krinkle wrote:

It appears the DT javascript layload is making API requests on all page views. E.g. viewing pages like https://www.mediawiki.org/wiki/ResourceLoader results in an uncached and unconditional background request. That seems likely unintentional and could become a performance problem if widespread.

I'm noticing it actual talk pages as well, e.g. https://meta.wikimedia.org/wiki/User_talk:Krinkle, and for logged-out users as well, where it's making three different API queries, each one private/uncacheable.

I'm not sure what this is for and whether it's by design that this is needed during the critical path. If not, it should be defered until interaction. Otherwise, if it is critical for the initial binding of click handlers, then it seems like this information may be worth including into the page metadata directly from the server-side.

It's only making those requests on discussion pages; however, on Meta-Wiki and MediaWiki.org, the main namespace is treated like a discussion page for annoying historical reasons (this was previously discussed in T291630). This is not the case on most wikis.

The API requests are not strictly needed, but pre-loading them allows the reply tool to load faster when the user actually requests it. (There's actually a TODO comment in the code to reconsider this: https://github.com/wikimedia/mediawiki-extensions-DiscussionTools/blob/7f329ca9a2a30eba79d3294d956246839c1cd705/modules/controller.js#L878-L883.) If you think this is bad, please file a bug. It doesn't seem too bad to me.

I filed T299685 about the incorrect rendering in Parsoid that triggered this problem.

matmarex moved this task from Ready to Be Worked On to Code Review on the Editing-team (Kanban Board) board.Jan 20 2022, 7:22 PM
gerritbot added a comment.Jan 20 2022, 7:50 PM

Change 755752 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Prevent assertion failure caused by empty headings

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

ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.19; 2022-01-24).Jan 20 2022, 8:00 PM
Maintenance_bot removed a project: Patch-For-Review.Jan 20 2022, 8:10 PM
gerritbot added a comment.Jan 20 2022, 8:13 PM

Change 755684 had a related patch set uploaded (by Jeena Huneidi; author: Bartosz Dziewoński):

[mediawiki/extensions/DiscussionTools@wmf/1.38.0-wmf.18] Prevent assertion failure caused by empty headings

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

gerritbot added a project: Patch-For-Review.Jan 20 2022, 8:13 PM
gerritbot added a comment.Jan 20 2022, 8:24 PM

Change 755684 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@wmf/1.38.0-wmf.18] Prevent assertion failure caused by empty headings

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

Stashbot added a comment.Jan 20 2022, 8:31 PM

Mentioned in SAL (#wikimedia-operations) [2022-01-20T20:31:31Z] <jhuneidi@deploy1002> Synchronized php-1.38.0-wmf.18/extensions/DiscussionTools/includes/HeadingItem.php: Backport: [[gerrit:755684|Prevent assertion failure caused by empty headings (T299583)]] (duration: 00m 50s)

matmarex moved this task from Code Review to Ready for Sign Off on the Editing-team (Kanban Board) board.Jan 20 2022, 8:42 PM
ReleaseTaggerBot edited projects, added MW-1.38-notes (1.38.0-wmf.18; 2022-01-17); removed MW-1.38-notes (1.38.0-wmf.19; 2022-01-24).Jan 20 2022, 9:00 PM
Maintenance_bot removed a project: Patch-For-Review.Jan 20 2022, 9:10 PM
ppelberg closed this task as Resolved.Jan 21 2022, 3:46 AM
Krinkle moved this task from Untriaged to Jan 2022 on the Wikimedia-production-error board.Feb 4 2022, 12:45 AM
brennen reopened this task as Open.Mar 21 2022, 9:13 PM
brennen subscribed.

Seeing this again in 1.39.0-wmf.2 (T300203):

Error
normalized_message
[{reqId}] {exception_url}   Wikimedia\Assert\PreconditionException: Precondition failed: Range is not collapsed
exception.trace
from /srv/mediawiki/php-1.39.0-wmf.2/vendor/wikimedia/assert/src/Assert.php(49)
#0 /srv/mediawiki/php-1.39.0-wmf.2/extensions/DiscussionTools/includes/CommentUtils.php(552): Wikimedia\Assert\Assert::precondition(boolean, string)
#1 /srv/mediawiki/php-1.39.0-wmf.2/extensions/DiscussionTools/includes/CommentUtils.php(663): MediaWiki\Extension\DiscussionTools\CommentUtils::getRangeFirstNode(MediaWiki\Extension\DiscussionTools\ImmutableRange)
#2 /srv/mediawiki/php-1.39.0-wmf.2/extensions/DiscussionTools/includes/CommentUtils.php(609): MediaWiki\Extension\DiscussionTools\CommentUtils::compareRangesAlmostEqualBoundaries(MediaWiki\Extension\DiscussionTools\ImmutableRange, MediaWiki\Extension\DiscussionTools\ImmutableRange, string)
#3 /srv/mediawiki/php-1.39.0-wmf.2/extensions/DiscussionTools/includes/CommentUtils.php(739): MediaWiki\Extension\DiscussionTools\CommentUtils::compareRanges(MediaWiki\Extension\DiscussionTools\ImmutableRange, MediaWiki\Extension\DiscussionTools\ImmutableRange)
#4 /srv/mediawiki/php-1.39.0-wmf.2/extensions/DiscussionTools/includes/ApiDiscussionToolsPreview.php(61): MediaWiki\Extension\DiscussionTools\CommentUtils::isSingleCommentSignedBy(MediaWiki\Extension\DiscussionTools\ThreadItemSet, string, Wikimedia\Parsoid\DOM\Element)
#5 /srv/mediawiki/php-1.39.0-wmf.2/includes/api/ApiMain.php(1893): MediaWiki\Extension\DiscussionTools\ApiDiscussionToolsPreview->execute()
#6 /srv/mediawiki/php-1.39.0-wmf.2/includes/api/ApiMain.php(870): ApiMain->executeAction()
#7 /srv/mediawiki/php-1.39.0-wmf.2/includes/api/ApiMain.php(841): ApiMain->executeActionWithErrorHandling()
#8 /srv/mediawiki/php-1.39.0-wmf.2/api.php(90): ApiMain->execute()
#9 /srv/mediawiki/php-1.39.0-wmf.2/api.php(45): wfApiMain()
#10 /srv/mediawiki/w/api.php(3): require(string)
#11 {main}
matmarex closed this task as Resolved.Mar 21 2022, 10:35 PM
matmarex mentioned this in T304377: Wikimedia\Assert\PreconditionException: Precondition failed: Range is not collapsed (from CommentUtils::isSingleCommentSignedBy).

I made a separate task for this: T304377, because the stack trace is different. This exception is coming from the newly introduced method CommentUtils::isSingleCommentSignedBy() (added in rEDTO0ecc8a4c051d: Improve detecting already signed comments).

Also it looks like there are only 8 occurrences in total, so it doesn't seem like a train blocker.

Krinkle mentioned this in T325598: Avoid uncached action=discussiontoolspageinfo API calls on page load.Dec 21 2022, 12:57 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL