shill unit test failing with libc++ |
|||||||||
Issue descriptionThe fail started between Nov 9 and Nov 10, 2017. failure is only when libc++ is used as the c++ std library. Nov 9 run: Shill passed. https://logs.chromium.org/v/?s=chromeos%2Fbb%2Fchromiumos.tryserver%2Fllvm_toolchain%2F476%2F%2B%2Frecipes%2Fsteps%2FUnitTest%2F0%2Fstdout Nov 10 run: Shill failed: https://logs.chromium.org/v/?s=chromeos%2Fbb%2Fchromiumos.tryserver%2Fllvm_toolchain%2F480%2F%2B%2Frecipes%2Fsteps%2FUnitTest%2F0%2Fstdout
,
Dec 7 2017
Still not exactly clear why the test fails with libc++ but this a guess:
The Bind Call is makeing a copy of the references to the string objects instead of the contents.
registration_dropped_update_callback_.Reset(
Bind(&CellularCapabilityUniversal::Handle3gppRegistrationChange,
weak_ptr_factory_.GetWeakPtr(),
state,
operator_code, // references are copied.
operator_name));
cellular()->dispatcher()->PostDelayedTask(
FROM_HERE,
registration_dropped_update_callback_.callback(),
registration_dropped_update_timeout_milliseconds_);
The suspicion is that the memory pointed to by reference is not necessarily available when the actual callback happens.
,
Dec 13 2017
After spending an eternity with this, I believe I have the root cause now. There is a call to Cancel the callback in the beginning of the function. However, the call also destructs the copies that were made during the callback registration.
Moving the Cancel() to end of function makes shill unit tests happy again.
void CellularCapabilityUniversal::Handle3gppRegistrationChange(
MMModem3gppRegistrationState updated_state,
const string& updated_operator_code,
const string& updated_operator_name) {
// A finished callback does not qualify as a canceled callback.
// We test for a canceled callback to check for outstanding callbacks.
// So, explicitly cancel the callback here.
**** Cancel also destroys the arguments that were copied ******
**** Moving this to end of function makes everyone happy.******
registration_dropped_update_callback_.Cancel();
SLOG(this, 3) << __func__ << ": regstate=" << updated_state
<< ", opercode=" << updated_operator_code
<< ", opername=" << updated_operator_name;
registration_state_ = updated_state;
serving_operator_[kOperatorCodeKey] = updated_operator_code;
serving_operator_[kOperatorNameKey] = updated_operator_name;
cellular()->serving_operator_info()->UpdateMCCMNC(updated_operator_code);
cellular()->serving_operator_info()->UpdateOperatorName(
updated_operator_name);
cellular()->HandleNewRegistrationState();
// If the modem registered with the network and the current ICCID is pending
// activation, then reset the modem.
UpdatePendingActivationState();
**** Moving cancel() here makes me happy ****
}
benchan@ Do you agree with the inference I made?
,
Dec 13 2017
manojgupta@, thanks for digging into the root cause. that makes pretty sense. registration_dropped_update_callback_ is bound to Handle3gppRegistrationChange, so the arguments for Handle3gppRegistrationChange are captured as internal state of registration_dropped_update_callback_. If Handle3gppRegistrationChange is invoked as a result of registration_dropped_update_callback_ being called, registration_dropped_update_callback_.Cancel() would invalidate the arguments too early as the rest of Handle3gppRegistrationChange code still access them.
,
Dec 14 2017
The fix should probably be merged back to M64 as well as the issue doesn't seem to be libc++ specific
,
Dec 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/aosp/platform/system/connectivity/shill/+/3e6977e2496ab535f74b2e55fcf23d3588c7fc2e commit 3e6977e2496ab535f74b2e55fcf23d3588c7fc2e Author: Manoj Gupta <manojgupta@google.com> Date: Thu Dec 14 06:33:28 2017 shill: Fix a unit test failure with libc++. The callback Cancel also destoys the function arguments copies made when registering the callback. Move the call to Cancel() after the arguments have been consumed. BUG= chromium:792615 TEST=Shill Unit Tests pass with libc++. Change-Id: Id20103a4870487bb38fa1cecb44ac35634ec0840 Reviewed-on: https://chromium-review.googlesource.com/825953 Commit-Ready: Manoj Gupta <manojgupta@chromium.org> Tested-by: Manoj Gupta <manojgupta@chromium.org> Reviewed-by: Ben Chan <benchan@chromium.org> [modify] https://crrev.com/3e6977e2496ab535f74b2e55fcf23d3588c7fc2e/cellular/cellular_capability_universal.cc
,
Dec 14 2017
Requesting merge to 64.
,
Dec 14 2017
This bug requires manual review: M64 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 14 2017
Approving merge to M64 Chrome OS.
,
Dec 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/aosp/platform/system/connectivity/shill/+/f905be9a45b1ae85d779a9e4b065a3bccebc4452 commit f905be9a45b1ae85d779a9e4b065a3bccebc4452 Author: Manoj Gupta <manojgupta@google.com> Date: Thu Dec 14 18:03:10 2017 shill: Fix a unit test failure with libc++. The callback Cancel also destoys the function arguments copies made when registering the callback. Move the call to Cancel() after the arguments have been consumed. BUG= chromium:792615 TEST=Shill Unit Tests pass with libc++. Change-Id: Id20103a4870487bb38fa1cecb44ac35634ec0840 Reviewed-on: https://chromium-review.googlesource.com/825953 Commit-Ready: Manoj Gupta <manojgupta@chromium.org> Tested-by: Manoj Gupta <manojgupta@chromium.org> Reviewed-by: Ben Chan <benchan@chromium.org> (cherry picked from commit 3e6977e2496ab535f74b2e55fcf23d3588c7fc2e) [modify] https://crrev.com/f905be9a45b1ae85d779a9e4b065a3bccebc4452/cellular/cellular_capability_universal.cc
,
Dec 14 2017
,
Jan 2 2018
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by manojgupta@chromium.org
, Dec 6 201711.5 KB
11.5 KB View Download