Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(95)

Issue 11187017: Use RejectAuthenticatingClient instead of DisconnectAllClients to reject incoming clients on curtai… (Closed)

Created:
8 years, 2 months ago by rmsousa
Modified:
8 years, 2 months ago
Reviewers:
Jamie
CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Use RejectAuthenticatingClient instead of DisconnectAllClients to reject incoming clients on curtain mode activation errors. If we DisconnectAllClients from within the CurtainingHostObserver's OnClientAuthenticated callback, it will ultimately cause OnClientDisconnected callbacks for all HostStatusObservers to run *immediately*, before the OnClientAuthenticated for the next observer in the queue are called. This can cause the OnClientDisconnected callback for that observer to run *before* its OnClientAuthenticated callback. For example, if HostUserInterface is enabled, a remote client tries to connect to the console, and curtaining fails, this will cause the local disconnect window to stay up after the client is rejected (before the Hide() call for the disconnection runs before Show()). BUG=156254 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162376

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -1 line) Patch
M remoting/host/remoting_me2me_host.cc View 2 chunks +7 lines, -1 line 1 comment Download

Messages

Total messages: 5 (0 generated)
rmsousa
It turned out that we need the different callbacks for the curtain mode after all... ...
8 years, 2 months ago (2012-10-17 01:03:55 UTC) #1
Jamie
Yikes! Thanks for spotting this one. LGTM.
8 years, 2 months ago (2012-10-17 01:05:41 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmsousa@chromium.org/11187017/1
8 years, 2 months ago (2012-10-17 01:59:38 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmsousa@chromium.org/11187017/1
8 years, 2 months ago (2012-10-17 11:18:05 UTC) #4
commit-bot: I haz the power
8 years, 2 months ago (2012-10-17 13:13:46 UTC) #5
Change committed as 162376

Powered by Google App Engine
This is Rietveld 408576698