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?

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.

In T191011#4327310, @Esanders wrote:

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

In T191011#4410032, @Catrope wrote:

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.

@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