Page MenuHomePhabricator
Create Task
Maniphest T27061

Planar figures crash if drawn with empty data storage
Closed, ResolvedPublic
Actions

Description

How to reproduce:

  1. Start workbench
  2. Select an measurment option and start drawing with an empty storage

crash.

Reason:

  • mitk::PlanarFigureInteractor::MoveCurrentPoint() is retrieves a planargeometry which is a nullptr
  • then this->TransformPositionEventToPoint2D(positionEvent, planarFigureGeometry, point2D) is invoked with the nullptr and crashes.

What todo:

  • Introduce a nullptr check in PlanarFigureInteractor to avoid nullptr exception (see above) but throw meaning full exceptions it it is a violated precondition.

Revisions and Commits

rMITK MITK
Restricted Differential RevisionrMITK67d34b5de004 Introduce nullptr checks in PlanarFigureInteractor to avoid nullptr exception
Restricted Differential RevisionrMITK5b5d523a5c51 Add nullptr checks to prevent crashes
Restricted Differential RevisionrMITK780e8d84937f Clean class

Related Objects
Search...

Event Timeline

floca created this task.Feb 5 2020, 5:26 PM
floca added a parent task: T27049: Refactor Measurement plugin to use the new node selection widgets.Feb 5 2020, 5:49 PM
kislinsk triaged this task as Normal priority.Feb 7 2020, 9:03 AM
kislinsk added a subscriber: kislinsk.

While there should be a nullptr check I guess, this is another regression in the first place as it wasn't possible with the Measurement view to draw with an empty storage until recently. Is this possibly related to your work on selections?

kislinsk added a comment.Feb 7 2020, 9:05 AM

Here's a screenshot from the last release. All the tools are deactivated as long as there isn't any visible object.

Capture.PNG (120×435 px, 10 KB)

kalali added a subscriber: kalali.Feb 7 2020, 9:33 AM

I guess @floca mentioned this because it was introduced by refactoring the Measurement plugin. Still working on this.

floca added a comment.Feb 7 2020, 9:41 PM

Yup. And it is remarked as a regression in the review. I wrote this task while reviewing, before I chacked the baseline. Sorry for that.
Even if the regression test is fixed. I think a nullptr check and a reasonable error reaction or throw, wouldn't hurt ;)

floca added a comment.EditedFeb 16 2020, 10:30 PM

@kalali isn't this task obsolete after your rework of the rework measurement view?

floca added a comment.Feb 27 2020, 8:32 PM

@kalali Could you tell if this is obsolete now?

kalali added a comment.Feb 28 2020, 1:11 PM

As noted, this was never a real issue since the broken code was never merged.
As D238#5458 stated, measurement options are only enabled if an image is selected.

However another task should be opened to add a nullptr check somewhere in the classes mentioned in the description to prohibit planar figure measurements without an image. This is out of scope of the Measurement-plugin.
Additionally, in https://phabricator.mitk.org/source/mitk/browse/master/Plugins/org.mitk.gui.qt.measurementtoolbox/src/internal/QmitkMeasurementView.cpp;4b80587b8479b1eec289231b4a317fb1ff1d03ca$357 there was an option to work without an image. That is not possible anymore and I don't know what that meant so we should clarify this.

floca added a comment.Feb 28 2020, 1:18 PM

@kalali OK, could you please open task where needed or tag with RequestForDiscussion. Thanks

kalali added a project: Request for Discussion.Mar 3 2020, 11:55 AM
kislinsk added a comment.Apr 15 2020, 9:24 AM
In T27061#195564, @kalali wrote:

As noted, this was never a real issue since the broken code was never merged.
As D238#5458 stated, measurement options are only enabled if an image is selected.

However another task should be opened to add a nullptr check somewhere in the classes mentioned in the description to prohibit planar figure measurements without an image. This is out of scope of the Measurement-plugin.
Additionally, in https://phabricator.mitk.org/source/mitk/browse/master/Plugins/org.mitk.gui.qt.measurementtoolbox/src/internal/QmitkMeasurementView.cpp;4b80587b8479b1eec289231b4a317fb1ff1d03ca$357 there was an option to work without an image. That is not possible anymore and I don't know what that meant so we should clarify this.

Is that a yes or a no to Ralf's question? I do not understand if was fixed since I created this task or not but I experienced it in the master branch.

kalali added a comment.EditedApr 15 2020, 9:28 AM

People who experience this bug in master should state it here and leave the task open. I never experienced this bug and I did not open this task so please go ahead and see if the error can be reproduced with your systems.
As I wrote, the root of this error is still not fixed (missing check for nullptr) so I could add an issue for that - but that was not the scope of my refactoring, related to this issue.

kislinsk added a comment.Apr 15 2020, 9:44 AM

Just checked, seems to be fixed in the current master branch. Set this task's status to Resolved?

floca lowered the priority of this task from Normal to Low.EditedApr 15 2020, 10:55 AM
floca updated the task description. (Show Details)
floca edited projects, added Restricted Project; removed Request for Discussion.

It depends on the scope of the task. I have altered the description. I would reuse this task for the nullptr issue. But there is no accute release relevant issue anymore, like Amir said.

kalali added a revision: Restricted Differential Revision.Aug 11 2020, 4:51 PM
kalali added a commit: rMITK780e8d84937f: Clean class.Aug 12 2020, 8:18 AM
kalali added a commit: rMITK5b5d523a5c51: Add nullptr checks to prevent crashes.
kalali added a commit: rMITK67d34b5de004: Introduce nullptr checks in PlanarFigureInteractor to avoid nullptr exception.
kalali closed this task as Resolved.Aug 12 2020, 8:19 AM
kalali claimed this task.
kislinsk mentioned this in T27695: 2020 Week 36 (Very Early September).Sep 1 2020, 3:34 PM
MITK · MIC · IMSY · DKFZ