fix(view): isLoaded stability improvements - #6187
NathanWalker wants to merge 1 commit into
Conversation
Sorry, something went wrong.
|
test |
Sorry, something went wrong.
| } | ||
|
|
||
| if (frame.isLoaded && !page.isLoaded) { | ||
| if ((frame && frame.isLoaded) && (page && !page.isLoaded)) { |
There was a problem hiding this comment.
page is used couple of lines above - so no point of guarding it here. If it was undefined the code would have crashed here
Sorry, something went wrong.
This comment was marked as abuse.
There was a problem hiding this comment.
@vakrilov @NathanaelA I updated the handling to ensure of number of possible errors that could occur due to timing and null/undefined - also added trace handling to ensure the log is notified in event of such conditions.
Sorry, something went wrong.
| */ | ||
| public onChange(): void { | ||
| if (this.view.isLoaded) { | ||
| if (this.view && this.view.isLoaded) { |
There was a problem hiding this comment.
What are the cases when you get error here?
The view is passed in the constructor and never changed.
Maybe we should add a check there?
Sorry, something went wrong.
This comment was marked as abuse.
There was a problem hiding this comment.
That’s exactly right @NathanaelA
Sorry, something went wrong.
There was a problem hiding this comment.
I'm still struggling to get this one. The view is private and is never set (except from the constructor).
If there is a problem with trying to apply styles on disposed view - this check will do nothing more. The view will be disposed, but its CssState will still hold the reference and so this.view will not be null.
Sorry, something went wrong.
|
Worth noting I am using these changes in a custom pack of core modules in a current project to solve the issues reported and is working 100% without error with these in place. |
Sorry, something went wrong.
|
We should consider merging this in the release branch. |
Sorry, something went wrong.
|
Hey @NathanWalker and @NathanaelA. I will cherry pick the commit and open a PR in release, so that we can include it in a patch release. There are still one change that don't really make sense to me. The one in style-scope.t. I'm trying to understand if you have crash reports related to them - this might require a more thorough investigation and maybe additional fix somewhere else. The stack trace from #6179 points only to the loadView method in the view-base.ts. |
Sorry, something went wrong.
|
Closing in favor of #6255 |
Sorry, something went wrong.
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Sorry, something went wrong.
PR Checklist
What is the current behavior?
App's can randomly crash when various bindings update upon resuming an app dependent upon a wide array of possible view setups via developers.
What is the new behavior?
Guards have been added to account for various troubling race conditions.
closes #6179