Page MenuHomePhabricator

ve.dm.InternalList#sortGroupIndexes throws javascript error
Closed, ResolvedPublic

Description

Observed the following javascript error while debugging T190917: CX2: Fix issues in paragraph alignment

jQuery.Deferred exception: Cannot read property 'getRange' of undefined TypeError: Cannot read property 'getRange' of undefined
    at http://localhost/wiki/extensions/VisualEditor/lib/ve/src/dm/ve.dm.InternalList.js?a92bc:434:83
    at Array.sort (native)
    at VeDmInternalList.ve.dm.InternalList.sortGroupIndexes (http://localhost/wiki/extensions/VisualEditor/lib/ve/src/dm/ve.dm.InternalList.js?a92bc:425:19)
    at VeDmInternalList.ve.dm.InternalList.onTransact (http://localhost/wiki/extensions/VisualEditor/lib/ve/src/dm/ve.dm.InternalList.js?a92bc:377:9)
    at VeDmDocument.oo.EventEmitter.emit (http://localhost/wiki/resources/lib/oojs/oojs.jquery.js?4bc88:829:12)
    at VeDmDocument.ve.dm.Document.commit (http://localhost/wiki/extensions/VisualEditor/lib/ve/src/dm/ve.dm.Document.js?27bdb:330:7)
    at VeDmSurface.ve.dm.Surface.changeInternal (http://localhost/wiki/extensions/VisualEditor/lib/ve/src/dm/ve.dm.Surface.js?7b00b:877:25)
    at VeDmSurface.ve.dm.Surface.change (http://localhost/wiki/extensions/VisualEditor/lib/ve/src/dm/ve.dm.Surface.js?7b00b:841:7)
    at VeDmSurfaceFragment.ve.dm.SurfaceFragment.change (http://localhost/wiki/extensions/VisualEditor/lib/ve/src/dm/ve.dm.SurfaceFragment.js?2f9b0:128:15)
    at VeDmSurfaceFragment.ve.dm.SurfaceFragment.insertDocument (http://localhost/wiki/extensions/VisualEditor/lib/ve/src/dm/ve.dm.SurfaceFragment.js?2f9b0:903:8)

To reproduce, follow the steps as given in T190917: CX2: Fix issues in paragraph alignment and observe the dev console. While translating sections by clicking on place holder, in certain sections, you will see that instead of translation, it collapse and console shows the above error. As far as I can see this is not related to the content because if I retry the same translation the problematic section would not have any error, and some other random section will throw this error? Mostly observed while clicking on the sections one after another. Some internal race condition is suspected.

Details

Related Gerrit Patches:
mediawiki/extensions/ContentTranslation : masterWork around name collisions/duplication of references
VisualEditor/VisualEditor : masterFix error in ve.dm.InternalList#sortGroupIndexes

Event Timeline

Restricted Application added a project: VisualEditor. · View Herald TranscriptMar 29 2018, 11:22 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

It seems this issue is popping up in other contexts too. For example T196136. @dchan Do you think this is something to be fixed in CX or checked in VE library?

I did some debugging. In ve.dm.InternalList.prototype.sortGroupIndexes , the group.firstNodes array does not have non-null value for all indexes.

ve.dm.InternalList.prototype.sortGroupIndexes = function ( group ) {
	// Sort indexOrder
	group.indexOrder.sort( function ( index1, index2 ) {
		return group.firstNodes[ index1 ].getRange().start - group.firstNodes[ index2 ].getRange().start;
	} );
};

When the above issue happens, for an example, for index 1 value is null and hence the js error. Apparently this issue is not reproducible if I do step through the code using debugger(hint: execution pause), but reproducible when executed without pause. When I debug, I see all the 5 items in the array as non-null values.

I don't know enough about the logic of this class to understand why this happens. @dchan, @Esanders help?

Deskana triaged this task as Medium priority.Jun 26 2018, 2:01 PM
Deskana edited projects, added VisualEditor (Current work); removed VisualEditor.

We haven't seen this in VE, so this likely something to do with adding references through translation.

Change 443591 had a related patch set uploaded (by Santhosh; owner: Santhosh):
[VisualEditor/VisualEditor@master] Fix error in ve.dm.InternalList#sortGroupIndexes

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

I found that the issue can be consistently reproduced in case a reference is reused with a key multiple times across paragraphs and the reference content is not at the first use of reference.

Translating Atazanavir from English to Catalan is one example. It is illustrated with a video in T196136.

The ve.dm.MWReferenceNode.static.toDomElements method does sophisticated fix up on the order of reference with content and reference with keys. I believe some of these movements in the data model causes a shorter group.firstNodes array compared to group.indexOrder in the ve.dm.InternalList.prototype.sortGroupIndexes method. I know very little about this class, so I might be wrong too.

Anyway, by just adding a simple check before accessing the range of group.firstNodes[ index ], the javascript error can be avoided. From quick testing I don't see any side effect for this, but please point out if any. That is https://gerrit.wikimedia.org/r/#/c/VisualEditor/VisualEditor/+/443591

An assumption that is different from usual VE surface and CX related to references is, the content is added section by section. I see that ve.dm.MWReferenceNode.static.toDomElements may end up adding a reference with content definition repeated in multiple sections(but not repeated in single section). This may also influenced by which section is added first to the translation. We will test this further and see the issues if any.

Note that CX does not have any customization for ve.dm.MWReferenceNode or related classes so far.

We haven't seen this in VE, so this likely something to do with adding references through translation.

Santhosh has been working on a fix for the issue with a patchset on Visual Editor libraries (mentioned in T191011#4393608). That may be helpful to better track the origin of the issue. It would be helpful if you could take a look to those patchsets in case there are implications for Visual Editor. Thanks.

I've seen this error before when encountering when references are inserted in buggy ways. I'll investigate this in depth later (Wednesday at the latest), but for now I'll just ask: are you doing anything unusual in CX with multiple documents, or sharing the same InternalList object between documents, or anything like that?

As Pau says as T196136#4275129 , you can reproduce this in production by translating 1936 Cansiglio earthquake from English to Italian in production (using version=2) and converting the first ~5 paragraphs until you encounter a reference that's reused from a previous paragraph.

I managed to pause the execution right before it would otherwise have crashed, and group.firstNodes only has keys 0, 1 and 4 while group.indexOrder = [ 0, 1, 2, 3, 4 ]:

Going up the call stack, I see that this code is inserting an entire paragraph that contains references using insertDocument(), and that function is not designed to be able to deal with this scenario. What I believe is causing this problem is that we end up inserting a reference that has the same name as an existing one (this is normal) but points to a different index in the internalList (not normal). This looks/behaves like a naming collision between two different references (except that in this case the two internalList items are identical), and InternalList isn't able to deal with that.

We'll need to fix the reference management in VE (which is kind of fragile anyway) to deal with this better. The only reason this doesn't explode in regular VE all the time is that we don't let users pick reference names, so names always come from a trusted source (Parsoid) and that prevents name collisions from happening. Alternatively, we could add a flag to insertDocument() / newFromDocumentInsertion() instructing it not duplicate internal list items, i.e. behave as if .origInternalListLength is set, or actually set that property, or figure out a way to make one document a clone of the other so that that property is set.

I've -1ed the patch in the meantime, and I'll work on one of the possibilities I described on Wednesday.

Change 443591 abandoned by Santhosh:
Fix error in ve.dm.InternalList#sortGroupIndexes

Reason:
Based on the discussion T191011. Roan is going to work on the VE reference handling to fix this.

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

I've -1ed the patch in the meantime, and I'll work on one of the possibilities I described on Wednesday.

Thanks for looking into this and the detailed diagnosis, @Catrope.
A translator who has reported a couple of errors related to this was asking for an estimated time of resolution in this comment. If anyone has an estimate, feel free to comment.

Arrbee added a subscriber: Arrbee.Jul 18 2018, 7:52 AM

@Catrope Hello, did you manage to take another look at this? Looks like we added a one more blocked ticket to the list around this issue. Thanks.

Change 446767 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/extensions/ContentTranslation@master] [WIP] Work around name collisions/duplication of references

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

Change 446767 merged by jenkins-bot:
[mediawiki/extensions/ContentTranslation@master] Work around name collisions/duplication of references

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

Deskana closed this task as Resolved.Jul 30 2018, 3:20 PM
Deskana claimed this task.