fix: Fix crash issues.#112
Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom Sep 23, 2025
Merged
Conversation
- It crashed if try to download "magnet:?xt=xxx" which may be incorrect json info. - It crashed if drap bt file to download, that caused by loop handle. Log: Fix crash if magnet fink and drap file to download.
Contributor
deepin pr auto review代码审查意见总体评价这次代码修改主要涉及三个方面的改进：
详细分析1. createtaskwidget.h 和 createtaskwidget.cpp 的改进优点：
建议改进：
QFileInfo fileInfo(filePath);
if (!fileInfo.exists()) {
qDebug() << "[CreateTaskWidget] Error: File does not exist:" << filePath;
close();
return;
}
if (!dialog) {
qDebug() << "[CreateTaskWidget] Error: Failed to create BtInfoDialog";
close();
return;
}2. aria2rpcinterface.cpp 的改进优点：
建议改进：
if (code == 503 || code == 504) { // 服务不可用或网关超时
qDebug() << "[Aria2RPC] Retrying request due to temporary error:" << code;
// 实现重试逻辑
return;
}
if (parseError.error != QJsonParseError::NoError) {
// 尝试清理并重新解析
QByteArray cleanedBuf = buf.trimmed();
doc = QJsonDocument::fromJson(cleanedBuf, &parseError);
if (parseError.error != QJsonParseError::NoError) {
qDebug() << "[Aria2RPC] Error: JSON parse failed after cleanup";
return;
}
}3. tabledatacontrol.cpp 的改进优点：
建议改进：
bool ok;
long value = str.toLong(&ok);
if (!ok || value < 0) {
qDebug() << "Error: Invalid numeric value:" << str;
return false;
}
if (index < 0 || index >= array.size()) {
qDebug() << "Error: Array index out of bounds:" << index;
return false;
}安全性建议
性能建议
总结这次代码修改在解决回调循环问题和增强代码健壮性方面做得很好。建议在后续开发中：
这些改进将有助于提高代码的可靠性、安全性和性能。 |
Reviewer's GuideThis PR refactors CreateTaskWidget to break callback loops by deferring BT/Metalink file dialogs via a new signal-slot and QTimer-based async path, and adds defensive JSON parsing and validation in both TableDataControl and Aria2RPCInterface to avoid crashes on malformed or missing data. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- You have introduced many repetitive JSON safety checks in TableDataControl and Aria2RPCInterface; consider extracting common validation helpers to reduce boilerplate and improve maintainability.
- Using QTimer::singleShot to defer BT/Metalink dialog creation may introduce race conditions if CreateTaskWidget is destroyed before execution; ensure the widget’s lifetime is correctly managed or guard against invalid
thispointers. - For Aria2RPCInterface, instead of silently returning on JSON parse errors, consider propagating an error signal or status so the UI can provide feedback to the user.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You have introduced many repetitive JSON safety checks in TableDataControl and Aria2RPCInterface; consider extracting common validation helpers to reduce boilerplate and improve maintainability.
- Using QTimer::singleShot to defer BT/Metalink dialog creation may introduce race conditions if CreateTaskWidget is destroyed before execution; ensure the widget’s lifetime is correctly managed or guard against invalid `this` pointers.
- For Aria2RPCInterface, instead of silently returning on JSON parse errors, consider propagating an error signal or status so the UI can provide feedback to the user.
## Individual Comments
### Comment 1
<location> `src/src/ui/mainFrame/tabledatacontrol.cpp:286-293` </location>
<code_context>
+
if (files.size() == 1) {
- filePath = files[0].toObject().value("path").toString();
+ filePath = firstFile.value("path").toString();
} else {
- QString path = files[0].toObject().value("path").toString();
</code_context>
<issue_to_address>
**suggestion:** No fallback if 'path' field is missing or not a string.
Downstream code may fail if filePath is empty. Please add validation or a fallback for missing or invalid 'path' values.
```suggestion
QString pathValue;
if (firstFile.contains("path") && firstFile.value("path").isString()) {
pathValue = firstFile.value("path").toString();
} else {
qDebug() << "Warning: 'path' field missing or not a string in file object, using fallback";
pathValue = QString(); // fallback: empty string, or set to a default if desired
}
if (files.size() == 1) {
filePath = pathValue;
} else {
QString path2 = result.value("dir").toString();
if (!pathValue.isEmpty()) {
filePath = path2 + "/" + pathValue.split('/').at(path2.split('/').count());
} else {
filePath = path2; // fallback: just use dir if path is missing
}
//filePath = path.left(path.count() - path.split('/').last().count() - 1);
}
```
</issue_to_address>
### Comment 2
<location> `src/src/ui/mainFrame/tabledatacontrol.cpp:250` </location>
<code_context>
- QJsonObject result = json.value("result").toObject();
- QJsonObject bittorrent = result.value("bittorrent").toObject();
+
+ // Minimal safety check: ensure result field exists and is an object
+ QJsonValue resultValue = json.value("result");
+ if (!resultValue.isObject()) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring repetitive JSON type checks into small helper functions for cleaner and more readable code.
You can collapse all of these repetitive `isObject/isArray` guard‐clauses into three tiny helpers, then your code reads almost like pseudo-code:
```cpp
// somewhere in a shared/util header
static inline QJsonObject asObj (const QJsonValue& v) { return v.isObject() ? v.toObject() : QJsonObject(); }
static inline QJsonArray asArr (const QJsonValue& v) { return v.isArray() ? v.toArray() : QJsonArray(); }
static inline QString asStr (const QJsonValue& v) { return v.isString() ? v.toString() : QString(); }
```
Then your `aria2MethodStatusChanged` can shrink to:
```cpp
bool TableDataControl::aria2MethodStatusChanged(QJsonObject &json, int row, QString &searchContent) {
auto result = asObj(json["result"]);
if (result.isEmpty()) return false;
auto files = asArr(result["files"]);
if (files.isEmpty()) return false;
auto firstFile = asObj(files.first());
QString filePath;
if (files.size() == 1) {
filePath = asStr(firstFile["path"]);
} else {
auto dir = asStr(result["dir"]);
auto relative = asStr(firstFile["path"]);
filePath = dir + "/" + relative.split('/').at(dir.split('/').count());
}
auto uris = asArr(firstFile["uris"]);
QString fileUri = !uris.isEmpty()
? asStr(asObj(uris.first())["uri"])
: QString();
// ...
auto bittorrent = asObj(result["bittorrent"]);
data->announceList = asArr(bittorrent["announceList"]).size();
// rest of logic unchanged
…
}
```
Do the same in `aria2GetGlobalStatus` (and any other JSON access) so you only call `asObj`, `asArr`, and `asStr` instead of littering `isObject`/`isArray` every time.
</issue_to_address>
### Comment 3
<location> `src/src/aria2/aria2rpcinterface.cpp:583` </location>
<code_context>
QByteArray buf = reply->readAll(); //获取信息
- QJsonDocument doc = QJsonDocument::fromJson(buf); //转换为json格式
+
+ // Safety check: validate JSON parsing
+ if (buf.isEmpty()) {
+ qDebug() << "[Aria2RPC] Error: Empty response buffer for method:" << method;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the JSON parsing and validation logic into a reusable helper function to simplify the main handler code.
Here’s a small focused refactoring that pulls all of that JSON‐parsing/validation logic out into one helper. This keeps your `sendMessage` callback almost as short as it was before, but still preserves all of your new safety checks.
In some common header (e.g. `RpcUtils.h`):
```cpp
#pragma once
#include <QJsonDocument>
#include <QJsonObject>
#include <QJsonParseError>
inline bool parseAndValidateJsonObject(
const QByteArray &buf,
const QString &method,
QJsonObject &outObj)
{
if (buf.isEmpty()) {
qDebug() << "[Aria2RPC] Error: Empty response buffer for method:" << method;
return false;
}
QJsonParseError parseError;
QJsonDocument doc = QJsonDocument::fromJson(buf, &parseError);
if (parseError.error != QJsonParseError::NoError) {
qDebug() << "[Aria2RPC] Error: JSON parse failed for method:"
<< method << "Error:" << parseError.errorString()
<< "Offset:" << parseError.offset;
return false;
}
if (!doc.isObject()) {
qDebug() << "[Aria2RPC] Error: JSON document is not an object for method:" << method;
return false;
}
outObj = doc.object();
if (outObj.isEmpty()) {
qDebug() << "[Aria2RPC] Error: JSON object is empty for method:" << method;
return false;
}
return true;
}
```
Then your handler becomes:
```cpp
QByteArray buf = reply->readAll();
QJsonObject obj;
if (!parseAndValidateJsonObject(buf, method, obj)) {
reply->deleteLater();
return;
}
int code = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
if (code == 200) {
qDebug() << "[Aria2RPC] sendMessage, method =" << method << ", code = 200";
emit RPCSuccess(method, obj);
} else {
qDebug() << "[Aria2RPC] sendMessage, method =" << method << ", code =" << code;
emit RPCError(method, id, code, obj);
}
reply->deleteLater();
```
This removes the inline duplication and nesting while keeping every one of your safety checks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
286
to
293
| if (files.size() == 1) { | ||
| filePath = files[0].toObject().value("path").toString(); | ||
| filePath = firstFile.value("path").toString(); | ||
| } else { | ||
| QString path = files[0].toObject().value("path").toString(); | ||
| QString path = firstFile.value("path").toString(); | ||
| QString path2 = result.value("dir").toString(); | ||
| filePath = path2 + "/" + path.split('/').at(path2.split('/').count()); | ||
| //filePath = path.left(path.count() - path.split('/').last().count() - 1); | ||
| } |
There was a problem hiding this comment.
suggestion: No fallback if 'path' field is missing or not a string.
Downstream code may fail if filePath is empty. Please add validation or a fallback for missing or invalid 'path' values.
Suggested change
| if (files.size() == 1) { | |
| filePath = files[0].toObject().value("path").toString(); | |
| filePath = firstFile.value("path").toString(); | |
| } else { | |
| QString path = files[0].toObject().value("path").toString(); | |
| QString path = firstFile.value("path").toString(); | |
| QString path2 = result.value("dir").toString(); | |
| filePath = path2 + "/" + path.split('/').at(path2.split('/').count()); | |
| //filePath = path.left(path.count() - path.split('/').last().count() - 1); | |
| } | |
| QString pathValue; | |
| if (firstFile.contains("path") && firstFile.value("path").isString()) { | |
| pathValue = firstFile.value("path").toString(); | |
| } else { | |
| qDebug() << "Warning: 'path' field missing or not a string in file object, using fallback"; | |
| pathValue = QString(); // fallback: empty string, or set to a default if desired | |
| } | |
| if (files.size() == 1) { | |
| filePath = pathValue; | |
| } else { | |
| QString path2 = result.value("dir").toString(); | |
| if (!pathValue.isEmpty()) { | |
| filePath = path2 + "/" + pathValue.split('/').at(path2.split('/').count()); | |
| } else { | |
| filePath = path2; // fallback: just use dir if path is missing | |
| } | |
| //filePath = path.left(path.count() - path.split('/').last().count() - 1); | |
| } |
lzwind
approved these changes
Sep 23, 2025
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, re2zero The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
/merge |
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
3 participants
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.
Log: Fix crash if magnet fink and drap file to download.
Summary by Sourcery
Fix crash scenarios when handling invalid magnet links and dragging torrent/metalink files by deferring dialog processing asynchronously and adding robust JSON validation across aria2 RPC handling and status updates.
Bug Fixes:
Enhancements: