Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2291453006:
chromeos: Fix potential crash in SessionManagerClient (Closed)
Descriptionchromeos: Fix potential crash in SessionManagerClient
ErrorResponse can be null when an error happens locally.
Also, this logging is unnecessary as ObjectProxy::CallMethod is already
doing that.
BUG=642241
Committed: https://crrev.com/b159d73f7098b1d36aff32c4ff593495a3c710b7
Cr-Commit-Position: refs/heads/master@{#415591}
Patch Set 1 #
Total comments: 1
Messages
Total messages: 25 (9 generated)
The CQ bit was checked by hashimoto@chromium.org to run a CQ dry run
hashimoto@chromium.org changed reviewers: + achuith@chromium.org, derat@chromium.org
Description was changed from ========== chromeos: Fix potential crash in SessionManagerClient ErrorResponse can be null when an error happens locally. Also, this logging unnecessary as ObjectProxy::CallMethod is already doing that. BUG=642241 ========== to ========== chromeos: Fix potential crash in SessionManagerClient ErrorResponse can be null when an error happens locally. Also, this logging is unnecessary as ObjectProxy::CallMethod is already doing that. BUG=642241 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Could you hold off on landing this for a few days until we resolve this P0? This code is temporary anyway. Also, can you point me to where CallMethod is logging errors?
On 2016/08/30 08:08:06, achuithb wrote: > Could you hold off on landing this for a few days until we resolve this P0? This > code is temporary anyway. I don't think we should hold off as this can result in a crash. > > Also, can you point me to where CallMethod is logging errors? ObjectProxy::CallMethod uses ObjectProxy::OnCallMethodError as an error callback which calls LogMethodCallFailure on an error. (https://cs.chromium.org/chromium/src/dbus/object_proxy.cc?l=118&cl=GROK)
On 2016/08/30 08:12:50, hashimoto wrote: > On 2016/08/30 08:08:06, achuithb wrote: > > Could you hold off on landing this for a few days until we resolve this P0? > This > > code is temporary anyway. > I don't think we should hold off as this can result in a crash. > > The bug doesn't link to any actual crashes. Where did you see this crash? Please feel free to add a null check instead. I'll be happy to remove all this code once 631640 is resolved. We are trying to get to the bottom of a P0 release blocker (crbug.com/631640), which is actively affecting the PFQ almost every day. If you have free cycles, please feel free to contribute to the investigation of that bug instead of impeding it. > > Also, can you point me to where CallMethod is logging errors? > ObjectProxy::CallMethod uses ObjectProxy::OnCallMethodError as an error callback > which calls LogMethodCallFailure on an error. > (https://cs.chromium.org/chromium/src/dbus/object_proxy.cc?l=118&cl=GROK) LogMethodCallFailure could potentially not log anything here, right? https://cs.chromium.org/chromium/src/dbus/object_proxy.cc?l=570
https://codereview.chromium.org/2291453006/diff/1/chromeos/dbus/session_manag... File chromeos/dbus/session_manager_client.cc (left): https://codereview.chromium.org/2291453006/diff/1/chromeos/dbus/session_manag... chromeos/dbus/session_manager_client.cc:87: LOG(ERROR) << "DBus error " << response->ToString(); Add a null check, but please log something if you get a null response.
Sorry, I don't understand what you mean by "impeding". I'm just fixing a bug which was introduced in an attempt to fix another bug. I know it's stressful to work on a complicated bug, but it's sad to receive this kind of response in a professional code review. LogMethodCallFailure doesn't log service unknown and object unknown if it's constructed with options, but that's not the case here because SessionManagerClient uses GetObjectProxy instead of GetObjectProxyWithOptions. IIUC you don't need to add any additional logging because LogMethodCallFailure logs all D-Bus errors if ErrorResponse is available. If ErrorResponse is not available, SessionManager's OnRestartJob logs it as it gets called with null dbus::Response.
On 2016/08/30 11:20:07, hashimoto wrote: > Sorry, I don't understand what you mean by "impeding". > I'm just fixing a bug which was introduced in an attempt to fix another bug. > I know it's stressful to work on a complicated bug, but it's sad to receive this > kind of response in a professional code review. > > LogMethodCallFailure doesn't log service unknown and object unknown if it's > constructed with options, but that's not the case here because > SessionManagerClient uses GetObjectProxy instead of GetObjectProxyWithOptions. > > IIUC you don't need to add any additional logging because LogMethodCallFailure > logs all D-Bus errors if ErrorResponse is available. > If ErrorResponse is not available, SessionManager's OnRestartJob logs it as it > gets called with null dbus::Response. lgtm thanks, i see the logging. it looks like we should see either LOG(WARNING) or LOG(ERROR) or failure. it'd probably be good to document the fact that ErrorResponse can be null in dbus/object_proxy.h -- it wasn't obvious to me that that can happen without reading the implementation.
On 2016/08/30 15:15:05, Daniel Erat wrote: > On 2016/08/30 11:20:07, hashimoto wrote: > > Sorry, I don't understand what you mean by "impeding". > > I'm just fixing a bug which was introduced in an attempt to fix another bug. > > I know it's stressful to work on a complicated bug, but it's sad to receive > this > > kind of response in a professional code review. > > > > LogMethodCallFailure doesn't log service unknown and object unknown if it's > > constructed with options, but that's not the case here because > > SessionManagerClient uses GetObjectProxy instead of GetObjectProxyWithOptions. > > > > IIUC you don't need to add any additional logging because LogMethodCallFailure > > logs all D-Bus errors if ErrorResponse is available. > > If ErrorResponse is not available, SessionManager's OnRestartJob logs it as it > > gets called with null dbus::Response. > > lgtm > > thanks, i see the logging. it looks like we should see either LOG(WARNING) or > LOG(ERROR) or failure. > > it'd probably be good to document the fact that ErrorResponse can be null in > dbus/object_proxy.h -- it wasn't obvious to me that that can happen without > reading the implementation. s/or failure/on failure/
On 2016/08/30 11:20:07, hashimoto wrote: > Sorry, I don't understand what you mean by "impeding". > I'm just fixing a bug which was introduced in an attempt to fix another bug. > I know it's stressful to work on a complicated bug, but it's sad to receive this > kind of response in a professional code review. > > LogMethodCallFailure doesn't log service unknown and object unknown if it's > constructed with options, but that's not the case here because > SessionManagerClient uses GetObjectProxy instead of GetObjectProxyWithOptions. > > IIUC you don't need to add any additional logging because LogMethodCallFailure > logs all D-Bus errors if ErrorResponse is available. > If ErrorResponse is not available, SessionManager's OnRestartJob logs it as it > gets called with null dbus::Response. I apologize for my tone in that case. lgtm.
On 2016/08/30 18:02:36, achuithb wrote: > On 2016/08/30 11:20:07, hashimoto wrote: > > Sorry, I don't understand what you mean by "impeding". > > I'm just fixing a bug which was introduced in an attempt to fix another bug. > > I know it's stressful to work on a complicated bug, but it's sad to receive > this > > kind of response in a professional code review. > > > > LogMethodCallFailure doesn't log service unknown and object unknown if it's > > constructed with options, but that's not the case here because > > SessionManagerClient uses GetObjectProxy instead of GetObjectProxyWithOptions. > > > > IIUC you don't need to add any additional logging because LogMethodCallFailure > > logs all D-Bus errors if ErrorResponse is available. > > If ErrorResponse is not available, SessionManager's OnRestartJob logs it as it > > gets called with null dbus::Response. > > I apologize for my tone in that case. > > lgtm. no worries.
On 2016/08/30 15:15:22, Daniel Erat wrote: > On 2016/08/30 15:15:05, Daniel Erat wrote: > > On 2016/08/30 11:20:07, hashimoto wrote: > > > Sorry, I don't understand what you mean by "impeding". > > > I'm just fixing a bug which was introduced in an attempt to fix another bug. > > > I know it's stressful to work on a complicated bug, but it's sad to receive > > this > > > kind of response in a professional code review. > > > > > > LogMethodCallFailure doesn't log service unknown and object unknown if it's > > > constructed with options, but that's not the case here because > > > SessionManagerClient uses GetObjectProxy instead of > GetObjectProxyWithOptions. > > > > > > IIUC you don't need to add any additional logging because > LogMethodCallFailure > > > logs all D-Bus errors if ErrorResponse is available. > > > If ErrorResponse is not available, SessionManager's OnRestartJob logs it as > it > > > gets called with null dbus::Response. > > > > lgtm > > > > thanks, i see the logging. it looks like we should see either LOG(WARNING) or > > LOG(ERROR) or failure. > > > > it'd probably be good to document the fact that ErrorResponse can be null in > > dbus/object_proxy.h -- it wasn't obvious to me that that can happen without > > reading the implementation. > > s/or failure/on failure/ It's already written in object_proxy.h (https://cs.chromium.org/chromium/src/dbus/object_proxy.h?rcl=0&l=143), but maybe we should change the comment to make it more obvious. Do you have any idea for improvement? I am the one who added CallMethodWithErrorCallback in https://chromiumcodereview.appspot.com/10121005 with the existing comment, so probably I'm a too bad writer to rewrite it.
The CQ bit was checked by hashimoto@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== chromeos: Fix potential crash in SessionManagerClient ErrorResponse can be null when an error happens locally. Also, this logging is unnecessary as ObjectProxy::CallMethod is already doing that. BUG=642241 ========== to ========== chromeos: Fix potential crash in SessionManagerClient ErrorResponse can be null when an error happens locally. Also, this logging is unnecessary as ObjectProxy::CallMethod is already doing that. BUG=642241 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== chromeos: Fix potential crash in SessionManagerClient ErrorResponse can be null when an error happens locally. Also, this logging is unnecessary as ObjectProxy::CallMethod is already doing that. BUG=642241 ========== to ========== chromeos: Fix potential crash in SessionManagerClient ErrorResponse can be null when an error happens locally. Also, this logging is unnecessary as ObjectProxy::CallMethod is already doing that. BUG=642241 Committed: https://crrev.com/b159d73f7098b1d36aff32c4ff593495a3c710b7 Cr-Commit-Position: refs/heads/master@{#415591} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/b159d73f7098b1d36aff32c4ff593495a3c710b7 Cr-Commit-Position: refs/heads/master@{#415591}
Message was sent while issue was closed.
On 2016/08/31 08:29:34, hashimoto wrote: > On 2016/08/30 15:15:22, Daniel Erat wrote: > > On 2016/08/30 15:15:05, Daniel Erat wrote: > > > On 2016/08/30 11:20:07, hashimoto wrote: > > > > Sorry, I don't understand what you mean by "impeding". > > > > I'm just fixing a bug which was introduced in an attempt to fix another > bug. > > > > I know it's stressful to work on a complicated bug, but it's sad to > receive > > > this > > > > kind of response in a professional code review. > > > > > > > > LogMethodCallFailure doesn't log service unknown and object unknown if > it's > > > > constructed with options, but that's not the case here because > > > > SessionManagerClient uses GetObjectProxy instead of > > GetObjectProxyWithOptions. > > > > > > > > IIUC you don't need to add any additional logging because > > LogMethodCallFailure > > > > logs all D-Bus errors if ErrorResponse is available. > > > > If ErrorResponse is not available, SessionManager's OnRestartJob logs it > as > > it > > > > gets called with null dbus::Response. > > > > > > lgtm > > > > > > thanks, i see the logging. it looks like we should see either LOG(WARNING) > or > > > LOG(ERROR) or failure. > > > > > > it'd probably be good to document the fact that ErrorResponse can be null in > > > dbus/object_proxy.h -- it wasn't obvious to me that that can happen without > > > reading the implementation. > > > > s/or failure/on failure/ > It's already written in object_proxy.h > (https://cs.chromium.org/chromium/src/dbus/object_proxy.h?rcl=0&l=143), but > maybe we should change the comment to make it more obvious. > Do you have any idea for improvement? > I am the one who added CallMethodWithErrorCallback in > https://chromiumcodereview.appspot.com/10121005 with the existing comment, so > probably I'm a too bad writer to rewrite it. ah, i think i misunderstood it as saying that the non-error callback will get NULL: // If the method call is successful, a pointer to Response object will // be passed to the callback. If unsuccessful, the error callback will be // called and a pointer to ErrorResponse object will be passed to the error // callback if available, otherwise NULL will be passed. maybe this would be a bit clearer? "If unsuccessful, the error callback will be invoked with an ErrorResponse object (if the remote object returned an error) or NULL (if a response was not received at all)." it's also not completely clear to me when the non-error callback will be invoked with NULL. from glancing at the code, i *think* that that only happens when the call fails after you use ObjectProxy::CallMethod() instead of passing your own error callback. is that correct?
Message was sent while issue was closed.
On 2016/08/31 15:33:05, Daniel Erat wrote: > On 2016/08/31 08:29:34, hashimoto wrote: > > On 2016/08/30 15:15:22, Daniel Erat wrote: > > > On 2016/08/30 15:15:05, Daniel Erat wrote: > > > > On 2016/08/30 11:20:07, hashimoto wrote: > > > > > Sorry, I don't understand what you mean by "impeding". > > > > > I'm just fixing a bug which was introduced in an attempt to fix another > > bug. > > > > > I know it's stressful to work on a complicated bug, but it's sad to > > receive > > > > this > > > > > kind of response in a professional code review. > > > > > > > > > > LogMethodCallFailure doesn't log service unknown and object unknown if > > it's > > > > > constructed with options, but that's not the case here because > > > > > SessionManagerClient uses GetObjectProxy instead of > > > GetObjectProxyWithOptions. > > > > > > > > > > IIUC you don't need to add any additional logging because > > > LogMethodCallFailure > > > > > logs all D-Bus errors if ErrorResponse is available. > > > > > If ErrorResponse is not available, SessionManager's OnRestartJob logs it > > as > > > it > > > > > gets called with null dbus::Response. > > > > > > > > lgtm > > > > > > > > thanks, i see the logging. it looks like we should see either LOG(WARNING) > > or > > > > LOG(ERROR) or failure. > > > > > > > > it'd probably be good to document the fact that ErrorResponse can be null > in > > > > dbus/object_proxy.h -- it wasn't obvious to me that that can happen > without > > > > reading the implementation. > > > > > > s/or failure/on failure/ > > It's already written in object_proxy.h > > (https://cs.chromium.org/chromium/src/dbus/object_proxy.h?rcl=0&l=143), but > > maybe we should change the comment to make it more obvious. > > Do you have any idea for improvement? > > I am the one who added CallMethodWithErrorCallback in > > https://chromiumcodereview.appspot.com/10121005 with the existing comment, so > > probably I'm a too bad writer to rewrite it. > > ah, i think i misunderstood it as saying that the non-error callback will get > NULL: > > // If the method call is successful, a pointer to Response object will > // be passed to the callback. If unsuccessful, the error callback will be > // called and a pointer to ErrorResponse object will be passed to the error > // callback if available, otherwise NULL will be passed. > > maybe this would be a bit clearer? > > "If unsuccessful, the error callback will be invoked with an ErrorResponse > object (if the remote object returned an error) or NULL (if a response was not > received at all)." Looks great, thanks! Made a CL https://codereview.chromium.org/2299853002 > > it's also not completely clear to me when the non-error callback will be invoked > with NULL. from glancing at the code, i *think* that that only happens when the > call fails after you use ObjectProxy::CallMethod() instead of passing your own > error callback. is that correct? Correct. CallMethod(): No response -> callback.Run(nullptr) Error response -> callback.Run(nullptr) + logging Success -> callback.Run(response) CallMethodWithErrorCallback(): No response -> error_callback.Run(nullptr) Error response -> error_callback.Run(error_response) Success -> callback.Run(response) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
