Page MenuHomePhabricator

Make linear model data immutable
Closed, ResolvedPublic8 Estimated Story Points

Description

We do a lot of defensive copying of linear model data, and have had difficult bugs in the past due to people modifying passed-by-reference objects. Using something like Facebook's immutable-js https://github.com/salier/immutable-js might protect us against some of these issues.

Cons: Would probably require a lot of additional code, replacing our neat plain object/array APIs with method calls. May also be a performance hit doing method calls instead of simple property accesses.

Thoughts?


Some different language for context
Changes can be made to the document without the software knowing these changes were made. This could lead to the software being out of sync with itself. Example: someone could make a change to the document without them being able to undo or redo that change because no record exists of that change being made.

Event Timeline

Esanders raised the priority of this task from to Needs Triage.
Esanders updated the task description. (Show Details)
Esanders added subscribers: Esanders, DLynch, dchan, Catrope.
Jdforrester-WMF set Security to None.
Jdforrester-WMF moved this task from To Triage to Freezer on the VisualEditor board.
Jdforrester-WMF edited a custom field.

Another possibility (not necessarily better; possibly less drastic) would be to have clones of ve.dm.ElementLinearData (or whatever) that use copy-on-write semantics. (So thing2 = thing.clone() is very fast, with thing2 essentially being an "empty" reference to thing until such time as a mutator is called on either thing or thing2).

Another option I've found is to use Object.freeze (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze)
which works on any object type (including arrays). This causes an exception to be thrown if you try to modify the object (but only if strict mode is enabled).

Change 463238 had a related patch set uploaded (by Esanders; owner: Esanders):
[VisualEditor/VisualEditor@master] Deep-freeze linear data

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

Using Object.freeze this is relatively simple, with most of the changes happening in ElementLinearData. I didn't even have to make any changes to ve-mw to keep unit tests working.

This is probably because we do most transacting via SurfaceFragment these days.

The next step would be to evaluate our uses of OO.copy/ve.copy and see where those can be dropped.

JTannerWMF moved this task from Freezer to Triaged on the VisualEditor board.
JTannerWMF subscribed.

It looks like you are workng on this @Esanders

Esanders renamed this task from Evalulate pros and cons of an immutable data type for linear model data to Make linear model data immutable.Oct 17 2020, 4:57 PM
Esanders moved this task from Doing to Code Review on the Editing-team (Kanban Board) board.

Change 463238 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Deep-freeze linear data

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

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

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

The above patches only enabled the feature in test and debug mode.

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

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

Change 658003 abandoned by Esanders:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (f8439f4cc)

Reason:
See Iae4362c8dab0f2bd335e24498f3e0522b8b1d4fc

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

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

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

Example: someone could make a change to the document without them being able to undo or redo that change because no record exists of that change being made.

Worse, in that scenario, attempting to undo/redo would break the document in unpredictable ways, because the transactions applied wouldn't account for the changed data.

The above patches only enabled the feature in test and debug mode.

T275675 is a follow-up to enforce this everywhere. This task can be closed (really it should be renamed to Make linear model data immutable-able).