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

chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc

Index: chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
diff --git a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
index c7bc0b00fd40443b7f5d68dd1faac3842706eb94..f3c59b0cea21eca751acd58782e4ff398e0b179e 100644
--- a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
+++ b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
@@ -55,7 +55,10 @@ syncer::SyncMergeResult ChromeNotifierService::MergeDataAndStartSyncing(
     // 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();
@@ -187,8 +190,10 @@ scoped_ptr<SyncedNotification>
   // 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 =
@@ -203,8 +208,10 @@ scoped_ptr<SyncedNotification>
 
   // 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(