Skip to content
This repository has been archived by the owner on May 11, 2022. It is now read-only.

FUSETOOLS2-689 - fixing webview disposed error #189

Merged

Conversation

bfitzpat
Copy link
Collaborator

lhein
lhein previously approved these changes Sep 14, 2020
Signed-off-by: bfitzpat@redhat.com <bfitzpat@redhat.com>
Signed-off-by: bfitzpat@redhat.com <bfitzpat@redhat.com>
@bfitzpat bfitzpat force-pushed the FUSETOOLS2-689-fixing-disposed-error branch from b13407f to 789dca5 Compare September 14, 2020 17:47
Signed-off-by: bfitzpat@redhat.com <bfitzpat@redhat.com>
@bfitzpat bfitzpat changed the title fixing webview disposed error FUSETOOLS2-689 - fixing webview disposed error Sep 14, 2020
Copy link
Member

@apupier apupier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why the dispose method and the wrapDidactContent are called although the Webview is disposed? Can it hide another issue?

@bfitzpat
Copy link
Collaborator Author

Can you explain why the dispose method and the wrapDidactContent are called although the Webview is disposed? Can it hide another issue?

I could move it up further, in _update, but I put the fix where the error was appearing. I have not seen any other issues in my testing.

@apupier
Copy link
Member

apupier commented Sep 14, 2020

_update

why _update is called although the webview is disposed? Shouldn't be dereferenced from somewhere when the dispose is called?

@bfitzpat
Copy link
Collaborator Author

Since there are NO other indicators of trouble, I am going with the simplest solution to solve the issue that was presented. We had messages that appeared in a test console indicating that the webview was disposed and those messages did not prevent tests from passing. I found a solution that was used elsewhere to handle a similar problem (microsoft/vscode@5a0ead5). End of story. I'm not going looking for trouble.

@apupier
Copy link
Member

apupier commented Sep 15, 2020

Just hiding the error log might hide a bigger problems. A problem with not-disposed elements might be a sign of a memory leak. It can also be less harmful like just a problem of wrong order of operations during disposal. i'm asking because i think that it is importan to know if we know what is the root cause. If it is a memory leak it can be important to at least create another issue to investigate as it won't be very visible anymore given that the log has been removed.

@lhein
Copy link
Collaborator

lhein commented Sep 15, 2020

One oddity I spotted right now is

this._panel.onDidDispose(() => this.dispose(), null, this._disposables);

If I understand well then this handler will call the dispose function whenever the 'vscode.WebviewPanel' is disposed. However in the dispose function we call there is another call to the 'vscode.WebviewPanel's dispose function.
See
this._panel.dispose();
.

So we basically try to dispose the same object 2 times which most likely causes the initial error and all the other handlings added by this PR are probably not even needed to fix the issue. Please correct me if I am wrong.

@lhein lhein dismissed their stale review September 15, 2020 08:59

spotted a problem

@bfitzpat
Copy link
Collaborator Author

Please correct me if I am wrong.

Though we can explicitly dispose the object ourselves, there are listeners in place already that also have to handle that responsibility. Think things like when the window closes. We do not explicitly listen for such events and rely on the onDispose handler to call it for us. We do call it explicitly when the user opens a new didact file -- disposing the old panel and creating a new one. We're not disposing it twice. The dispose function is called in two places, but it's not redundant.

@bfitzpat
Copy link
Collaborator Author

Spinning off the separate issue to handle it - https://issues.redhat.com/browse/FUSETOOLS2-698

@bfitzpat bfitzpat merged commit 5abfc21 into redhat-developer:master Sep 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants