Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
190723002:
Fixed null pointer crash in ExtensionSettingsHandler. (Closed)
|
Created:
6 years, 9 months ago by vasilii Modified:
6 years, 9 months ago Reviewers:
Finnur CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFixed null pointer crash in ExtensionSettingsHandler.
BUG=349838
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255923
Patch Set 1 #
Total comments: 5
Patch Set 2 : Changed the message #Messages
Total messages: 14 (0 generated)
Hi Finnur, The bug has been existing for years. I use the same pattern as in other methods like HandleEnableIncognitoMessage(). Please review.
https://codereview.chromium.org/190723002/diff/1/chrome/browser/ui/webui/exte... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/190723002/diff/1/chrome/browser/ui/webui/exte... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:853: } Wow... this gave me a serious sense of deja-vu! And rightly so, because I fixed this exact bug in another class, but didn't realize it existed here also. https://codereview.chromium.org/23496030/ However, I would prefer if you fixed it in the same manner as I did back then... (see CL).
https://codereview.chromium.org/190723002/diff/1/chrome/browser/ui/webui/exte... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/190723002/diff/1/chrome/browser/ui/webui/exte... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:853: } On 2014/03/07 12:55:53, Finnur wrote: > Wow... this gave me a serious sense of deja-vu! And rightly so, because I fixed > this exact bug in another class, but didn't realize it existed here also. > https://codereview.chromium.org/23496030/ > > However, I would prefer if you fixed it in the same manner as I did back then... > (see CL). That means the log message is wrong. What should I do with other places in this file which simply return?
https://codereview.chromium.org/190723002/diff/1/chrome/browser/ui/webui/exte... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/190723002/diff/1/chrome/browser/ui/webui/exte... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:853: } If you are sure that we should not output this LOG message when |extension| is NULL (which I think is likely, but wasn't sure how the ManagementPolicy stuff worked), then your fix is better and the other file needs to change as well. But if not, I'd like to evaluate "these other places" you refer to. Also, the wording of the LOG message is bothering me a bit. Can you change it to say: "An attempt was made to enable an extension that is non-usermanagable. Extension id: " << extension->id();
https://codereview.chromium.org/190723002/diff/1/chrome/browser/ui/webui/exte... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/190723002/diff/1/chrome/browser/ui/webui/exte... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:853: } On 2014/03/07 13:44:03, Finnur wrote: > If you are sure that we should not output this LOG message when |extension| is > NULL (which I think is likely, but wasn't sure how the ManagementPolicy stuff > worked), then your fix is better and the other file needs to change as well. > > But if not, I'd like to evaluate "these other places" you refer to. > > Also, the wording of the LOG message is bothering me a bit. Can you change it to > say: > "An attempt was made to enable an extension that is non-usermanagable. Extension > id: " << extension->id(); We definitely shouldn't output this message. The bug I'm fixing is the UI bug. A user clicks "delete extension" and shortly something else. Thus, Chrome tries to perform some action on the uninstalled extension. The question is if we want to LOG() something else in this scenario?
https://codereview.chromium.org/190723002/diff/1/chrome/browser/ui/webui/exte... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/190723002/diff/1/chrome/browser/ui/webui/exte... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:853: } Nah, probably not. We return early for NULL extensions in a lot of places in this file and we don't LOG that. But I'd make the same changes in the other file. And with that, LGTM.
It's already fixed in https://codereview.chromium.org/54903011
The CQ bit was checked by vasilii@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vasilii@chromium.org/190723002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by vasilii@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vasilii@chromium.org/190723002/20001
Message was sent while issue was closed.
Change committed as 255923 |
