New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 717325 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

[Media Router] Ensure Media Router and the MR extension states are consistent

Project Member Reported by imch...@chromium.org, May 2 2017

Issue description

Normally, the MR extension uses localStorage to save state (e.g. sink / route queries, message observers) before the underlying event page is suspended. This works most of the time. However, there are some cases where its state may get out of sync with MR:

(1) The extension crashed and lost unpersisted changes.
(2) The extension was updated; temporary data is cleared.
(3) The extension has an unforseen bug which causes temporary data to be
    persisted incorrectly on suspension.

Thus we should have a mechanism to ensure the states are synced up after the MR extension is reconnected to MR.
 
Cc: dbbrooks@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, May 3 2017

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

commit adb75ff0ecd96123cfeab63e9ab2bfc368e29f05
Author: imcheng <imcheng@chromium.org>
Date: Wed May 03 19:20:20 2017

[Media Router] Sync state when MR extension is (re)connected to MR.

The extension might have become out of sync with MediaRouter due to one
of few reasons:
(1) The extension crashed and lost unpersisted changes.
(2) The extension was updated; temporary data is cleared.
(3) The extension has an unforseen bug which causes temporary data to be
    persisted incorrectly on suspension.

The number of calls made for the state sync should be relatively small
(< 10), and quite inexpensive since the extension is already up. Note
that the calls must be (and are) idempotent; the extension will no-op
if the query / observer being registered already exists.

Note that the existing mDNS activation call is also considered part of
state sync.

BUG= 717325 

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

[modify] https://crrev.com/adb75ff0ecd96123cfeab63e9ab2bfc368e29f05/chrome/browser/media/router/mojo/media_router_mojo_impl.cc
[modify] https://crrev.com/adb75ff0ecd96123cfeab63e9ab2bfc368e29f05/chrome/browser/media/router/mojo/media_router_mojo_impl.h
[modify] https://crrev.com/adb75ff0ecd96123cfeab63e9ab2bfc368e29f05/chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc

I just verified this fix in 60.0.3089.0
Labels: Merge-Request-59
Verified the fix with dbbrooks@ on canary. Request merge to 59. 
Project Member

Comment 5 by sheriffbot@chromium.org, May 4 2017

Labels: -Merge-Request-59 Hotlist-Merge-Approved Merge-Approved-59
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 6 by bugdroid1@chromium.org, May 4 2017

Labels: -merge-approved-59 merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee03b79650fe4de5f74e0979ee8e6eec900f2d2b

commit ee03b79650fe4de5f74e0979ee8e6eec900f2d2b
Author: Derek Cheng <imcheng@chromium.org>
Date: Thu May 04 21:30:07 2017

[Media Router] Sync state when MR extension is (re)connected to MR.

The extension might have become out of sync with MediaRouter due to one
of few reasons:
(1) The extension crashed and lost unpersisted changes.
(2) The extension was updated; temporary data is cleared.
(3) The extension has an unforseen bug which causes temporary data to be
    persisted incorrectly on suspension.

The number of calls made for the state sync should be relatively small
(< 10), and quite inexpensive since the extension is already up. Note
that the calls must be (and are) idempotent; the extension will no-op
if the query / observer being registered already exists.

Note that the existing mDNS activation call is also considered part of
state sync.

BUG= 717325 

Review-Url: https://codereview.chromium.org/2855473004
Cr-Commit-Position: refs/heads/master@{#469067}
(cherry picked from commit adb75ff0ecd96123cfeab63e9ab2bfc368e29f05)

Review-Url: https://codereview.chromium.org/2857393005 .
Cr-Commit-Position: refs/branch-heads/3071@{#407}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/ee03b79650fe4de5f74e0979ee8e6eec900f2d2b/chrome/browser/media/router/mojo/media_router_mojo_impl.cc
[modify] https://crrev.com/ee03b79650fe4de5f74e0979ee8e6eec900f2d2b/chrome/browser/media/router/mojo/media_router_mojo_impl.h
[modify] https://crrev.com/ee03b79650fe4de5f74e0979ee8e6eec900f2d2b/chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment