Fix another outfit-related CTD#347
Open
Quinn-Elara wants to merge 4 commits into
Open
Conversation
Defers notifying observers to run on the main thread. Fixes a CTD wherein glyph creation could happen outside of the main thread which would cause a CTD if uncaught.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesInventory notification control
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@indra/newview/llaisapi.cpp`:
- Around line 1748-1757: Throttle the deferred inventory notifications so they
are posted once per batch instead of once for every overflowed item. In the
inventory fetch/update path around gInventory.notifyObservers() and the
category-create path, add a guard or pending flag so repeated iterations while
mChangedItemIDs remains above MAX_UPDATE_BACKLOG do not queue duplicate
postToMainCoro callbacks. Make sure the same batching logic is applied
consistently in both places that currently defer notifyObservers().
- Around line 1683-1692: The backlog refresh logic in the inventory checks is
repeatedly posting notifyObservers() because deferred execution leaves
gInventory.getChangedIDs().size() above MAX_UPDATE_BACKLOG until the main loop
runs. Add a single pending-refresh/coalescing flag around the backlog paths in
llAisApi so only one deferred inventory refresh can be queued at a time, and
clear that flag when the posted callback runs and notifyObservers() completes.
Apply the same guard to both backlog checks in llAisApi to preserve throttling
and prevent gMainloopWork from being flooded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5873a2ea-9cbc-418c-a777-28e71e0a6d75
📒 Files selected for processing (1)
indra/newview/llaisapi.cpp
The early notifyObservers() call before tree cleanup was unconditional, ignoring the do_notify_observers parameter. This caused observers to fire from non-main coroutines (e.g. the AIS REMOVEITEM coro), which triggered UI updates and GL texture operations that assert they must run on the main coroutine. Deferred notification via idleNotifyObservers() handles the AIS path correctly on the next main loop tick.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
indra/newview/llaisapi.cpp (2)
1693-1702: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the deferred-notification block into a helper to avoid duplication.
Both backlog paths contain the identical 8-line coalescing block. A small static helper would keep them in sync and reduce maintenance risk if the notification logic changes.
♻️ Proposed helper extraction
+static void postDeferredNotifyObservers() +{ + if (!sAISNotifyObserversPending) + { + sAISNotifyObserversPending = true; + LLAppViewer::instance()->postToMainCoro( + []() + { + gInventory.notifyObservers(); + sAISNotifyObserversPending = false; + }); + } +} + // ... then at both backlog sites: - if (!sAISNotifyObserversPending) - { - sAISNotifyObserversPending = true; - LLAppViewer::instance()->postToMainCoro( - []() - { - gInventory.notifyObservers(); - sAISNotifyObserversPending = false; - }); - } + postDeferredNotifyObservers();Also applies to: 1763-1772
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/llaisapi.cpp` around lines 1693 - 1702, Extract the duplicated sAISNotifyObserversPending coalescing logic into a shared static helper near the existing AIS notification code, including posting the main-coroutine callback and resetting the flag after gInventory.notifyObservers(). Replace both backlog-path blocks around the relevant notification handling with calls to this helper, preserving the current behavior.
1843-1848: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueGuard the final deferred notification with the coalescing flag.
When the backlog path already posted a deferred
notifyObservers()(flag is stilltrue), this unconditional post queues a secondnotifyObservers()that will run after the backlog lambda. The second call iterates all observers but processes no changes (IDs already cleared). Checking!sAISNotifyObserversPendingavoids the redundant post — the pending backlog lambda will process all accumulated changes sincedoUpdate()doesn't yield between the backlog post and this final post.♻️ Proposed fix
- LLAppViewer::instance()->postToMainCoro( - []() - { - gInventory.notifyObservers(); - }); + if (!sAISNotifyObserversPending) + { + LLAppViewer::instance()->postToMainCoro( + []() + { + gInventory.notifyObservers(); + }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/llaisapi.cpp` around lines 1843 - 1848, Guard the final deferred notification in the surrounding AIS update flow with !sAISNotifyObserversPending before calling LLAppViewer::instance()->postToMainCoro. Keep the existing notifyObservers callback unchanged, and avoid posting it when the backlog path has already scheduled the pending notification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@indra/newview/llaisapi.cpp`:
- Around line 1693-1702: Extract the duplicated sAISNotifyObserversPending
coalescing logic into a shared static helper near the existing AIS notification
code, including posting the main-coroutine callback and resetting the flag after
gInventory.notifyObservers(). Replace both backlog-path blocks around the
relevant notification handling with calls to this helper, preserving the current
behavior.
- Around line 1843-1848: Guard the final deferred notification in the
surrounding AIS update flow with !sAISNotifyObserversPending before calling
LLAppViewer::instance()->postToMainCoro. Keep the existing notifyObservers
callback unchanged, and avoid posting it when the backlog path has already
scheduled the pending notification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eb6fd5b0-2819-445c-be3c-8636fe17e72c
📒 Files selected for processing (2)
indra/newview/llaisapi.cppindra/newview/llinventorymodel.cpp
Extract the duplicated sAISNotifyObserversPending coalescing logic from the category and item creation backlog paths into a shared static helper aisScheduleNotifyObservers(). Guard the final postToMainCoro at the end of AISUpdate::doUpdate() with !sAISNotifyObserversPending to avoid posting a redundant notifyObservers() when the backlog path has already scheduled one. No behaviour change. Addresses review nitpicks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Defers notifying observers to run on the main thread. This fixes a CTD wherein glyph creation could happen outside of the main thread which would cause a CTD if uncaught.
Related Issues
Issue Link:
Checklist
Please ensure the following before requesting review:
Additional Notes