1651764 - Use of uninitialized variable in Database::EnsureBookmarkRoots()?

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Alexey Izbyshev]

If https://searchfox.org/mozilla-central/rev/8d55e188/toolkit/components/places/Database.cpp#1449 is reachable without visiting any of the prior if statements, it reads uninitialized rv. In any case, this check is useless (rv is checked after each assignment in the prior ifs).

Comment 1

[Mark Banner (:standard8)]

Yes, that line is wrong - it shouldn't & doesn't need to be there. Alexey, do you want to make a patch to remove that one line?

Comment 2

[Alexey Izbyshev]

I don't mind making a patch, but that would be my first patch here, so I don't have any setup apart from a build environment for Firefox that I needed to run Svace on it. Should I follow this guide (including creation of Phabricator account, etc.), or are there any shortcuts for trivial patches?

Comment 3

[Mark Banner (:standard8)]

(In reply to Alexey Izbyshev from comment #2)

I don't mind making a patch, but that would be my first patch here, so I don't have any setup apart from a build environment for Firefox that I needed to run Svace on it. Should I follow this guide (including creation of Phabricator account, etc.), or are there any shortcuts for trivial patches?

We're easy either way, I'm quite happy to mentor this bug. This is probably the more up to date guide to look at: https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html

Comment 4

[Alexey Izbyshev]

Attachment: Bug 1651764 - Remove unneeded 'rv' check in Database::EnsureBookmarkRoots. r?standard8

Apart from being unneeded, this check reads uninitialized 'rv' in case
no prior 'if' statement is visited.

Comment 5

[Alexey Izbyshev]

(In reply to Mark Banner (:standard8) from comment #3)

We're easy either way, I'm quite happy to mentor this bug. This is probably the more up to date guide to look at: https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html

Thanks for the link! I've submitted the patch (with you as the requested reviewer). I've run ./mach build locally, but haven't run any tests.

Comment 6

[Mark Banner (:standard8)]

Thank you for the patch, I've just triggered it to land - it will land on our integration branch first and then will be merged to the main branch within about 24 hours. There will be comments here when that happens.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92e126b6a9b7
Remove unneeded 'rv' check in Database::EnsureBookmarkRoots. r=Standard8

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/92e126b6a9b7