Bug 444728 - NPE on DDiagram deletion when one of the palette tools listens it.
Summary: NPE on DDiagram deletion when one of the palette tools listens it.
Status: CLOSED FIXED
Alias: None
Product: Sirius
Classification: Modeling
Component: Diagram (show other bugs)
Version: 1.0.0   Edit
Hardware: PC Windows NT
: P3 normal (vote)
Target Milestone: 2.0.0   Edit
Assignee: Maxime Porhel CLA
QA Contact:
URL:
Whiteboard:
Keywords: triaged
Depends on:
Blocks:
 
Reported: 2014-09-22 08:20 EDT by Maxime Porhel CLA
Modified: 2014-10-27 06:52 EDT (History)
3 users (show)

See Also:


Attachments
Reproduction case (1.78 KB, application/x-zip-compressed)
2014-09-22 08:20 EDT, Maxime Porhel CLA
no flags Details
Reproduction case (2.35 KB, application/zip)
2014-09-22 09:54 EDT, Laurent Redor CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Maxime Porhel CLA 2014-09-22 08:20:52 EDT
Created attachment 247273 [details]
Reproduction case

Step to reproduce: 
 . Unzip the attached use case
 . Open the session
 . Open or create a diagram on the package
 . Add/Remove classes: the palette tool disappears from the palette when there are more than 5 elements displayed (the tool has a ToolFilter with a feature change listener on DDiagram.ownedDiagramElements)
 . Delete the diagram from the model explorer
 . NPE: no domain found from the gmf diagram in post commit (it has been deleted and so detached from its previous container)
Comment 1 Maxime Porhel CLA 2014-09-22 08:23:29 EDT
The error stack:
 java.lang.NullPointerException
                at 
 org.eclipse.sirius.diagram.ui.tools.internal.palette.PaletteToolFilterDescriptionListenersManager.addListenersForFilters(PaletteToolFilterDescriptionListenersManager.java:69)
                at org.eclipse.sirius.diagram.ui.tools.internal.palette.PaletteManagerImpl.updateFilters(PaletteManagerImpl.java:687)
                at org.eclipse.sirius.diagram.ui.tools.internal.palette.PaletteManagerImpl.updateFilters(PaletteManagerImpl.java:689)
                at org.eclipse.sirius.diagram.ui.tools.internal.palette.PaletteManagerImpl.updatePaletteForDiagramWithLayer(PaletteManagerImpl.java:323)
                at org.eclipse.sirius.diagram.ui.tools.internal.palette.PaletteManagerImpl.updatePalette(PaletteManagerImpl.java:272)
                at org.eclipse.sirius.diagram.ui.tools.internal.palette.PaletteManagerImpl.updatePalette(PaletteManagerImpl.java:251)
                at org.eclipse.sirius.diagram.ui.tools.internal.palette.PaletteManagerImpl.updatePalette(PaletteManagerImpl.java:211)
                at org.eclipse.sirius.diagram.ui.tools.internal.palette.PaletteManagerImpl.update(PaletteManagerImpl.java:173)
                at org.eclipse.sirius.diagram.ui.tools.internal.palette.PaletteManagerImpl.update(PaletteManagerImpl.java:161)
                at org.eclipse.sirius.diagram.ui.tools.internal.palette.PaletteToolFilterDescriptionListenersManager.run(PaletteToolFilterDescriptionListenersManager.java:98)
                at org.eclipse.sirius.business.api.tool.ToolFilterDescriptionListener.resourceSetChanged(ToolFilterDescriptionListener.java:84)
                at org.eclipse.emf.transaction.impl.TransactionalEditingDomainImpl$1.run(TransactionalEditingDomainImpl.java:781)
                at org.eclipse.emf.transaction.impl.TransactionalEditingDomainImpl.runExclusive(TransactionalEditingDomainImpl.java:328)
                at org.eclipse.emf.transaction.impl.TransactionalEditingDomainImpl.postcommit(TransactionalEditingDomainImpl.java:771)
                at org.eclipse.emf.transaction.impl.TransactionalEditingDomainImpl.deactivate(TransactionalEditingDomainImpl.java:543)
                at org.eclipse.emf.transaction.impl.TransactionImpl.close(TransactionImpl.java:712)
                at org.eclipse.emf.transaction.impl.TransactionImpl.commit(TransactionImpl.java:474)
                at org.eclipse.emf.workspace.AbstractEMFOperation.execute(AbstractEMFOperation.java:155)
...
Comment 2 Maxime Porhel CLA 2014-09-22 08:36:16 EDT
A tool has a Filter with a FeatureChangeListener on DDiagram.getOwnedElements.
So a refresh of the Palette is triggered in post commit.

But the current diagram has been detached, we cannot find its corresponding domain any more. The non null domain and listeners checks should be added in 
org.eclipse.sirius.diagram.ui.tools.internal.palette.PaletteToolFilterDescriptionListenersManager.addListenersForFilters(Collection<ToolFilterDescription>) (as it is done in the remove method).

The regression has been introduced by commit 9f2c003093bca93f402fdf65ecd0d4858fd3907d which add the DanglingRefRemovalTrigger as pre-commit listener (Bug 425561). This pre-commit listener detects all detached EObjects (each explicitely detached EObjects and all its contents without the elements moved to a non detached new container) and calls on them ModelAccessor.eDelete with the current session cross referencer as parameter. So every known references are unsetted. 

Before commit 9f2c003093bca93f402fdf65ecd0d4858fd3907d, the delete representation action/command just detached the representation to delete (and removed all references to it) but now the DanglingRefRemovalTrigger also correctly clean the representation elements.
Comment 3 Maxime Porhel CLA 2014-09-22 09:07:44 EDT
Proposed correction: https://git.eclipse.org/r/33683
Comment 4 Laurent Redor CLA 2014-09-22 09:54:50 EDT
Created attachment 247277 [details]
Reproduction case
Comment 5 Maxime Porhel CLA 2014-09-22 10:19:18 EDT
Corrected by commit dd55653f01285a8a9688d87bc5d236d63a3a6dec
Comment 6 Cedric Brun CLA 2014-10-01 08:33:50 EDT
While setting up the sirius tests on the eclipse infrastructure, we noticed a few failures in the log like this :

!MESSAGE Impossible to find an interpreter - Could not find a session for model element : org.eclipse.sirius.diagram.business.internal.metamodel.spec.DSemanticDiagramSpec@54005601 (documentation: ) (name: ) (synchronized: true, isInLayoutingMode: false, headerHeight: 1)
!STACK 0
java.lang.RuntimeException
	at org.eclipse.sirius.tools.api.interpreter.InterpreterRegistry.getInterpreter(InterpreterRegistry.java:70)
	at org.eclipse.sirius.tools.api.interpreter.InterpreterUtil.getInterpreter(InterpreterUtil.java:39)
	at org.eclipse.sirius.business.api.tool.ToolFilterDescriptionListener.elementsToListen(ToolFilterDescriptionListener.java:126)
	at org.eclipse.sirius.business.api.tool.ToolFilterDescriptionListener.getFilter(ToolFilterDescriptionListener.java:117)
	at org.eclipse.sirius.business.api.tool.ToolFilterDescriptionListener.<init>(ToolFilterDescriptionListener.java:59)
	at org.eclipse.sirius.tests.unit.api.tools.ToolFilterDescriptionListenerTests.testAddRemoveListener(ToolFilterDescriptionListenerTests.java:158)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

It looks very similar to the pattern described here.

https://git.eclipse.org/r/#/c/34196/ is a tentative change to "capture" the IInterpreter instance and not try to find it back from the model instances.
Comment 7 Mickael LANOE CLA 2014-10-06 07:58:52 EDT
Add automated test: https://git.eclipse.org/r/#/c/34451
Comment 8 Maxime Porhel CLA 2014-10-16 11:59:51 EDT
Additional correction from Cedric has been merged in commits:
 . 80b8e630c298fd4d8aa4cdeb4a4edf255705f1bb
 . 2bdf2a535360cc533b8ce398944eb4babefd7b59
Comment 9 Pierre-Charles David CLA 2014-10-27 06:52:11 EDT
Available in Sirius 2.0.0.