New issue
Advanced search Search tips

Issue 736472 link

Starred by 2 users
Status: Fixed
Owner:
mmenke@chromium.org
Closed: Jun 2017
Cc:
jam@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug
Proj-Servicification



Sign in to add a comment

NetworkService::CreateNetworkContext calls std::move twice on the same object.

Project Member Reported by mmenke@chromium.org, Jun 23 2017 
NetworkService::CreateNetworkContext calls std::move(request) twice on the request object in the same line, which has undefined before.  I assume that the NetworkContext should not be setting up a binding itself, and only the StrongBinding should be set up instead, but I'm not all that familiar with Mojo.

Comment 1 by mmenke@chromium.org, Jun 23 2017

Cc: -mmenke@chromium.org
Owner: mmenke@chromium.org
Status: Assigned (was: Untriaged)
Think I'll take this on - I'll need to switch it to a weak binding so the NetworkService can destroy them on shutdown, anyways (Since NetworkContexts will be sharing the NetLog and some other objects with each other, which will be owned by the NetworkService)

Comment 3 by mmenke@chromium.org, Jun 27 2017

Status: Fixed (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1caf74b3fa2e801138207bae19471d408298268b

commit 1caf74b3fa2e801138207bae19471d408298268b
Author: mmenke <mmenke@chromium.org>
Date: Tue Jun 27 22:19:07 2017

Revert of NetworkService: Destroy NetworkContexts on NetworkService teardown. (patchset #3 id:40001 of https://codereview.chromium.org/2962693002/ )

Reason for revert:
This conflicted with https://codereview.chromium.org/2951813002/, which landed at about the same time.  Will fix and reland.

Original issue's description:
> NetworkService: Destroy NetworkContexts on NetworkService teardown.
>
> Also fix NetworkContexts potentially not being destroyed on connection
> error / consumer teardown, due to calling str::move twice on the same
> RequestPtr.
>
> BUG= 736472 
>
> Review-Url: https://codereview.chromium.org/2962693002
> Cr-Commit-Position: refs/heads/master@{#482745}
> Committed: https://chromium.googlesource.com/chromium/src/+/cfbf85b0c37adf3acccead282bf5c9b794ba4009

TBR=jam@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 736472 

Review-Url: https://codereview.chromium.org/2960843005
Cr-Commit-Position: refs/heads/master@{#482763}

[modify] https://crrev.com/1caf74b3fa2e801138207bae19471d408298268b/content/network/network_context.cc
[modify] https://crrev.com/1caf74b3fa2e801138207bae19471d408298268b/content/network/network_context.h
[modify] https://crrev.com/1caf74b3fa2e801138207bae19471d408298268b/content/network/network_service.cc
[modify] https://crrev.com/1caf74b3fa2e801138207bae19471d408298268b/content/network/network_service.h
[delete] https://crrev.com/b962595bafd843504c3489fe8dbbf5e4c518528b/content/network/network_service_unittest.cc
[modify] https://crrev.com/1caf74b3fa2e801138207bae19471d408298268b/content/test/BUILD.gn
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 29 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/502a6133a5974683b23edab936d6b480dcd7684e

commit 502a6133a5974683b23edab936d6b480dcd7684e
Author: mmenke <mmenke@chromium.org>
Date: Thu Jun 29 00:36:29 2017

NetworkService: Destroy NetworkContexts on NetworkService teardown.

Also fix NetworkContexts potentially not being destroyed on connection
error / consumer teardown, due to calling str::move twice on the same
RequestPtr.

This is a reland of https://codereview.chromium.org/2962693002,
which was reverted in https://codereview.chromium.org/2960843005/ due
to a conflict with https://codereview.chromium.org/2951813002/. The
solution was to add a base::test::ScopedTaskEnvironment to the tests.

BUG= 736472 

Review-Url: https://codereview.chromium.org/2963823002
Cr-Commit-Position: refs/heads/master@{#483225}

[modify] https://crrev.com/502a6133a5974683b23edab936d6b480dcd7684e/content/network/network_context.cc
[modify] https://crrev.com/502a6133a5974683b23edab936d6b480dcd7684e/content/network/network_context.h
[modify] https://crrev.com/502a6133a5974683b23edab936d6b480dcd7684e/content/network/network_service.cc
[modify] https://crrev.com/502a6133a5974683b23edab936d6b480dcd7684e/content/network/network_service.h
[add] https://crrev.com/502a6133a5974683b23edab936d6b480dcd7684e/content/network/network_service_unittest.cc
[modify] https://crrev.com/502a6133a5974683b23edab936d6b480dcd7684e/content/test/BUILD.gn

Comment 6 by laforge@google.com, Nov 7 2017

Components: -Internals>Network>Service Internals>Services>Network
Apologies, applied the wrong component in bulk.
►

Sign in to add a comment