- Notifications
You must be signed in to change notification settings - Fork6k
[fuchsia] Pass WeakPtrs to GfxConnection FIDL fns.#28951
[fuchsia] Pass WeakPtrs to GfxConnection FIDL fns.#28951fluttergithubbot merged 3 commits intoflutter:masterfrom
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel inChat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read theTree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| FireCallbackMaybe(); | ||
| // | ||
| // Inspect updates must run on the inspect dispatcher. We don't pass | ||
| // |weak| to the callback because there is no guarantee that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure if this statement that "there is no guarantee that callbacks [invoked by inspect_dispatcher_] will run on the same thread" is true or not. However, because I'm not certain, it feels safer to avoid passing the WeakPtr into here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Upon closer inspection of the code, that statement is not true n this case. But it's all super implicit (sorry about that) so it requires scanning the entire class to know that....
- inspect_dispatcher_ == async_get_default_dispatcher() from the thread the GfxConnection ctor ran on (this is in the constructor).
- session_wrapper's dispatcher also == async_get_default_dispatcher() from the thread the GfxConnection ctor ran on (the fidl InterfaceHandle was bound to this thread in the constructor).
- Thus, the body of the session_wrapper_.set_on_frame_presented_handler callback runs on the same dispatcher as inspect_dispatcher_. All of this code is running on the same thread. It's safe to just use the weak_ptr everywhere (and that is by design, we never intend the callback and inspect to operate on multiple threads).
Again, super non-obvious without a deep understanding of this class. Suggestions on how to make that more clear very welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Further explanation: the only part of this class that runs on another thread is the "AwaitVsync" method. The flutter engine calls back into that method on the "UI" thread. All of the other operations in this class are taking place on the "Raster" thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No worries and thanks for the detailed explanation - super useful to know what threads different parts of this code are running on. I had a hard time finding the header for async_get_default_dispatcher, but now that I've found it, it's super clear. Changed the code to avoid the nastythiz = this nonsense.
chinmaygarde commentedSep 30, 2021
Just a heads up, fml::WeakPtrs can only be "used" (copying around is fine) on the threads on which they were were created. If is hard for me to tell if this condition is being violated here. Probably not, but just a heads up. |
akbiggs commentedSep 30, 2021
@chinmaygarde Thanks for the heads-up. I had read through the documentation and looked through the callbacks to verify that they are all being run on the same thread in this case. |
All (the pull request submitter and all commit authors) CLAs are signed,but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️Googlers:Go here for more info. |
akbiggs commentedOct 1, 2021
Ugh I blew up my pull request again. One sec, not going to submit it. |
Uh oh!
There was an error while loading.Please reload this page.
Bug: fxb/85211
Tested: Ran Spinning Square with flutter_jit_runner.
Crash occurs in
GfxSessionConnection::FireCallbackMaybeon session restart. This implies that the crash might be happening as a result of state that has been destroyed when the callback fires. We pass a WeakPtr to the current GfxSessionConnection into FIDL callbacks to check that we haven't already been destroyed when the FIDL callback fires.I haven't been able to reproduce the crash locally so I'm not 100% sure if this fixes the issue or not, but it seems like it will make the code safer.