New issue
Advanced search Search tips

Issue 792615 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Dec 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 724628



Sign in to add a comment

shill unit test failing with libc++

Project Member Reported by manojgupta@chromium.org, Dec 6 2017

Issue description

Reverting https://chromium-review.googlesource.com/c/aosp/platform/system/connectivity/shill/+/761699 seems to fix the problem. 
I am not sure of the root cause, but somehow when using const string references, gtest is accessing a void memory resulting in SEGV.

Attaching the backtrace.


gdb.txt
11.5 KB View Download
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.
Cc: manojgupta@chromium.org
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?

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.
Labels: M-64
The fix should probably be merged back to M64 as well as the issue doesn't seem to be libc++ specific
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Labels: Merge-Request-64
Requesting merge to 64.
Project Member

Comment 8 by sheriffbot@chromium.org, Dec 14 2017

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
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
Labels: -Merge-Review-64 Merge-Approved-64
Approving merge to M64 Chrome OS.
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 14 2017

Labels: merge-merged-release-R64-10176.B
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

Status: Verified (was: Untriaged)
Labels: -Hotlist-Merge-Review -Merge-Approved-64 -merge-merged-release-R64-10176.B

Sign in to add a comment