Closed Bug 659234 Opened 9 years ago Closed 8 years ago

Remove unused variable |rv| from test_file_perms.cpp:52

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: dholbert, Assigned: emorley)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [build_warning][inbound])

Attachments

(1 file, 3 obsolete files)

New build warning introduced recently in
  http://hg.mozilla.org/mozilla-central/rev/6105fb36f613
{
mozilla/storage/test/test_file_perms.cpp: In function ‘void test_file_perms()’:
mozilla/storage/test/test_file_perms.cpp:64:44: warning: suggest parentheses around comparison in operand of ‘|’
mozilla/storage/test/test_file_perms.cpp:52:12: warning: unused variable ‘rv’
}

We probably don't care about |rv| and can just get rid of it...?
(and might as well add the suggested parentheses while we're at it)
Assignee: nobody → bmo
Blocks: buildwarning
Status: NEW → ASSIGNED
Attached patch Fix build warnings (obsolete) — Splinter Review
Attachment #540356 - Flags: review?(sdwilsh)
Attached patch Fix build warnings v1.1 (obsolete) — Splinter Review
The previous patch failed try: http://tinderbox.mozilla.org/showlog.cgi?log=Try/1308556811.1308559722.5901.gz#err0

TEST-UNEXPECTED-FAIL | /builds/slave/try-lnx64-dbg/build/storage/test/test_file_perms.cpp | Expected true, got false at line 63

Due to this change:
-  do_check_true(perms == PR_IRUSR | PR_IWUSR | PR_IRGRP | PR_IWGRP | PR_IROTH | PR_IWOTH);
+  do_check_true(perms == (PR_IRUSR | PR_IWUSR | PR_IRGRP | PR_IWGRP | PR_IROTH | PR_IWOTH));

Whereas for the logic to remain the same it operator precedence says it should have been:
+  do_check_true( (perms == PR_IRUSR) | PR_IWUSR | PR_IRGRP | PR_IWGRP | PR_IROTH | PR_IWOTH );

I've updated the patch to add the parenthesis but still keep the logic the same as before - however, was the existing code correct? Only comparing PR_IRUSR to perms and not the others doesn't make sense, unless I'm missing something? This file was created in bug 650509, which is still hidden, so not sure what the intention was.
Attachment #540356 - Attachment is obsolete: true
Attachment #540356 - Flags: review?(sdwilsh)
Attachment #540431 - Flags: review?(sdwilsh)
Ping for review / comment 3 question. Thanks :-)
(In reply to comment #3)
> Whereas for the logic to remain the same it operator precedence says it
> should have been:
> +  do_check_true( (perms == PR_IRUSR) | PR_IWUSR | PR_IRGRP | PR_IWGRP |
> PR_IROTH | PR_IWOTH );

I agree with you that that looks busted -- I seriously doubt that's the intended behavior.

> This file was created in bug 650509, which is still hidden, so
> not sure what the intention was.

(I just glanced at the bug -- it doesn't have any more information about this test.)

blassey, mind taking a look at this?  It looks to me like there's a bug in the code and/or the test (in which case bug 650509 may not actually be fixed, or at least not tested sufficiently...)
Comment on attachment 540431 [details] [diff] [review]
Fix build warnings v1.1

(If I'm reading correctly, that do_check_true statement is trivially always true.)

Requesting feedback?blassey on the latest (broken-behavior-preserving) patch, to be sure this is on his radar.
Attachment #540431 - Flags: feedback?(blassey.bugs)
Comment on attachment 540431 [details] [diff] [review]
Fix build warnings v1.1

I concur with previous comments: this patch has to be wrong.
Attachment #540431 - Flags: feedback-
Comment on attachment 540356 [details] [diff] [review]
Fix build warnings

Review of attachment 540356 [details] [diff] [review]:
-----------------------------------------------------------------

http://mxr.mozilla.org/mozilla-central/search?string=SQLITE_DEFAULT_FILE_PERMISSIONS&case=on

Iiuc, rights are 0644, except on Android where they are 0600.
To be explicit, this could be added as a comment.

::: storage/test/test_file_perms.cpp
@@ +57,5 @@
>  
>    // This reflexts the permissions defined by SQLITE_DEFAULT_FILE_PERMISSIONS in
>    // db/sqlite3/src/Makefile.in and must be kept in sync with that
>  #ifdef ANDROID
> +  do_check_true(perms == (PR_IRUSR | PR_IWUSR));

(This seems just right to me.)

@@ +62,2 @@
>  #else
> +  do_check_true(perms == (PR_IRUSR | PR_IWUSR | PR_IRGRP | PR_IWGRP | PR_IROTH | PR_IWOTH));

Parens are right, but PR_IWGRP and PR_IWOTH should be removed.
Comment on attachment 540431 [details] [diff] [review]
Fix build warnings v1.1

the problem here (or at least one of them) is that the places.sqlite file doesn't exist in the test profile for unit tests.

The point of this test is to ensure that the permissions of the files containing sensitive data are not too permissive on Android. Its obviously completely busted on other platforms and was only passing due to the incorrect parenthesis.

Does anyone know how to make the unit tests generate a places.sqlite file so it can be tested?
Attachment #540431 - Flags: feedback?(blassey.bugs) → feedback-
(I'd have never seen this bug because it was in the wrong component)
Component: General → Storage
Product: Fennec → Toolkit
QA Contact: general → storage
(fwiw, the original component choice was to match the bug that added test_file_perms.cpp. The new classification makes more sense, though - thanks for fixing it.)
(In reply to comment #8)
> I concur with previous comments: this patch has to be wrong.

The patch preserves the existing (broken) behaviour, so isn't wrong per se, the original code is.

Anyway, I'm going to morph this bug back to being just about the unused variable build warning and file a new one for the broken behaviour in test_file_perms.cpp, so the work on that doesn't hold this up (and given that I can't really fix the rest).
Summary: test_file_perms.cpp:52:12: warning: unused variable ‘rv’ → Remove unused variable |rv| from test_file_perms.cpp:52
Attached patch Remove unused variable rv (obsolete) — Splinter Review
Updated patch that only removes the unused variable.

The other build warning/sorting out the broken behaviour in this test has been left for (the now reopened) bug 650509.
Attachment #540431 - Attachment is obsolete: true
Attachment #540431 - Flags: review?(sdwilsh)
Attachment #546299 - Flags: review?(sdwilsh)
Flags: in-testsuite-
Shawn, ping for review (one line change that removes unused variable). Thanks :-)
Shawn, ping for one line unused variable removal review.
Sorry, I had over 1000 bugmail threads to read.  I should get to this review either today or some time this week.
Comment on attachment 546299 [details] [diff] [review]
Remove unused variable rv

Review of attachment 546299 [details] [diff] [review]:
-----------------------------------------------------------------

So sorry.  This patch used to be more complicated which is why I kept punting on it because it required more time to fact-check.  Clearly, with this version, that isn't needed.  Sorry!

r=sdwilsh

::: storage/test/test_file_perms.cpp
@@ +48,5 @@
>  void
>  test_file_perms()
>  {
>    nsCOMPtr<nsIFile> profDir;
> +  NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(profDir));

nit: Storage style guide says to cast to (void) here since you are not checking the return value.
Attachment #546299 - Flags: review?(sdwilsh) → review+
No worries :-)

Only change is the addition of the void cast per comment 18 / storage style guide; carrying forwards r+.

Thanks!
Attachment #546299 - Attachment is obsolete: true
Oh and the other problems found in this file were broken out to bug 672324.
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/a98208ca4e14
Keywords: checkin-needed
Whiteboard: [build_warning] → [build_warning][inbound]
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/a98208ca4e14
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 680552
No longer blocks: 680552
You need to log in before you can comment on or make changes to this bug.