Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NRE when tooltip closing(#381) #2167

Open
wants to merge 1 commit into
base: master
from

Conversation

@YoshihiroIto
Copy link
Contributor

YoshihiroIto commented Nov 8, 2019

Reproduction
#381 (comment)

@msftbot msftbot bot requested review from vatsan-madhavan and rladuca Nov 8, 2019
@msftbot msftbot bot added the PR label Nov 8, 2019
@msftbot msftbot bot requested a review from SamBent Nov 8, 2019
@weltkante

This comment has been minimized.

Copy link

weltkante commented Nov 8, 2019

I'm not sure this is the right fix, this might just be supressing the symptoms and leave the bug unfixed.

RaiseToolTipOpeningEvent seems to be the only place where _currentToolTip is set to null, and it does already have a if (_currentToolTip != null) guard further up the method. So the only reasonable explanation is that you are triggering reentrancy in RaiseToolTipOpeningEvent and execute it twice as a side effect of something done in that method. Maybe _currentToolTip.ClearValue(OwnerProperty); can trigger reentrancy?

I'd like to get the problem getting looked at closer before suppressing it with a seemlingly redundant check. If the method is reentrant there might be more that needs to be done to make sure the logic executes correctly. For example you might want to make sure not to call element.RaiseEvent(new ToolTipEventArgs(false)); twice and that BindingOperations.ClearBinding(_currentToolTip, ToolTip.ContentProperty); correctly clears the content property and doesn't just no-op because its null.

In either case it definitely needs adding a comment (preferrably after debugging and understanding whats happening here), otherwise you are at risk that someone removes the seemingly redundant if (_currentToolTip != null) you are adding.

@YoshihiroIto

This comment has been minimized.

Copy link
Contributor Author

YoshihiroIto commented Nov 8, 2019

@weltkante

Thank you.

As you said, this method is re-entered during processing. I recognized this and created this pull request.
This is because the original code makes it clear.
Here.

Your suggestion will be a re-implementation of this method, so I want someone to do it in another pull request. I'm doing it if I can, but I'm not yet familiar with the WPF code.

This PR is intended to correct a small part of WPF that has been stable for a long time. There is only one revision.

I agree that comments are needed. I will add a comment.

As you can see, I am not good at English. I will give you a little time.

@weltkante

This comment has been minimized.

Copy link

weltkante commented Nov 8, 2019

The problem with your fix is that it still will raise the event twice, so while it may not crash anymore you haven't fixed the problem of RaiseToolTipOpeningEvent not being able to deal with reentrancy correctly. We can wait for someone from the WPF team to decide the direction to take with the fix, I'm just commenting.

Your suggestion will be a re-implementation of this method, so I want someone to do it in another pull request.

I don't think a reimplementation will be necessary, but I didn't debug it so I can't tell for sure. The problem with doing it in another PR is that if there is too much time between fixing the crash people may start accidentally depending on the broken event order and then it'll be too late to fix the event. The WPF project isn't exactly seeing a lot of action so I want to make sure this is fixed correctly while its impossible for people to depend on the reentrant event ordering.

That doesn't mean you have to do the other fix, I just want to bring it to the attention of the WPF team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.