Fill empty fields from keyring result with default values.

Fill empty fields from keyring result with default values.

This is fixing of crashes in libsecret when some of fields from keyring result are absent.

BUG=453218
Committed: https://crrev.com/480ca0e54a373befb27813fd9e1276e4627c30de
Cr-Commit-Position: refs/heads/master@{#313710}

[dvadym, 2015-01-29 13:46:39]

dvadym@chromium.org changed reviewers:
+ vabr@chromium.org

[dvadym, 2015-01-29 13:46:40]

Hi Vaclav,

 Could you please review this CL?

Best regards,
Vadym

[vabr (Chromium), 2015-01-29 14:10:34]

Hi Vadym,

Looks reasonable, but I'm concerned about performance. Happy to have a chat
about this, if you want.

Cheers,
Vaclav

https://codereview.chromium.org/891463002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_libsecret.cc (right):

https://codereview.chromium.org/891463002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:122: std::string
GetStringFromAttributes(GHashTable* attrs, const char* keyname) {
I would suggest still returning char* and carefully checking for null on the
callsites instead. String copying harms performance, and should be avoided.

https://codereview.chromium.org/891463002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:129: if (value ==
NULL)
nit: NULL -> nullptr

https://codereview.chromium.org/891463002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:141: std::string
app_string = GetStringFromAttributes(attrs, "application");
Please do not convert to std::string, that harms performance. Instead, check if
the string piece is null in the if condition below, and return null then.

[dvadym, 2015-01-29 14:21:40]

Thanks Vaclav, fixed please have a look at PatchSet 3.

https://codereview.chromium.org/891463002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_libsecret.cc (right):

https://codereview.chromium.org/891463002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:122: std::string
GetStringFromAttributes(GHashTable* attrs, const char* keyname) {
On 2015/01/29 14:10:34, vabr (Chromium) wrote:
> I would suggest still returning char* and carefully checking for null on the
> callsites instead. String copying harms performance, and should be avoided.

Done.

https://codereview.chromium.org/891463002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:129: if (value ==
NULL)
On 2015/01/29 14:10:34, vabr (Chromium) wrote:
> nit: NULL -> nullptr

Done.

https://codereview.chromium.org/891463002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:141: std::string
app_string = GetStringFromAttributes(attrs, "application");
On 2015/01/29 14:10:34, vabr (Chromium) wrote:
> Please do not convert to std::string, that harms performance. Instead, check
if
> the string piece is null in the if condition below, and return null then.

Done.

[vabr (Chromium), 2015-01-29 14:23:13]

LGTM, with one comment.

Cheers,
Vaclav

https://codereview.chromium.org/891463002/diff/40001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_libsecret.cc (right):

https://codereview.chromium.org/891463002/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:23: char
empty_string[] = "";
const char kEmptyString[] = "";

[dvadym, 2015-01-29 14:27:57]

Thanks a lot, fixed.

https://codereview.chromium.org/891463002/diff/40001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_libsecret.cc (right):

https://codereview.chromium.org/891463002/diff/40001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:23: char
empty_string[] = "";
On 2015/01/29 14:23:13, vabr (Chromium) wrote:
> const char kEmptyString[] = "";

Done.

[dvadym, 2015-01-29 14:28:20]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2015-01-29 14:28:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/891463002/60001

[dvadym, 2015-01-29 14:29:46]

The CQ bit was unchecked by dvadym@chromium.org

[dvadym, 2015-01-29 14:33:42]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2015-01-29 14:34:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/891463002/100001

[vabr (Chromium), 2015-01-29 14:49:11]

Patch set 6 (https://codereview.chromium.org/891463002/#ps100001) LGTM.
Vaclav

[commit-bot: I haz the power, 2015-01-29 15:35:57]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-29 15:37:06]

Patchset 6 (id:??) landed as
https://crrev.com/480ca0e54a373befb27813fd9e1276e4627c30de
Cr-Commit-Position: refs/heads/master@{#313710}