Reland of https://codereview.chromium.org/13891016/. This includes the fix for the Segfault on Chro…

Reland of https://codereview.chromium.org/13891016/. This includes the fix for the Segfault on ChromeOS.

Make DriveSystemService an observer of DriveNotificationManager so that it no longer has to handle registering for Drive XMPP notifications itself. This will also allow both SyncFileSystem and the ChromeOS Filemanager (and possibly more observers) to use XMPP notifications solving the problem from before where the same XMPP Invalidation ID was being registered twice.

Also removed polling code from SyncFS it's OKed to remove by both SyncFS and ChromeOS teams.

BUG=173339
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196625

[calvinlo, 2013-04-24 06:16:05]

Sorry for the previous problems with this patch. After local testing, I think
the problem with the previous patch is that I tried to store the pointer for
ProfileSyncService in the DriveNotificationManager constructor and reuse it
later rather than trying to get it again using     
ProfileSyncServiceFactory::GetForProfile(profile_);

In testing, it seems like the address of ProfileSyncService can be invalid
(something like 0xabababababab but not null as to pass the CHECK) when the
DriveNotificationManager constructor runs but then change to a valid address
when OnIncomingValidation() is called. The fix then I think is to just always
call ProfileSyncServiceFactory::GetForProfile() and never store the
ProfileSyncService as a member variable.

I believe this only happened on ChromeOS because it's possible to start the
FileManager in ChromeOS while not being signed in and then signing in later
changes the return of ProfileSyncServiceFactory::GetForProfile();

I also uploaded the patch to the previous review
https://codereview.chromium.org/13891016/#ps11008 so you can see the delta
between the latest patch version and the previous version which was reverted.

[tzik, 2013-04-24 12:01:23]

On 2013/04/24 06:16:05, calvinlo wrote:
> Sorry for the previous problems with this patch. After local testing, I think
> the problem with the previous patch is that I tried to store the pointer for
> ProfileSyncService in the DriveNotificationManager constructor and reuse it
> later rather than trying to get it again using     
> ProfileSyncServiceFactory::GetForProfile(profile_);
> 
> In testing, it seems like the address of ProfileSyncService can be invalid
> (something like 0xabababababab but not null as to pass the CHECK) when the
> DriveNotificationManager constructor runs but then change to a valid address
> when OnIncomingValidation() is called. The fix then I think is to just always
> call ProfileSyncServiceFactory::GetForProfile() and never store the
> ProfileSyncService as a member variable.
> 
> I believe this only happened on ChromeOS because it's possible to start the
> FileManager in ChromeOS while not being signed in and then signing in later
> changes the return of ProfileSyncServiceFactory::GetForProfile();
> 
> I also uploaded the patch to the previous review
> https://codereview.chromium.org/13891016/#ps11008 so you can see the delta
> between the latest patch version and the previous version which was reverted.

The change itself looks good.
In general for relanding change, please upload rebased previous version as the
first patchset, and upload fixed version as the following patchset.
It's hard to see their diff in review site.

[satorux1, 2013-04-25 01:04:04]

On 2013/04/24 12:01:23, tzik wrote:
> On 2013/04/24 06:16:05, calvinlo wrote:
> > Sorry for the previous problems with this patch. After local testing, I
think
> > the problem with the previous patch is that I tried to store the pointer for
> > ProfileSyncService in the DriveNotificationManager constructor and reuse it
> > later rather than trying to get it again using     
> > ProfileSyncServiceFactory::GetForProfile(profile_);
> > 
> > In testing, it seems like the address of ProfileSyncService can be invalid
> > (something like 0xabababababab but not null as to pass the CHECK) when the
> > DriveNotificationManager constructor runs but then change to a valid address
> > when OnIncomingValidation() is called. The fix then I think is to just
always
> > call ProfileSyncServiceFactory::GetForProfile() and never store the
> > ProfileSyncService as a member variable.
> > 
> > I believe this only happened on ChromeOS because it's possible to start the
> > FileManager in ChromeOS while not being signed in and then signing in later
> > changes the return of ProfileSyncServiceFactory::GetForProfile();
> > 
> > I also uploaded the patch to the previous review
> > https://codereview.chromium.org/13891016/#ps11008 so you can see the delta
> > between the latest patch version and the previous version which was
reverted.
> 
> The change itself looks good.
> In general for relanding change, please upload rebased previous version as the
> first patchset, and upload fixed version as the following patchset.
> It's hard to see their diff in review site.

hmm, that's also what I suggested:
https://codereview.chromium.org/13891016/#msg27

could you follow the instruction?

[calvinlo, 2013-04-25 03:59:09]

Sorry, I made a mistake in reading step 1 of the instructions. I think I've
followed 1-3 correctly now, you should now be able to see the diff from this new
review URL and I've noted the reland and original review URL in the description.
Thanks.

[tzik, 2013-04-25 05:06:47]

lgtm

[satorux1, 2013-04-25 05:17:23]

lgtm

[commit-bot: I haz the power, 2013-04-25 05:32:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/13976019/6011

[commit-bot: I haz the power, 2013-04-25 05:32:47]

Failed to apply patch for chrome/browser/chromeos/drive/drive_system_service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/drive/drive_system_service.cc
  Hunk #1 FAILED at 24.
  Hunk #2 succeeded at 47 with fuzz 2 (offset -2 lines).
  Hunk #3 succeeded at 101 (offset -2 lines).
  Hunk #4 succeeded at 168 (offset -4 lines).
  Hunk #5 succeeded at 186 (offset -4 lines).
  Hunk #6 succeeded at 203 (offset -4 lines).
  Hunk #7 succeeded at 258 (offset -4 lines).
  Hunk #8 succeeded at 337 (offset -4 lines).
  Hunk #9 succeeded at 409 (offset -4 lines).
  1 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/drive/drive_system_service.cc.rej

Patch:       chrome/browser/chromeos/drive/drive_system_service.cc
Index: chrome/browser/chromeos/drive/drive_system_service.cc
diff --git a/chrome/browser/chromeos/drive/drive_system_service.cc
b/chrome/browser/chromeos/drive/drive_system_service.cc
index
46dbf9c240cf7b9ee9028f177140e9b891039ee8..2926381bee175863a1e96fd5ab3576edf50e5ccb
100644
--- a/chrome/browser/chromeos/drive/drive_system_service.cc
+++ b/chrome/browser/chromeos/drive/drive_system_service.cc
@@ -24,12 +24,13 @@
 #include "chrome/browser/google_apis/auth_service.h"
 #include "chrome/browser/google_apis/drive_api_service.h"
 #include "chrome/browser/google_apis/drive_api_util.h"
+#include "chrome/browser/google_apis/drive_notification_manager.h"
+#include "chrome/browser/google_apis/drive_notification_manager_factory.h"
 #include "chrome/browser/google_apis/event_logger.h"
 #include "chrome/browser/google_apis/gdata_wapi_service.h"
 #include "chrome/browser/google_apis/gdata_wapi_url_generator.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_dependency_manager.h"
-#include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
@@ -49,9 +50,6 @@ namespace {
 
 static const size_t kEventLogHistorySize = 100;
 
-// The sync invalidation object ID for Google Drive.
-const char kDriveInvalidationObjectId[] = "CHANGELOG";
-
 // Returns true if Drive is enabled for the given Profile.
 bool IsDriveEnabledForProfile(Profile* profile) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -106,7 +104,6 @@ DriveSystemService::DriveSystemService(
     DriveFileSystemInterface* test_file_system)
     : profile_(profile),
       drive_disabled_(false),
-      push_notification_registered_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   base::SequencedWorkerPool* blocking_pool = BrowserThread::GetBlockingPool();
@@ -176,19 +173,10 @@ void DriveSystemService::Initialize() {
 void DriveSystemService::Shutdown() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  ProfileSyncService* profile_sync_service =
-      profile_ ? ProfileSyncServiceFactory::GetForProfile(profile_) : NULL;
-  if (profile_sync_service && push_notification_registered_) {
-    // TODO(kochi): Once DriveSystemService gets started / stopped at runtime,
-    // this ID needs to be unregistered *before* the handler is unregistered
-    // as ID persists across browser restarts.
-    if (!IsDriveEnabledForProfile(profile_)) {
-      profile_sync_service->UpdateRegisteredInvalidationIds(
-          this, syncer::ObjectIdSet());
-    }
-    profile_sync_service->UnregisterInvalidationHandler(this);
-    push_notification_registered_ = false;
-  }
+  google_apis::DriveNotificationManager* drive_notification_manager =
+      google_apis::DriveNotificationManagerFactory::GetForProfile(profile_);
+  if (drive_notification_manager)
+    drive_notification_manager->RemoveObserver(this);
 
   RemoveDriveMountPoint();
 }
@@ -203,6 +191,10 @@ void
DriveSystemService::RemoveObserver(DriveSystemServiceObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void DriveSystemService::OnNotificationReceived() {
+  file_system_->CheckForUpdates();
+}
+
 bool DriveSystemService::IsDriveEnabled() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -216,22 +208,6 @@ bool DriveSystemService::IsDriveEnabled() {
   return true;
 }
 
-void DriveSystemService::OnInvalidatorStateChange(
-    syncer::InvalidatorState state) {
-  DVLOG(1) << "InvalidatorState changed to " << state;
-}
-
-void DriveSystemService::OnIncomingInvalidation(
-    const syncer::ObjectIdInvalidationMap& invalidation_map) {
-  DCHECK_EQ(1U, invalidation_map.size());
-  const invalidation::ObjectId object_id(
-      ipc::invalidation::ObjectSource::COSMO_CHANGELOG,
-      kDriveInvalidationObjectId);
-  DCHECK_EQ(1U, invalidation_map.count(object_id));
-
-  file_system_->CheckForUpdates();
-}
-
 void DriveSystemService::ClearCacheAndRemountFileSystem(
     const base::Callback<void(bool)>& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -287,19 +263,6 @@ void DriveSystemService::ReloadAndRemountFileSystem() {
   AddDriveMountPoint();
 }
 
-bool DriveSystemService::PushNotificationEnabled() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(profile_);
-
-  ProfileSyncService* profile_sync_service =
-      ProfileSyncServiceFactory::GetForProfile(profile_);
-  if (!profile_sync_service)
-    return false;
-
-  return (profile_sync_service->GetInvalidatorState() ==
-          syncer::INVALIDATIONS_ENABLED);
-}
-
 void DriveSystemService::AddDriveMountPoint() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!file_system_proxy_.get());
@@ -379,18 +342,10 @@ void
DriveSystemService::InitializeAfterResourceMetadataInitialized(
       cache_->GetCacheDirectoryPath(DriveCache::CACHE_TYPE_TMP_DOWNLOADS));
 
   // Register for Google Drive invalidation notifications.
-  ProfileSyncService* profile_sync_service =
-      profile_ ? ProfileSyncServiceFactory::GetForProfile(profile_) : NULL;
-  if (profile_sync_service) {
-    DCHECK(!push_notification_registered_);
-    profile_sync_service->RegisterInvalidationHandler(this);
-    syncer::ObjectIdSet ids;
-    ids.insert(invalidation::ObjectId(
-        ipc::invalidation::ObjectSource::COSMO_CHANGELOG,
-        kDriveInvalidationObjectId));
-    profile_sync_service->UpdateRegisteredInvalidationIds(this, ids);
-    push_notification_registered_ = true;
-  }
+  google_apis::DriveNotificationManager* drive_notification_manager =
+      google_apis::DriveNotificationManagerFactory::GetForProfile(profile_);
+  if (drive_notification_manager)
+    drive_notification_manager->AddObserver(this);
 
   AddDriveMountPoint();
 }
@@ -459,7 +414,7 @@ void DriveSystemServiceFactory::SetFactoryForTest(
 DriveSystemServiceFactory::DriveSystemServiceFactory()
     : ProfileKeyedServiceFactory("DriveSystemService",
                                  ProfileDependencyManager::GetInstance()) {
-  DependsOn(ProfileSyncServiceFactory::GetInstance());
+  DependsOn(google_apis::DriveNotificationManagerFactory::GetInstance());
   DependsOn(DownloadServiceFactory::GetInstance());
 }

[commit-bot: I haz the power, 2013-04-25 07:17:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/13976019/6012

[commit-bot: I haz the power, 2013-04-25 22:03:28]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[satorux1, 2013-04-26 04:07:52]

maybe dcommit is the way to go?

[calvinlo, 2013-04-26 04:09:36]

Yeah but there's another conflict now on rebase. Must resolve that first. Will
dcommit after.

[calvinlo, 2013-04-26 05:16:57]

Committed patchset #6 manually as r196625 (presubmit successful).