ubuntu-download-manager

Merge lp:~michael-sheldon/ubuntu-download-manager/fix-1628635 into lp:ubuntu-download-manager

  1. fix-1628635
  2. Merge into trunk
Proposed by Michael Sheldon
Status: Merged
Approved by: Ken VanDine
Approved revision: 375
Merged at revision: 377
Proposed branch: lp:~michael-sheldon/ubuntu-download-manager/fix-1628635
Merge into: lp:ubuntu-download-manager
Diff against target: 301 lines (+103/-12)
3 files modified
src/common/priv/ubuntu/transfers/base_daemon.cpp (+2/-2)
src/downloads/client/ubuntu/download_manager/download_impl.cpp (+90/-0)
tests/downloads/daemon/test_daemon.cpp (+11/-10)
To merge this branch: bzr merge lp:~michael-sheldon/ubuntu-download-manager/fix-1628635
Reviewer Review Type Date Requested Status
system-apps-ci-bot continuous-integration Needs Fixing
Ubuntu Phablet Team Pending
Review via email: mp+307313@code.launchpad.net

Commit message

Reorder dbus object and service registration to be compatible with Qt 5.6
Fix crash when client can't talk to dbus service

Description of the change

Reorder dbus object and service registration to be compatible with Qt 5.6
Fix crash when client can't talk to dbus service

To post a comment you must log in.
Revision history for this message
system-apps-ci-bot (system-apps-ci-bot) wrote :

FAILED: Continuous integration, rev:375
https://jenkins.canonical.com/system-apps/job/lp-ubuntu-download-manager-ci/20/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/system-apps/job/build/1692/console
    FAILURE: https://jenkins.canonical.com/system-apps/job/build-0-fetch/1692/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-ubuntu-download-manager-ci/20/rebuild

review: Needs Fixing (continuous-integration)
lp:~michael-sheldon/ubuntu-download-manager/fix-1628635 updated
376. By Michael Sheldon

Add checks full invalid dbus interface in DownloadImpl

377. By Michael Sheldon

Check validity of dbus interface as well as whether or not it's null prior to usage

378. By Michael Sheldon

Reorder dbus object and service registration to be compatible with Qt 5.6

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/common/priv/ubuntu/transfers/base_daemon.cpp'
2--- src/common/priv/ubuntu/transfers/base_daemon.cpp 2014-10-22 23:40:53 +0000
3+++ src/common/priv/ubuntu/transfers/base_daemon.cpp 2016-11-03 13:38:52 +0000
4@@ -117,10 +117,10 @@
5 TRACE;
6 _path = path;
7 _managerAdaptor = _adaptorFactory->createAdaptor(_manager);
8- bool ret = _conn->registerService(_path);
9+ bool ret = _conn->registerObject("/", _manager);
10 if (ret) {
11 LOG(INFO) << "Service registered to" << _path;
12- ret = _conn->registerObject("/", _manager);
13+ ret = _conn->registerService(_path);
14 if (!ret) {
15 LOG(ERROR) << "Could not register interface. DBus Error =>"
16 << _conn->connection().lastError();
17
18=== modified file 'src/downloads/client/ubuntu/download_manager/download_impl.cpp'
19--- src/downloads/client/ubuntu/download_manager/download_impl.cpp 2016-01-12 01:07:34 +0000
20+++ src/downloads/client/ubuntu/download_manager/download_impl.cpp 2016-11-03 13:38:52 +0000
21@@ -185,6 +185,10 @@
22
23 void
24 DownloadImpl::start() {
25+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
26+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
27+ return;
28+ }
29 Logger::log(Logger::Debug, QString("Download{%1} start())").arg(_id));
30 QDBusPendingCall call =
31 _dbusInterface->start();
32@@ -195,6 +199,10 @@
33
34 void
35 DownloadImpl::pause() {
36+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
37+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
38+ return;
39+ }
40 Logger::log(Logger::Debug, QString("Download{%1} pause())").arg(_id));
41 QDBusPendingCall call =
42 _dbusInterface->pause();
43@@ -205,6 +213,10 @@
44
45 void
46 DownloadImpl::resume() {
47+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
48+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
49+ return;
50+ }
51 Logger::log(Logger::Debug, QString("Download{%1} resume())").arg(_id));
52 QDBusPendingCall call =
53 _dbusInterface->resume();
54@@ -215,6 +227,10 @@
55
56 void
57 DownloadImpl::cancel() {
58+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
59+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
60+ return;
61+ }
62 Logger::log(Logger::Debug, QString("Download{%1} cancel())").arg(_id));
63 QDBusPendingCall call =
64 _dbusInterface->cancel();
65@@ -225,6 +241,10 @@
66
67 void
68 DownloadImpl::collected() {
69+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
70+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
71+ return;
72+ }
73 Logger::log(Logger::Debug, QString("Download{%1} collected()").arg(_id));
74 QDBusPendingReply<> reply =
75 _dbusInterface->collected();
76@@ -238,6 +258,10 @@
77
78 void
79 DownloadImpl::allowMobileDownload(bool allowed) {
80+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
81+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
82+ return;
83+ }
84 Logger::log(Logger::Debug,
85 QString("Download{%1} allowMobileDownload%2())").arg(_id).arg(allowed));
86 QDBusPendingReply<> reply =
87@@ -252,6 +276,10 @@
88
89 bool
90 DownloadImpl::isMobileDownloadAllowed() {
91+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
92+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
93+ return false;
94+ }
95 Logger::log(Logger::Debug,
96 QString("Download{%1} isMobileDownloadAllowed").arg(_id));
97 QDBusPendingReply<bool> reply =
98@@ -270,6 +298,10 @@
99
100 void
101 DownloadImpl::setDestinationDir(const QString& path) {
102+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
103+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
104+ return;
105+ }
106 Logger::log(Logger::Debug, QString("Dowmload{%1} setDestinationDir(%2)")
107 .arg(_id).arg(path));
108 QDBusPendingReply<> reply =
109@@ -284,6 +316,10 @@
110
111 void
112 DownloadImpl::setHeaders(QMap<QString, QString> headers) {
113+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
114+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
115+ return;
116+ }
117 Logger::log(Logger::Debug,
118 QString("Download {%1} setHeaders(%2)").arg(_id), headers);
119
120@@ -299,6 +335,11 @@
121
122 QVariantMap
123 DownloadImpl::metadata() {
124+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
125+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
126+ QVariantMap emptyResult;
127+ return emptyResult;
128+ }
129 Logger::log(Logger::Debug, QString("Download{%1} metadata()").arg(_id));
130 QDBusPendingReply<QVariantMap> reply =
131 _dbusInterface->metadata();
132@@ -317,6 +358,10 @@
133
134 void
135 DownloadImpl::setMetadata(QVariantMap map) {
136+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
137+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
138+ return;
139+ }
140 Logger::log(Logger::Debug,
141 QString("Download {%1} setMetadata(%2)").arg(_id), map);
142
143@@ -332,6 +377,11 @@
144
145 QMap<QString, QString>
146 DownloadImpl::headers() {
147+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
148+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
149+ QMap<QString, QString> empty;
150+ return empty;
151+ }
152 Logger::log(Logger::Debug, QString("Download{%1} headers()").arg(_id));
153 QDBusPendingReply<QMap<QString, QString> > reply =
154 _dbusInterface->headers();
155@@ -351,6 +401,10 @@
156
157 void
158 DownloadImpl::setThrottle(qulonglong speed) {
159+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
160+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
161+ return;
162+ }
163 Logger::log(Logger::Debug,
164 QString("Download{%1} setThrottle(%2)").arg(_id).arg(speed));
165 QDBusPendingReply<> reply =
166@@ -365,6 +419,10 @@
167
168 qulonglong
169 DownloadImpl::throttle() {
170+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
171+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
172+ return 0;
173+ }
174 Logger::log(Logger::Debug, QString("Download{%1} throttle()").arg(_id));
175 QDBusPendingReply<qulonglong> reply =
176 _dbusInterface->throttle();
177@@ -382,6 +440,10 @@
178
179 QString
180 DownloadImpl::filePath() {
181+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
182+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
183+ return "";
184+ }
185 Logger::log(Logger::Debug, QString("Download{%1} filePath()").arg(_id));
186 QDBusPendingReply<QString> reply =
187 _dbusInterface->filePath();
188@@ -399,6 +461,10 @@
189
190 Download::State
191 DownloadImpl::state() {
192+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
193+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
194+ return Download::ERROR;
195+ }
196 Logger::log(Logger::Debug, QString("Download{%1} state()").arg(_id));
197 QDBusPendingReply<int> reply =
198 _dbusInterface->state();
199@@ -421,6 +487,10 @@
200
201 qulonglong
202 DownloadImpl::progress() {
203+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
204+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
205+ return 0;
206+ }
207 Logger::log(Logger::Debug, QString("Download{%1} progress()").arg(_id));
208 QDBusPendingReply<qulonglong> reply =
209 _dbusInterface->progress();
210@@ -438,6 +508,10 @@
211
212 qulonglong
213 DownloadImpl::totalSize() {
214+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
215+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
216+ return 0;
217+ }
218 Logger::log(Logger::Debug, QString("Download{%1} totalSize()").arg(_id));
219 QDBusPendingReply<qulonglong> reply =
220 _dbusInterface->totalSize();
221@@ -465,21 +539,37 @@
222
223 QString
224 DownloadImpl::clickPackage() const {
225+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
226+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
227+ return "";
228+ }
229 return _dbusInterface->clickPackage();
230 }
231
232 bool
233 DownloadImpl::showInIndicator() const {
234+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
235+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
236+ return false;
237+ }
238 return _dbusInterface->showInIndicator();
239 }
240
241 QString
242 DownloadImpl::title() const {
243+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
244+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
245+ return "";
246+ }
247 return _dbusInterface->title();
248 }
249
250 QString
251 DownloadImpl::destinationApp() const {
252+ if (_dbusInterface == nullptr || !_dbusInterface->isValid()) {
253+ Logger::log(Logger::Error, QString("Invalid dbus interface: %1").arg(_lastError->errorString()));
254+ return "";
255+ }
256 return _dbusInterface->destinationApp();
257 }
258
259
260=== modified file 'tests/downloads/daemon/test_daemon.cpp'
261--- tests/downloads/daemon/test_daemon.cpp 2014-11-17 20:34:24 +0000
262+++ tests/downloads/daemon/test_daemon.cpp 2016-11-03 13:38:52 +0000
263@@ -151,12 +151,13 @@
264 .Times(1)
265 .WillRepeatedly(Return(args));
266
267+ EXPECT_CALL(*conn.data(), registerObject(_, _, _))
268+ .Times(1)
269+ .WillRepeatedly(Return(false));
270+
271 EXPECT_CALL(*conn.data(),
272 registerService(QString("com.canonical.applications.Downloader")))
273- .Times(1)
274- .WillRepeatedly(Return(false));
275- EXPECT_CALL(*conn.data(), registerObject(_, _, _))
276- .Times(0);
277+ .Times(0);
278
279 EXPECT_CALL(*factory, createManager(_, _, _, _))
280 .Times(1)
281@@ -196,14 +197,14 @@
282 .Times(1)
283 .WillRepeatedly(Return(args));
284
285+ EXPECT_CALL(*conn.data(), registerObject(_, _, _))
286+ .Times(1)
287+ .WillRepeatedly(Return(false));
288+
289 EXPECT_CALL(*conn.data(),
290 registerService(QString("com.canonical.applications.Downloader")))
291- .Times(1)
292- .WillRepeatedly(Return(true));
293- EXPECT_CALL(*conn.data(), registerObject(_, _, _))
294- .Times(1)
295- .WillRepeatedly(Return(false));
296-
297+ .Times(0);
298+
299 EXPECT_CALL(*factory, createManager(app, conn.data(), _, _))
300 .Times(1)
301 .WillRepeatedly(Return(man));

Subscribers

People subscribed via source and target branches