Last modified: 2013-08-22 14:55:07 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T43352, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 41352 - Text loss on edit page
Text loss on edit page
Status: VERIFIED FIXED
Product: MediaWiki
Classification: Unclassified
ContentHandler (Other open bugs)
unspecified
All All
: High major (vote)
: ---
Assigned To: Wikidata bugs
: code-update-regression
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-24 15:47 UTC by Aude
Modified: 2013-08-22 14:55 UTC (History)
13 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Diff between 0f8a08acb4 and I0b1db6d7 on includes/EditPage.php (5.68 KB, patch)
2012-10-29 15:30 UTC, Bartosz Dziewoński
Details

Description Aude 2012-10-24 15:47:04 UTC
There still appears a problem that happens sometimes (not sure exactly how it occurs) when saving pages, similar to the one with edit conflicts in bug 41280.

https://bugzilla.wikimedia.org/41280

Some reports of this issue:

http://commons.wikimedia.org/wiki/Commons:Village_pump#Edit_conflict_issues

http://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#Apparent_bug_causing_massive_text_loss_when_saving
Comment 1 Daniel Kinzler 2012-10-24 15:49:05 UTC
I have tried and failed to reproduce this issue. The best we can do for now is ask people to report and look for patterns.
Comment 2 Richard Guk 2012-10-24 19:34:15 UTC
Another recent example of loss of all text outside the section being edited, as reported by [[User:Ryan_Vesey]] at [[WP:VPT#Edit_conflicts_with_other_sections.2C_new_issue]]:

http://en.wikipedia.org/w/index.php?title=Talk:Jesus&diff=prev&oldid=519620328
Comment 3 Aude 2012-10-24 20:33:53 UTC
here's a temporary fix that should resolve the problem:

https://gerrit.wikimedia.org/r/#/c/29872/

the merge implementation in TextContentHandler is a buggy, and causing issues I think with wfMerge3()

the temporary fix uses the pre-contenthandler version of the mergeChangesInto EditPage function where it attempts to resolve edit conflicts.  This appears to work and fix the problem.

The better solution, of course, is to fix the issue in the content handler but I want a quick fix now.
Comment 4 Sumana Harihareswara 2012-10-24 21:03:13 UTC
Both Gerrit change # 29872 and https://gerrit.wikimedia.org/r/#/c/29875/ (the backport to 1.21wmf2) have been merged.  But MatmaRex commented in Gerrit on the backport:

"I belive that this causes the following fatal when an edit conflict would happen:

PHP fatal error in /usr/local/apache/common-local/php-1.21wmf2/includes/content/ContentHandler.php line 69:
Argument 1 passed to ContentHandler::getContentText() must implement interface Content, string given, called in /usr/local/apache/common-local/php-1.21wmf2/includes/EditPage.php on line 1479 and defined"
Comment 5 Bartosz Dziewoński 2012-10-24 21:07:52 UTC
I511b7866 (by Aude) is supposed to fix the fatal.
Comment 6 Aude 2012-10-24 21:08:26 UTC
https://gerrit.wikimedia.org/r/#/c/29881/

sorry about that!
Comment 7 Bartosz Dziewoński 2012-10-24 21:49:39 UTC
Appears fixed on pl.wiki. Thanks.
Comment 8 Sumana Harihareswara 2012-10-24 21:52:01 UTC
https://gerrit.wikimedia.org/r/#/c/29887/ is the backport to 1.21wmf2 by the way.
Comment 9 Aude 2012-10-25 07:38:37 UTC
How i was able to reproduce the bug, with such article text:

1)

intro

==Section 1==
He was born in 1980.
He likes ice cream.

==Section 2==
blah blah blah...

2) User A clicks edit (such as section 1)

3) User B clicks edit (e.g. section 1)

4) User B change section 1 to and saves.

He was born in 1990.
He likes ices cream.

5) User A changes section 1 to:

He was born in 1980.
He likes cookies.

6) User A hits save.

It should auto merge that the entire page into:

intro

==Section 1==
He was born in 1990.
He likes cookies.

==Section 2==
blah blah blah.

With the bug, it instead automerged the section and saved just the section of the page, erasing the rest of the page, so users would see:

==Section 1==
He was born in 1990.
He likes cookies.

The issue appears to be TextContent object being passed to the wfMerge3 function in TextContentHandler.  It also appears that edit conflicts were happening much more often, such as the mergeChangesIntoContent function in EditPage returning false more often when it should auto merge successfully.

I just wanted the quickest fix that would work, since this was a live bug on Wikipedia, but sure now that we understand the problem, it can be fixed correctly (forward-compatible) without too much difficulty.
Comment 10 Daniel Kinzler 2012-10-25 09:16:13 UTC
Some observations:

* Looking at textContentHandler::merge3, I don't see any way how a TextContent object could be passed to wfMerge. 
* Looking at WikitextContentHandlerTest, there is testMerge3, a test case for checking the auto merge functionality. This test is routinely called and passes.

So the problem has to lie somewhere else, probably in the way EditPage handles auto-merging of conflicts of *sections*. I'll investigate some more.
Comment 11 Aude 2012-10-25 09:48:11 UTC
it seems to matter whether User A or User B hits save first.

With User B, I get the blanking.

With User A saving first, I get edit conflict.

This is consistent with reports from Wikipedians that they were getting edit conflicts more often, along with reports of blanking content.
Comment 12 Dweller 2012-10-25 14:44:18 UTC
I just got this problem as an edit conflict on en:
Using Firefox 11.0 if that helps.
Comment 13 Daniel Kinzler 2012-10-25 16:10:20 UTC
Wrote test cases for EditPage, including conflict resolution: I8f23c653

I have the issue isolated now, should be able to fix it until tomorrow.
Comment 14 Daniel Kinzler 2012-10-25 17:04:40 UTC
Proper fix posted as I0b1db6d7
Comment 15 Andre Klapper 2012-10-29 09:09:39 UTC
ping - could anybody review? Would be great to get this in for wmf3.
Comment 16 Rob Lanphier 2012-10-29 15:18:42 UTC
Hi Daniel, could you explain your fix a little bit (possibly including a diff between the version before aude's change and your new version)?  It's pretty difficult to piece this together just from gerrit, but I think I figured it out.  If I'm reading things right, you removed this:
> $contentObj = $content; 

...and this line:
> $this->textbox1 = ContentHandler::getContentText( $contentObj );

...was changed to:
> $this->textbox1 = ContentHandler::getContentText( $content );

Is that indeed what changed?  If so, why does what you did fix this?
Comment 17 Bartosz Dziewoński 2012-10-29 15:30:43 UTC
Created attachment 11257 [details]
Diff between 0f8a08acb4 and I0b1db6d7 on includes/EditPage.php

Uploading a diff between 0f8a08acb4 (the change before Chad's revert I4c2055be) and I0b1db6d7 (the latest patchset) on includes/EditPage.php.

(Generated using git diff 0f8a08a c9d972a -- includes\EditPage.php)
Comment 18 Daniel Kinzler 2012-10-29 15:48:22 UTC
(In reply to comment #16)
> ...and this line:
> > $this->textbox1 = ContentHandler::getContentText( $contentObj );
> 
> ...was changed to:
> > $this->textbox1 = ContentHandler::getContentText( $content );
> 
> Is that indeed what changed?  If so, why does what you did fix this?

No. Essentially, I replaced this:

> if ( $this->mergeChangesIntoContent( $textbox_content ) ) { ... }

By this:

> if ( $this->mergeChangesIntoContent( $content ) ) { ... }

The rest is just cleanup.

$textbox_content is holding the content as submitted - for a section edit, just the content of the section. $content holds the full page content before the edit. mergeChangesIntoContent( $content ) merges the new changes into the old content, as it should be. mergeChangesIntoContent( $textbox_content ) merged the new changes into the new section content, which does not make sense, and also loses all content outside that section.

This was a simple oversight - I got confused about which variable holds what and when.
Comment 19 Aude 2012-10-29 15:49:15 UTC
(edit conflict!)

I see what Daniel did and it works, makes sense, etc.  Crazy that I didn't figure this exactly out last week. :/

$textbox_content is whatever was in the editing text box.  If one clicks the section edit link, then it's just the section.  If textbox_content is just a section, replaceSectionContent creates the entire content with the new section content.

for merging conflicts, $content should be used not $textbox_content.
Comment 20 Bartosz Dziewoński 2012-10-31 16:46:20 UTC
Reopening, I0b1db6d7 (the proper fix) is not merged yet.
Comment 21 db [inactive,noenotif] 2012-11-11 13:25:49 UTC
Change I0b1db6d7: Status Merged
Comment 22 denny vrandecic 2013-08-22 14:55:07 UTC
Closed older resolved bugs as verified.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links