Page MenuHomePhabricator

Switching from source to VE loses edit (un-savable and un-recoverable)
Closed, ResolvedPublicBUG REPORT

Description

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

What happens?:

There is an error message:

⧼The specified revision is deleted or suppressed.⧽

Screenshot 2022-10-27 at 21.54.02.png (870×1 px, 112 KB)

Of note is that the error message is rendered between angle brackets, which are usuaully indicate of an an error in resolving a localisable message key. E.g. ⧼edit-label⧽. The fact that those show up here is probably another bug in itself.

Of note is also that in this state, the edit is (to my knowledge) impossible to save or rescue in any way. Attempting to switch back to source mode yields the same error:

Screenshot 2022-10-27 at 21.56.52.png (362×1 px, 25 KB)

Event Timeline

@matmarex @xSavitar might this be related to changes to the VE extension as part of the RESTBase sunsetting work?

@matmarex @xSavitar might this be related to changes to the VE extension as part of the RESTBase sunsetting work?

Yeah, looks related.

I was able to reproduce this locally and it happens even if you don't convert from HTML -> Wikitext. From source mode directly to HTML causes this problem. I'm working on a fix. Let's see how it goes. Thanks for reporting @Krinkle. The regression was introduced by the DualParsoidClient: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/844465.

ppelberg subscribed.

Change 850598 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):

[mediawiki/extensions/VisualEditor@master] DirectParsoidClient: Don't access cache with revID of 0 in renderID

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

Let me try and follow what's going on.

  • when switching from source mode to visual, the client receives HTML along with an etag that has the rev ID 0 embedded, since the rendering is based on unsaved wikitext. The HTML should at this point be put in the stash, using the etag as the key.
  • when attempting to save the edit, the client sends the modified HTML back to the server, along with the etag that has rev ID 0 embedded in it.
  • In order to process the HTML, the server needs the original HTML, which it can get from the stash, using the etag as the key. I assume that things work fine up to this point.
  • In order to apply selser, the server also needs the base wikitext. It tries to look it up based on the revid embedded ,in the etag, which is 0. This fails.

(Note that this analysis is off the top of my head since I'm traveling, please tell me if I go this wrong).

If the above is a correct representation of the problem, I only see one solution: we need to stash the unsaved base wikitext along with the base HTML. This would require us to modify how stashing is handled and how attach wikitext to the PageBundle object. Doable, but potentially hacky. And also somewhat expensive.

Is RESTbase doing this right now? I didn't spot this behavior when looking at the Parosid glue code in RESTbase, but I find the logic there hard to follow. It's entirely possible that I missed it.... After all, we HAVE to store the modified wikitext somewhere if we don't want to lose cosmetic changes to wikitext when switching modes...

Btw, the rev ID embedded in the etag will also be 0 when creating a page. In that case, we can assume that the wikitext is the empty string.

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

[mediawiki/core@master] Stash original wikitext when rendering unsaved content.

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

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

[mediawiki/extensions/VisualEditor@master] Add mocha test for switching from source to visual mode

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

Maybe @Arlolra remembers how RESTBase handles this.

Is RESTbase doing this right now? I didn't spot this behavior when looking at the Parosid glue code in RESTbase, but I find the logic there hard to follow. It's entirely possible that I missed it.... After all, we HAVE to store the modified wikitext somewhere if we don't want to lose cosmetic changes to wikitext when switching modes...

I found the relevant code that stashes the wikitext, it's in the stashTransform() method in RESTbase's parsoid.js. The relevant ticket is T114548.

The structure it stashes is:

{
                'data-parsoid': original.body['data-parsoid'],
                wikitext: {
                    headers: { 'content-type': wtType },
                    body: req.body.wikitext
                },
                html: original.body.html
}

I think it's relevant to not that we only need to stash the wikitext when there is no revision ID. If there is a revision ID in the etag (resp. render ID), then the wikitext can be loaded from the respective revision.

Change 850637 merged by jenkins-bot:

[mediawiki/core@master] Stash original wikitext when rendering unsaved content.

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

Change 850638 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] api-testing: Add test for switching from source to visual mode

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

This should be fixed now, I'm leaving the ticket open until the new code has gone out with the train and and confirmed on testwiki.

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

[mediawiki/core@wmf/1.40.0-wmf.8] Stash original wikitext when rendering unsaved content.

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

Change 854073 merged by jenkins-bot:

[mediawiki/core@wmf/1.40.0-wmf.8] Stash original wikitext when rendering unsaved content.

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

Mentioned in SAL (#wikimedia-operations) [2022-11-08T14:57:15Z] <daniel@deploy1002> Started scap: Backport for [[gerrit:854073|Stash original wikitext when rendering unsaved content. (T321862)]]

Mentioned in SAL (#wikimedia-operations) [2022-11-08T14:57:35Z] <daniel@deploy1002> daniel and daniel: Backport for [[gerrit:854073|Stash original wikitext when rendering unsaved content. (T321862)]] synced to the testservers: mwdebug2001.codfw.wmnet, mwdebug1001.eqiad.wmnet, mwdebug2002.codfw.wmnet, mwdebug1002.eqiad.wmnet

Mentioned in SAL (#wikimedia-operations) [2022-11-08T15:04:40Z] <daniel@deploy1002> Finished scap: Backport for [[gerrit:854073|Stash original wikitext when rendering unsaved content. (T321862)]] (duration: 07m 25s)

Change 850598 abandoned by D3r1ck01:

[mediawiki/extensions/VisualEditor@master] DirectParsoidClient: Fake revision when parsoid renderID has 0 as revID

Reason:

Another approach went in :), thanks Daniel.

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