Fix null pointer bug, add better logging to see why pointer is sometimes null.

chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // The ChromeNotifierService works together with sync to maintain the state of
 // user notifications, which can then be presented in the notification center,
 // via the Notification UI Manager.
 
 #include "chrome/browser/notifications/sync_notifier/chrome_notifier_service.h"
 
@@ skipping 37 matching lines @@
   sync_processor_ = sync_processor.Pass();
 
   for (syncer::SyncDataList::const_iterator it = initial_sync_data.begin();
        it != initial_sync_data.end(); ++it) {
     const syncer::SyncData& sync_data = *it;
     DCHECK_EQ(syncer::SYNCED_NOTIFICATIONS, sync_data.GetDataType());
 
     // Build a local notification object from the sync data.
     scoped_ptr<SyncedNotification> incoming(CreateNotificationFromSyncData(
         sync_data));
-    DCHECK(incoming.get());
+    if (!incoming.get()) {

[dcheng, 2013/05/03 17:01:02]

if (!incoming)

[Pete Williamson, 2013/05/03 18:09:53]

Done.

+      NOTREACHED() << "Badly formed sync data in incoming notification";

[dcheng, 2013/05/03 17:01:02]

Given that this is actually happening, NOTREACHED() may not be the best check
here.

[Pete Williamson, 2013/05/03 18:09:53]

 It shouldn't ever happen. I plan to fix the rest of the code to prevent this,
so I think that NOTREACHED() is a better statement of the intent of the code.

[dcheng, 2013/05/03 18:13:32]

I agree that's the eventual intent, but that's not what happens today and will
break the debug build for stevenjb@ (and others who encounter this issue).

Once the root cause is understood and addressed, then I think it's fine to turn
it into NOTREACHED().

[Pete Williamson, 2013/05/03 18:30:20]

OK. using LOG(WARNING) now, 

+      continue;
+    }
 
     // Process each incoming remote notification.
     const std::string& key = incoming->GetKey();
     DCHECK_GT(key.length(), 0U);
     SyncedNotification* found = FindNotificationByKey(key);
 
     if (NULL == found) {
       // If there are no conflicts, copy in the data from remote.
       Add(incoming.Pass());
     } else {
@@ skipping 111 matching lines @@
 scoped_ptr<SyncedNotification>
     ChromeNotifierService::CreateNotificationFromSyncData(
         const syncer::SyncData& sync_data) {
   // Get a pointer to our data within the sync_data object.
   sync_pb::SyncedNotificationSpecifics specifics =
       sync_data.GetSpecifics().synced_notification();
 
   // Check for mandatory fields in the sync_data object.
   if (!specifics.has_coalesced_notification() ||
       !specifics.coalesced_notification().has_key() ||
-      !specifics.coalesced_notification().has_read_state())
+      !specifics.coalesced_notification().has_read_state()) {
+    DVLOG(1) << "Synced Notification missing mandatory fields";

[dcheng, 2013/05/03 17:01:02]

You may want to consider logging the results of the various checks in the if
statement above.

[Pete Williamson, 2013/05/03 18:09:53]

Done, and also done for the next DVLOG statement.

     return scoped_ptr<SyncedNotification>();
+  }
 
   // TODO(petewil): Is this the right set?  Should I add more?
   bool is_well_formed_unread_notification =
       (static_cast<SyncedNotification::ReadState>(
           specifics.coalesced_notification().read_state()) ==
        SyncedNotification::kUnread &&
        specifics.coalesced_notification().has_render_info());
   bool is_well_formed_dismissed_notification =
       (static_cast<SyncedNotification::ReadState>(
           specifics.coalesced_notification().read_state()) ==
        SyncedNotification::kDismissed);
 
   // if the notification is poorly formed, return a null pointer
   if (!is_well_formed_unread_notification &&
-      !is_well_formed_dismissed_notification)
+      !is_well_formed_dismissed_notification) {
+    DVLOG(1) << "Synced Notification is not well formed.";
     return scoped_ptr<SyncedNotification>();
+  }
 
   // Create a new notification object based on the supplied sync_data.
   scoped_ptr<SyncedNotification> notification(
       new SyncedNotification(sync_data));
 
   return notification.Pass();
 }
 
 // This returns a pointer into a vector that we own.  Caller must not free it.
 // Returns NULL if no match is found.
@@ skipping 37 matching lines @@
 void ChromeNotifierService::Add(scoped_ptr<SyncedNotification> notification) {
   SyncedNotification* notification_copy = notification.get();
   // Take ownership of the object and put it into our local storage.
   notification_data_.push_back(notification.release());
 
   // Get the contained bitmaps, and show the notification once we have them.
   notification_copy->Show(notification_manager_, this, profile_);
 }
 
 }  // namespace notifier