Page MenuHomePhabricator

Saving edits with VE throws an error on Beta Cluster
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • Create/Edit any page with VisualEditor
  • Try to Save it

What happens?:
An error is thrown as shown in the screenshot

Screenshot 2022-09-19 at 13.14.10.png (381×563 px, 46 KB)

What should have happened instead?:
The publish action should have been successful

Software version (skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):
Further Observations
1️⃣ Switching from VisualEditor to Source Editor does not work sometimes. I get

Screenshot 2022-09-19 at 13.21.50.png (184×475 px, 8 KB)

2️⃣ Adding atopic or replying on Talk pages throw a 500
See
Screenshot 2022-09-19 at 13.34.01.png (242×541 px, 32 KB)

Related Objects

StatusSubtypeAssignedTask
ResolvedReleasejnuche
ResolvedBUG REPORTZabe

Event Timeline

Daimona renamed this task from Saving edits throws an error on Beta Cluster to Saving edits with VE throws an error on Beta Cluster.Sep 19 2022, 12:22 PM
Daimona triaged this task as Unbreak Now! priority.
Daimona added a project: VisualEditor.
Daimona updated the task description. (Show Details)
Daimona added a subscriber: Daimona.

I can reproduce this, although it's VE-only. Escalating to UBN to make sure that this gets enough attention, because AFAIK right now, it could be an actual bug that would reach production with the next train.

EAkinloose renamed this task from Saving edits with VE throws an error on Beta Cluster to Saving edits throws an error on Beta Cluster.Sep 19 2022, 12:35 PM
EAkinloose lowered the priority of this task from Unbreak Now! to Needs Triage.
EAkinloose updated the task description. (Show Details)

Unsurprisingly getting the same on clicking "Show changes"

image.png (955×1 px, 159 KB)

@EAkinloose Just noting, given the task rename, that I can save successfully using a normal source edit — this does seem to only affect VE?

@EAkinloose Just noting, given the task rename, that I can save successfully using a normal source edit — this does seem to only affect VE?

Yeah, I assume it's just a classic edit conflict. Phab doesn't handle them as gracefully as MW.

Daimona renamed this task from Saving edits throws an error on Beta Cluster to Saving edits with VE throws an error on Beta Cluster.Sep 19 2022, 12:40 PM
Daimona triaged this task as Unbreak Now! priority.
Daimona updated the task description. (Show Details)

@EAkinloose Just noting, given the task rename, that I can save successfully using a normal source edit — this does seem to only affect VE?

Yeah, I assume it's just a classic edit conflict. Phab doesn't handle them as gracefully as MW.

(ah, my bad!)

Random guess: maybe caused by https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762 ? It touches the API modules, and particularly some bits where they do error handling.

Random guess: maybe caused by https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762 ? It touches the API modules, and particularly some bits where they do error handling.

FTR, I was talking about https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762/14/includes/ApiParsoidTrait.php#87 specifically. If, say, $response['error'] is always set in the new API but it's a falsey value when there's no error, I think it would die with the empty string as error msg, which is compatible with the symptoms.

A relevant (?) logstash entry

Error
message
Invalid API error code ""
normalized_message
Invalid API error code "{code}"
exception.trace
from /srv/mediawiki/php-master/includes/api/ApiUsageException.php(71)
#0 /srv/mediawiki/php-master/includes/api/ApiBase.php(1454): ApiUsageException::newWithMessage(MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit, string, NULL, NULL, integer)
#1 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiParsoidTrait.php(89): ApiBase->dieWithError(string)
#2 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiParsoidTrait.php(133): MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit->forwardErrorsAndCacheHeaders(array)
#3 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiVisualEditorEdit.php(278): MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit->transformHTML(Title, string, NULL, NULL)
#4 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiVisualEditorEdit.php(252): MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit->getWikitextNoCache(Title, array, array)
#5 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiVisualEditorEdit.php(442): MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit->getWikitext(Title, array, array)
#6 /srv/mediawiki/php-master/includes/api/ApiMain.php(1900): MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit->execute()
#7 /srv/mediawiki/php-master/includes/api/ApiMain.php(875): ApiMain->executeAction()
#8 /srv/mediawiki/php-master/includes/api/ApiMain.php(846): ApiMain->executeActionWithErrorHandling()
#9 /srv/mediawiki/php-master/api.php(90): ApiMain->execute()
#10 /srv/mediawiki/php-master/api.php(45): wfApiMain()
#11 /srv/mediawiki/w/api.php(3): require(string)
#12 {main}

FTR, I was talking about https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762/14/includes/ApiParsoidTrait.php#87 specifically. If, say, $response['error'] is always set in the new API but it's a falsey value when there's no error, I think it would die with the empty string as error msg, which is compatible with the symptoms.

The above Invalid API error code "" seems to somewhat confirm your findings @Daimona?

A relevant (?) logstash entry
[...]
#1 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiParsoidTrait.php(89): ApiBase->dieWithError(string)

So I imagine my guess

FTR, I was talking about https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762/14/includes/ApiParsoidTrait.php#87 specifically. If, say, $response['error'] is always set in the new API but it's a falsey value when there's no error, I think it would die with the empty string as error msg, which is compatible with the symptoms.

could very well be correct, and the "falsey value" would be the empty string. CC'ing people from that gerrit change.

Change 833003 had a related patch set uploaded (by Zabe; author: Zabe):

[mediawiki/extensions/VisualEditor@master] Only die when error string is not empty

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

Looking at the relevant code, it seems that the only path which could potentially cause this error is the one leading to VRSParsoidClient::requestRestbase. Its documentation says:

@return array If successful, the value is the RESTbase server's response as an array with keys 'code', 'error', 'headers' and 'body'

So it does look like "error" is always set. That same method checks it using empty(). For a quick fix, I assume ApiParsoidTrait could do the same (instead of isset()). For a longer-term fix, I think requestRestbase() should either

  1. Not include "error" if there's no error, or
  2. Document what its value is when there's no error (i.e., empty string, false, null, etc.)

Using empty() could potentially be wrong in the edge (very edge) case where a falsey value is a valid error code (e.g., '0'). I don't think that could really happen, but knowing what to compare the value against would make the code more readable IMHO.

A relevant (?) logstash entry
[...]
#1 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiParsoidTrait.php(89): ApiBase->dieWithError(string)

So I imagine my guess

FTR, I was talking about https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762/14/includes/ApiParsoidTrait.php#87 specifically. If, say, $response['error'] is always set in the new API but it's a falsey value when there's no error, I think it would die with the empty string as error msg, which is compatible with the symptoms.

could very well be correct, and the "falsey value" would be the empty string. CC'ing people from that gerrit change.

Yes, the error value is an empty string, see https://gerrit.wikimedia.org/g/mediawiki/core/+/cf950f5ea3c3355c9e1374450d1012aacae6022a/includes/libs/http/MultiHttpClient.php#596.

I have tested locally and the commit before https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762 works fine. I think the break is related to this commit.

Change 833003 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Only die when error string is not empty

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

Zabe claimed this task.

I was capable of doing this, so I think this has been fixed.

Same issue wound up being a problem in the related patch to change away from ParsoidHelper in DiscussionTools. I'd agree with @Daimona that $response['error'] being an empty string is confusing and should at least be documented as a migration issue.

Change 833069 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/extensions/VisualEditor@master] ParsoidClient: error should be array or null

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

Change 833069 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] ParsoidClient: error should be array or null

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