Adding and removing the same view twice causes application to crash

[oltionchampari]

Environment

• Release number or git hash: develop
• Browser: chrome
• Deployed / Local: deployed & local

Steps to reproduce the bug

1. Open a dataset
2. Jump to the next view
3. Select a row
4. Close view and open it again
5. Select a row again
6. Replace the currently open view

Observed Behavior

The application crashes and the views are unresponsive

Expected Behavior

User should be able to replace view

Screenshot

[thinkh]

I can reproduce this error in my tests. I could even shorten it by omitting the selection in the copy number ranking.

The interesting fact is that only replacing the view (i.e., without add + remove before) works, but with adding and removing it breaks. My assumption is that the ref of the ViewWrapper is created and looked up by a generated static hash. Hence, the object ref is created when adding the first view and gets removed. Then, with the second opening CLUE tries to reuse the previous object ref. However, that ref is not available anymore.

ordino/src/internal/ViewWrapper.ts

Line 120 in a665c47

this.ref = ObjectRefUtils.objectRef(this, plugin.desc.name, ObjectRefUtils.category.visual, generateHash(plugin.desc, selection));

I investigate this hypothesis and see if this is the cause.

[thinkh]

I extended the generateHash() function by a random hash, to see whether or not the object ref is reused.

function generateHash(desc: IPluginDesc, selection: ISelection) {
  // const s = `${selection.idtype ? selection.idtype.id : ''}r${selection.ids}`;
  const s = `${selection.idtype ? selection.idtype.id : ''}r${selection.ids}_${BaseUtils.randomId()}`;
  return `${desc.id}_${s}`;
}

It turns out that with the random hash replacing the view after add + remove is working successfully! So my initial assumption seems to be confirmed.

Diffing the provenance graphs from before and after the change reveals that the object reference is not reused and the inverse action was added to the graph (which was previously not possible, due to the error within the replace action).

Screenshots

Added random id

Successfully added inverse action

Reused object ref with id 14 and new object ref with id 21; below the inverse edges

Exported provenance graphs

• clue_replace-view-only.json.txt -> works
• clue_replace-view-only+random-ref-id.json.txt -> works
• clue_add-remove-replace-fail.json.txt -> fails
• clue_add-remove-replace+random-ref-id.json.txt -> works

Follow-up questions

1. What (side) effects would the random id in the hash have? Does this change work with existing provenance graphs?
2. Why is the object ref added in both cases with id 21 (see last diff screenshot), but not used in the case without the random id?
3. Can we turn off / change the find object ref in CLUE?

[thinkh]

2. Why is the object ref added in both cases with id 21 (see last diff screenshot), but not used in the case without the random id?

The lookup (and reusage) is done in ProvenanceGraph.findInArray() and executed in initAction().

Without the random id the same object with id 14 is used from the _objects array.

3. Can we turn off / change the find object ref in CLUE?

Most likely not. Hence, adding a random id is probably the only solution and we must check the effects as mentioned in question 1.

[thinkh]

I check further how and when this bug could have been introduced and I couldn't find any recent change that could cause this error. The existingView variable in the replaceViewImpl is used and also the generate_hash() function hasn't changed (beside using template strings) since the beginning.

Interestingly, the ViewWrapper in tdp_core uses the default hash with ${name}_${category} and creates this.ref with graph.findOrAddObject().

I applied the graph.findOrAddObject() and the generate_hash() without the random id (so original state) to the Ordino ViewWrapper and this also seems to work!

-    this.ref = ObjectRefUtils.objectRef(this, plugin.desc.name, ObjectRefUtils.category.visual, generateHash(plugin.desc, selection));
+    this.ref = graph.findOrAddObject(ObjectRefUtils.objectRef(this, plugin.desc.name, ObjectRefUtils.category.visual, generateHash(plugin.desc, selection)));

Screenshots

The interesting point here is that no additional object ref 21 is created and the object ref 14 is reused successfully.

All following node ids are decreased by one, due to the missing object ref 21. The screenshot also shows that the replace action was successful by adding the inverse action.

Exported provenance graphs

• clue_add-remove-replace-fail.json.txt (same as in the comment above)
• clue_ref-with-findOrAddObject.json.txt -> works

[thinkh]

I've tested the compatibility with existing provenance graphs, by exporting a session from Ordino Daily (without the ref change) and imported it locally (with the ref change) again. Additionally I've tested the Ordino Paper Teaser Figure. In my tests all actions like add, remove, replace views work without problems. Also jumping to a specific state and the inverse actions work for me.

I also imported the previously broken clue_add-remove-replace-fail.json.txt session and now Ordino continues and finishes the action to replace the view. The graph got extended by the previously missing inverse action. Users could continue that session now.