New issue
Advanced search Search tips

Issue 900943 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 5
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug



Sign in to add a comment

BackgroundSync can think it's online when it's offline

Project Member Reported by jkarlin@google.com, Nov 1

Issue description

The BackgroundSyncNetworkObserver's call to GetConnectionType() on startup won't update the connection type if the call returns synchronously.

By default the manager thinks the network type is unknown (therefore online). So if it's actually offline then the background sync manager won't know this and will fire its events anyway.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 1

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

commit bf943695d0c24fed3f7a1b90566e66b5a8482c36
Author: Josh Karlin <jkarlin@chromium.org>
Date: Thu Nov 01 20:14:59 2018

[BackgroundSync] BGSync missing its initial connection type request

After the switch to NetworkConnectionTracker, the GetConnectionType
call wasn't checking for a synchronous return. In the case of a
synchronous return OnConnectionChanged needs to be called. This led to
BackgroundSyncManager not getting the correct connection type on
startup.

Bug:  900943 
Change-Id: I21ffcacdea9cca4c0780f40561e55b10fb9866b1
Reviewed-on: https://chromium-review.googlesource.com/c/1312676
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604689}
[modify] https://crrev.com/bf943695d0c24fed3f7a1b90566e66b5a8482c36/content/browser/background_sync/background_sync_network_observer.cc
[modify] https://crrev.com/bf943695d0c24fed3f7a1b90566e66b5a8482c36/content/browser/background_sync/background_sync_network_observer.h
[modify] https://crrev.com/bf943695d0c24fed3f7a1b90566e66b5a8482c36/content/browser/background_sync/background_sync_network_observer_unittest.cc

Labels: Merge-Request-71
Requesting merge into M71 as this is a pretty bad bug and a small fix. So far Chrome Canary seems okay.
Pls apply appropriate OSs label.
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Project Member

Comment 5 by sheriffbot@chromium.org, Nov 2

Labels: -Merge-Request-71 Hotlist-Merge-Reject Merge-Reject-71
The bug is marked as P3 or Feature. It should not be merged as M71 is in beta. 
Please contact the approriate milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Pri-3 -Hotlist-Merge-Reject -Merge-Reject-71 Merge-Request-71 Pri-1
Updated priority and rerequesting merge.
Project Member

Comment 7 by sheriffbot@chromium.org, Nov 2

Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-71 Merge-Approved-71
Approving merge to M71 branch 3578 based on comment #2. Please merge ASAP. Thank you.
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 5

Labels: -merge-approved-71 merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/562022db7ba4767c4764574b2c01c714560f1305

commit 562022db7ba4767c4764574b2c01c714560f1305
Author: Josh Karlin <jkarlin@chromium.org>
Date: Mon Nov 05 13:15:12 2018

[BackgroundSync] BGSync missing its initial connection type request

After the switch to NetworkConnectionTracker, the GetConnectionType
call wasn't checking for a synchronous return. In the case of a
synchronous return OnConnectionChanged needs to be called. This led to
BackgroundSyncManager not getting the correct connection type on
startup.

Bug:  900943 
Change-Id: I21ffcacdea9cca4c0780f40561e55b10fb9866b1
Reviewed-on: https://chromium-review.googlesource.com/c/1312676
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#604689}(cherry picked from commit bf943695d0c24fed3f7a1b90566e66b5a8482c36)
Reviewed-on: https://chromium-review.googlesource.com/c/1317829
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#495}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/562022db7ba4767c4764574b2c01c714560f1305/content/browser/background_sync/background_sync_network_observer.cc
[modify] https://crrev.com/562022db7ba4767c4764574b2c01c714560f1305/content/browser/background_sync/background_sync_network_observer.h
[modify] https://crrev.com/562022db7ba4767c4764574b2c01c714560f1305/content/browser/background_sync/background_sync_network_observer_unittest.cc

Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/562022db7ba4767c4764574b2c01c714560f1305

Commit: 562022db7ba4767c4764574b2c01c714560f1305
Author: jkarlin@chromium.org
Commiter: jkarlin@chromium.org
Date: 2018-11-05 13:15:12 +0000 UTC

[BackgroundSync] BGSync missing its initial connection type request

After the switch to NetworkConnectionTracker, the GetConnectionType
call wasn't checking for a synchronous return. In the case of a
synchronous return OnConnectionChanged needs to be called. This led to
BackgroundSyncManager not getting the correct connection type on
startup.

Bug:  900943 
Change-Id: I21ffcacdea9cca4c0780f40561e55b10fb9866b1
Reviewed-on: https://chromium-review.googlesource.com/c/1312676
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#604689}(cherry picked from commit bf943695d0c24fed3f7a1b90566e66b5a8482c36)
Reviewed-on: https://chromium-review.googlesource.com/c/1317829
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#495}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
Status: Fixed (was: Untriaged)

Sign in to add a comment