Fix Network tab not capturing requests after hot restart and clear#9856
Fix Network tab not capturing requests after hot restart and clear#9856muhammadkamel wants to merge 4 commits into
Conversation
Re-enable HTTP logging and socket profiling when new isolates are spawned, reset clear timestamps using the VM timeline, and keep the search field enabled while recording.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request addresses issues in the Network profiler where capturing stopped after a hot restart or after clearing recorded data, and introduces an onIsolateCreated stream to detect new isolates. The review feedback highlights a regression where the search field is disabled when recording is paused (even if requests exist), and suggests wrapping asynchronous VM service calls in error-handling blocks to prevent unhandled RPCErrors.
| child: SearchField<NetworkController>( | ||
| searchController: controller, | ||
| searchFieldEnabled: hasRequests, | ||
| searchFieldEnabled: _recording, |
There was a problem hiding this comment.
[MUST-FIX] Disabling the search field when _recording is false will prevent users from searching through captured requests when recording is paused. To fix this regression while still keeping the search field enabled when the list is cleared during active recording, the search field should be enabled if recording is active OR if there are existing requests in the list.
| searchFieldEnabled: _recording, | |
| searchFieldEnabled: _recording || controller.filteredData.value.isNotEmpty, |
References
- Categorize Severity: Prefix every comment with a severity:
[MUST-FIX]for logical bugs. (link)
| _, | ||
| ) async { | ||
| if (_recordingNotifier.value) { | ||
| await _enableNetworkTrafficRecordingOnAllIsolates(); |
There was a problem hiding this comment.
[CONCERN] Wrap the call to _enableNetworkTrafficRecordingOnAllIsolates() in allowedError to safely handle any potential RPCErrors that might occur if the newly created isolate is quickly destroyed or if the connection is lost during the asynchronous operation.
| await _enableNetworkTrafficRecordingOnAllIsolates(); | |
| await allowedError(_enableNetworkTrafficRecordingOnAllIsolates()); |
References
- Categorize Severity: Prefix every comment with a severity:
[CONCERN]for robustness/maintainability issues. (link)
| Future<void> clearData() async { | ||
| await updateLastSocketDataRefreshTime(); | ||
| updateLastHttpDataRefreshTime(); | ||
| final service = serviceConnection.serviceManager.service; | ||
| if (service == null) return; | ||
|
|
||
| final timestamp = (await service.getVMTimelineMicros()).timestamp!; | ||
| networkController.lastSocketDataRefreshMicros = timestamp; | ||
| await service.forEachIsolate((isolate) async { | ||
| lastHttpDataRefreshTimePerIsolate[isolate.id!] = timestamp; | ||
| }); | ||
| await _clearSocketProfile(); | ||
| await _clearHttpProfile(); | ||
| } |
There was a problem hiding this comment.
[CONCERN] Wrap the VM service calls in clearData() in a try-catch block for RPCError to prevent unhandled exceptions (and potential UI crashes) if the connection is lost or the service is disposed while clearing data.
Future<void> clearData() async {
final service = serviceConnection.serviceManager.service;
if (service == null) return;
try {
final timestamp = (await service.getVMTimelineMicros()).timestamp!;
networkController.lastSocketDataRefreshMicros = timestamp;
await service.forEachIsolate((isolate) async {
lastHttpDataRefreshTimePerIsolate[isolate.id!] = timestamp;
});
await _clearSocketProfile();
await _clearHttpProfile();
} on RPCError catch (e) {
if (!e.isServiceDisposedError) {
rethrow;
}
}
}References
- Categorize Severity: Prefix every comment with a severity:
[CONCERN]for robustness/maintainability issues. (link)
19e96b7 to
043132a
Compare
4a1a518 to
a082ddb
Compare
Keep search enabled when paused with visible requests, handle RPC errors during clear, and wrap isolate profiling re-enable in allowedError.
74fc05b to
7d34604
Compare
Summary
IsolateManager.onIsolateCreated, fixing the Network tab freeze after hot restart (#9783, #9839).IsolateManager.onIsolateCreatedindevtools_app_sharedfor isolate lifecycle detection.Test plan
flutter test test/screens/network/network_hot_restart_test.dartflutter test test/screens/network/network_clear_test.dartflutter test test/screens/network/network_profiler_test.dart./tool/bin/dt presubmit --fix