-
Notifications
You must be signed in to change notification settings - Fork 275
Conversation
- Wrap UIGadgetInfo.CreateUIGadgetInfo with try/catch and log packageId on exception - Add try/catch in UIGadgetManager static constructor to prevent TypeInitializationException - Add try/catch in foreach loop to continue loading other packages on failure - Add try/catch in UIGadgetManager.Refresh() with packageId in error logs - Add try/catch in NUIGadgetManager static constructor, LoadGadgetInfos, and Refresh Signed-off-by: Changgyu Choi <changyu.choi@samsung.com>
- Use 'as' cast instead of direct cast for CoreApplication.Current with null check - Remove redundant outer try/catch in foreach loops (CreateUIGadgetInfo already handles exceptions internally) - Move PackageInfoDestroy to finally block for proper native handle cleanup - Remove redundant null check after 'new NUIGadgetInfo' operator Co-Authored-By: Cline SR Signed-off-by: Changgyu Choi <changyu.choi@samsung.com>
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.
Code Review
This pull request enhances the robustness of the UIGadget and NUIGadget managers by implementing comprehensive error handling and improved logging. Key updates include wrapping static constructors and resource-intensive methods in try-catch blocks, ensuring native package handles are consistently released in UIGadgetInfo via finally blocks, and providing more context in error logs. Feedback suggests further improving reliability by broadening exception filters in UIGadgetManager to prevent TypeInitializationException and adding explicit logging for null application instances to aid in troubleshooting.
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.
The exception filter here is quite restrictive. To ensure the robustness intended by this pull request and prevent TypeInitializationException, it is safer to catch all exceptions during gadget info registration, consistent with the implementation in NUIGadgetManager.cs. Additionally, logging the full exception object e instead of just e.Message provides the stack trace, which is essential for debugging.
catch (Exception e) { Log.Error("Exception occurs while adding gadget info. packageId: " + pkg + ", " + e); }
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.
If CoreApplication.Current is null or cannot be cast to CoreApplication, the manager will silently skip registering critical system event handlers (AppControl, LowMemory, etc.). This could lead to difficult-to-debug issues where the application fails to respond to system events. It is recommended to log an error if app is null to aid in troubleshooting.
var app = CoreApplication.Current as CoreApplication; if (app != null) { app.AppControlReceived += (s, e) => HandleAppControl(e); app.LowMemory += (s, e) => HandleLowMemoryEvent(e); app.LowBattery += (s, e) => HandleLowBatteryEvent(e); app.LocaleChanged += (s, e) => HandleLocaleChangedEvent(e); app.RegionFormatChanged += (s, e) => HandleRegionFormatChangedEvent(e); app.DeviceOrientationChanged += (s, e) => HandleDeviceOrientationChangedEvent(e); } else { Log.Error("Failed to get CoreApplication instance. Event handlers will not be registered."); }
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.
For consistency and improved robustness, consider catching all exceptions here instead of using a specific filter. Logging the full exception object e is also preferred as it includes the stack trace for better diagnostics.
catch (Exception e) { Log.Error("Exception occurs while adding gadget info. packageId: " + pkg + ", " + e); }
JoonghyunCho
commented
May 20, 2026
🤖 [AI Review]
Reviewed — no findings.
Scope checked:
UIGadgetInfo.CreateUIGadgetInfo: try/catch/finally withIntPtr.Zeroguard aroundPackageInfoDestroycorrectly closes the handle-leak path; catch block returnsnullso the contract ("null means failed") is preserved for callers.UIGadgetManagerstatic constructor: hard cast(CoreApplication)CoreApplication.Currentreplaced byas-null-check pattern — this is the actual NRE/InvalidCastException fix called out in the PR title; lifecycle handlers are simply skipped whenCurrentis unavailable, which is the right call inside a static ctor where throwing would yieldTypeInitializationException.- Outer
catch (Exception)blocks added to:UIGadgetInfo.CreateUIGadgetInfo,UIGadgetManagerstatic ctor,NUIGadgetManagerstatic ctor,LoadGadgetInfos, and publicRefresh()— broad catches are defensible at the boundary of error-recovery code (per-package iteration, type init) and each logs a packageId or call-site context.Refresh()now swallows-and-logs instead of propagating; this is an intentional behavior change called out in the PR description (no explicit<exception>doc to break). - Inner inner
TryAddcatch inUIGadgetManagerretained its narrowerwhen (e is ArgumentNullException || e is OverflowException)filter — only the message was enriched withpackageId. Specific filter preserved where applicable; not over-widened. NUIGadgetManager.LoadGadgetInfosdrops the redundantif (gadgetInfo != null)check —new NUIGadgetInfo(info)either throws or returns non-null. Same cleanup as sibling PR Fix NullReferenceException in NUIGadgetManager #7653 .- Diff vs. Fix NullReferenceException in NUIGadgetManager #7653 : this API14 backport additionally wraps the static ctors and
Refresh()and broadens the per-item catch — consistent with API14 being a stabilization branch where "don't crash app init" outweighs "fail loudly".
No 🔴 critical issues, no 🟡 suggestions to flag.
Automated review — final merge decision rests with human reviewers.
Description of Change
API Changes