Page MenuHomePhabricator

Undo/redo on citations doesn't work correctly
Closed, ResolvedPublic

Description

  1. Change something in a citation.
  2. Click Undo.
  3. Click Redo.
  4. Click away. Why's that box still open?

It turns out that I can still edit (e.g., double-click another word and change its formatting), but I can't get rid of the box until I click the Cite button.

Reported by User:SUM1 at https://en.wikipedia.org/wiki/Wikipedia:VisualEditor/Feedback#Major_bug:_VisualEditor_stops_working_when_a_certain_sequence_of_operations_is_performed_to_a_citation

Event Timeline

Restricted Application added a project: VisualEditor. · View Herald TranscriptApr 25 2018, 6:00 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
  1. Change something in a citation.
  2. Click Undo.
  3. Click Redo.
  4. Click away. Why's that box still open?
  5. Click the Edit button on the box. Now it won't open, and now I can't save, even if I can do basic text formatting.
Elitre added a subscriber: Elitre.Apr 26 2018, 3:02 PM
Deskana triaged this task as High priority.Apr 26 2018, 3:09 PM
Deskana edited projects, added VisualEditor (Current work); removed VisualEditor.
Deskana added a subscriber: Deskana.

Following the above steps, when you click redo, there are two separate errors:

Uncaught Error: Removed data not as expected
    at checkExpected (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:401)
    at VeDmTreeModifier.ve.dm.TreeModifier.processRemove (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:402)
    at VeDmTreeModifier.ve.dm.TreeModifier.processOperation (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:395)
    at VeDmTreeModifier.ve.dm.TreeModifier.process (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:395)
    at VeDmTransactionProcessor.ve.dm.TransactionProcessor.process (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:347)
    at VeDmDocument.ve.dm.Document.commit (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:458)
    at VeDmSurface.ve.dm.Surface.changeInternal (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:420)
    at VeDmSurface.ve.dm.Surface.redo (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:422)
    at VeUiHistoryAction.ve.ui.HistoryAction.redo (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:886)
    at VeUiSurface.ve.ui.Surface.execute (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:818)
Uncaught TypeError: Cannot read property 'getInternalList' of null
    at VeDmMWReferenceNode.ve.dm.MWReferenceNode.getInternalItem (<anonymous>:742:871)
    at Object.ve.ui.MWCitationDialogTool.static.isCompatibleWith (<anonymous>:778:399)
    at collect (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:831)
    at VeUiContextItemFactory.ve.ui.ModeledFactory.getRelatedItems (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:831)
    at VeUiDesktopContext.ve.ui.LinearContext.getRelatedSources (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:827)
    at VeUiDesktopContext.ve.ui.LinearContext.isInspectable (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:826)
    at VeUiDesktopContext.ve.ui.LinearContext.afterContextChange (load.php?debug=false&lang=en&modules=ext.CodeMirror.lib|ext.visualEditor.core|oojs-ui-core%2Coojs-ui-widgets&skin=vector&version=0zbfnhw:825)
    at VeUiDesktopContext.ve.ui.DesktopContext.afterContextChange (<anonymous>:1136:342)

Exception is inside TreeModifier, pinging @dchan

This bug has been present for at least a year, probably since TreeModifier was introduced, and possibly before that.

dchan added a comment.Apr 29 2018, 2:52 AM

Ooooh, this happens because ve.dm.TransactionBuilder#pushAttributeChanges( { key: val }, ... ) puts a live reference to val into the Transaction object. So if val subsequently gets mutated, that invalidates the Transaction object.

Change 429649 had a related patch set uploaded (by Divec; owner: Divec):
[VisualEditor/VisualEditor@master] Copy attribute values when building transactions

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

Sounds about right, I assumed this was a shallow copy issue.

Change 429649 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Copy attribute values when building transactions

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

Esanders assigned this task to dchan.Apr 29 2018, 9:10 PM
Esanders moved this task from Incoming to Code review on the VisualEditor (Current work) board.
Deskana closed this task as Resolved.Apr 30 2018, 3:52 PM

Testing on the beta cluster, that seems to have fixed it.

Change 429832 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (099902ddb)

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

Change 429832 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (099902ddb)

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

Jdforrester-WMF set the point value for this task to 1.
Ryasmeen reopened this task as Open.May 1 2018, 8:35 PM
Ryasmeen added a subscriber: Ryasmeen.

I can still reproduce this on Beta cluster, re-opening it.

Okay this does not happen when you try with basic reference but for all other types of citations it is reproducible 100% time.

dchan added a comment.May 2 2018, 9:10 AM

Hmm, thanks, it seems I overlooked that there are a few different ways reference-instead-of-copy can happen here - investigating further.

Change 430413 had a related patch set uploaded (by Divec; owner: Divec):
[VisualEditor/VisualEditor@master] TreeModifier: Clone mutable linear data when inserting

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

dchan added a comment.May 2 2018, 4:53 PM

With change 430413, "change citation, undo, redo" seems to work fine in all the cases I've tried.

Great! Can we merge the fix now?

Change 430413 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] TreeModifier: Clone mutable linear data when inserting

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

Change 431590 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (c4ad0a7e3)

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

Change 431590 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (c4ad0a7e3)

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

Deskana closed this task as Resolved.May 8 2018, 10:14 AM
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptMay 8 2018, 10:14 AM
Vvjjkkii renamed this task from Undo/redo on citations doesn't work correctly to h8daaaaaaa.Jul 1 2018, 1:14 AM
Vvjjkkii reopened this task as Open.
Vvjjkkii removed dchan as the assignee of this task.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed the point value for this task.
Vvjjkkii removed subscribers: gerritbot, Aklapper.
Ryasmeen renamed this task from h8daaaaaaa to Undo/redo on citations doesn't work correctly.Jul 1 2018, 4:53 AM
Ryasmeen closed this task as Resolved.
Ryasmeen assigned this task to dchan.
Ryasmeen updated the task description. (Show Details)
Ryasmeen added subscribers: GerritBot, Aklapper.