Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14862004:
Fix null pointer bug, add better logging to see why pointer is sometimes null. (Closed)
|
Created:
7 years, 7 months ago by Pete Williamson Modified:
7 years, 7 months ago CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org, Andrew T Wilson (Slow) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFix crash in Synced Notifications.
Synced notifications tried to access a null pointer, make
sure we don't crash when we do.
Also add logging so we can determine how the code got into a
bad state.
BUG=235979
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198801
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fix null pointer bug, CR fixes per DCheng #Patch Set 3 : Synced Notifications null pointer bug fix. #Messages
Total messages: 12 (0 generated)
Drew: Please review the change to Background_mode_manager.h Mike: Please review the change to chrome_notifier_service.cc Daniel: Please review the change to push_messaging_api.h
Please make the CL description more specific for what's being fixed where, e.g.: "Fix crash in ChromeNotifierService when it handles bad sync data. CreateNotificationFromSyncData can return NULL when it considers the sync data to be malformed, so make sure it's non-null before dereferencing. Also added some logging statements to try to figure out why the sync data is malformed." I'd probably just skip the minor cleanup changes in the other files; just apply the cleanups when there are other real changes to those files. https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:58: if (!incoming.get()) { if (!incoming) https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:59: NOTREACHED() << "Badly formed sync data in incoming notification"; Given that this is actually happening, NOTREACHED() may not be the best check here. https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:194: DVLOG(1) << "Synced Notification missing mandatory fields"; You may want to consider logging the results of the various checks in the if statement above.
Subject to dcheng's comments, chrome_notifier_service.cc LGTM.
CR fixes and answers per DCheng. I removed Drew as a reviewer since I no longer need him to review the comment changes in background_mode_maanger.h https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:58: if (!incoming.get()) { On 2013/05/03 17:01:02, dcheng wrote: > if (!incoming) Done. https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:59: NOTREACHED() << "Badly formed sync data in incoming notification"; On 2013/05/03 17:01:02, dcheng wrote: > Given that this is actually happening, NOTREACHED() may not be the best check > here. 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. https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:194: DVLOG(1) << "Synced Notification missing mandatory fields"; On 2013/05/03 17:01:02, dcheng wrote: > You may want to consider logging the results of the various checks in the if > statement above. Done, and also done for the next DVLOG statement.
LGTM once all comments are addressed. https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:59: NOTREACHED() << "Badly formed sync data in incoming notification"; On 2013/05/03 18:09:53, Pete Williamson wrote: > On 2013/05/03 17:01:02, dcheng wrote: > > Given that this is actually happening, NOTREACHED() may not be the best check > > here. > 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. 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().
On 2013/05/03 18:13:32, dcheng wrote: > LGTM once all comments are addressed. > > https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... > File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc > (right): > > https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... > chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:59: > NOTREACHED() << "Badly formed sync data in incoming notification"; > On 2013/05/03 18:09:53, Pete Williamson wrote: > > On 2013/05/03 17:01:02, dcheng wrote: > > > Given that this is actually happening, NOTREACHED() may not be the best > check > > > here. > > 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. > > 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(). Also, followup comment that you should try to cap CL descriptions at about 72 characters per line. In general, git expects something like this: Short summary that is no longer than 72 characters. Longer description of what went into the change, each line no longer than 72 characters so git revert doesn't mangle it too much.
CR fix for DCheng https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... File chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc (right): https://codereview.chromium.org/14862004/diff/1/chrome/browser/notifications/... chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc:59: NOTREACHED() << "Badly formed sync data in incoming notification"; On 2013/05/03 18:13:32, dcheng wrote: > On 2013/05/03 18:09:53, Pete Williamson wrote: > > On 2013/05/03 17:01:02, dcheng wrote: > > > Given that this is actually happening, NOTREACHED() may not be the best > check > > > here. > > 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. > > 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(). OK. using LOG(WARNING) now,
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petewil@chromium.org/14862004/10001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petewil@chromium.org/14862004/10001
Message was sent while issue was closed.
Change committed as 198801 |
