653844 - Remove the ifdefs for pr_logging from storage code

Status: RESOLVED FIXED

(Toolkit :: Storage, enhancement)

Description

[Allison Naaktgeboren :ally]

Storage has lots of #ifdef PR_LOGGING, which then calls PR_LOG, so doesn't need the macro.  There are also a bunch of DEBUG ones, which are safe without the ifdef because the macros will be a no op if they are not defined.

Comment 1

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> Storage has lots of #ifdef PR_LOGGING, which then calls PR_LOG, so doesn't need
> the macro.  There are also a bunch of DEBUG ones, which are safe without the
> ifdef because the macros will be a no op if they are not defined.
Note that some of the #ifdefs are there because we do extra work and then warn or log.  However, if we don't do extra work before then, there is no reason to wrap it in the #ifdefs

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Ally, do you still think this is a good first bug? If so, maybe you could describe the problem a little bit more, or add yourself as a mentor.  I'm following up on older bugs that are marked "good first" and that haven't been changed in a long time. Thanks!

Comment 3

[Allison Naaktgeboren :ally]

I think it might be. That said, I have not looked at that code in ..2+ years, and I am not a reviewer for storage, so I am not qualified to mentor this bug. I imagine :mak would have a more informed opinion.

Comment 4

[Marco Bonardo [:mak]]

yes, there are still points where it's pointless to wrap PR_LOG with #ifdef PR_LOGGING or #ifdef DEBUG, because PR_LOG is a no-op in builds without PR_LOGGING.
It may still be useful where there is additional code needed by the PR_LOG invocation, as comment 1 said.

Comment 5

[Anand Soni]

Hi Marco,

I am interested in fixing this. Kindly let me know the complete details. Thanks a lot!

Comment 6

[Anup Kumar [:reznord]]

Hi, I am interested in working on this bug. So, I request you to assign it to me and guide me on how to get started with because I am a newbie. I am trying to fix the PR_LOG error and if you assign it to me I will complete the patch work for this.

Thanks in advance,
A.Anup

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Anup, here's some basic information on how to submit a patch. 

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

Comment 8

[Anup Kumar [:reznord]]

Marco Bonardo:

Sir,

I have created a patch file and attached it for a review. I have a small question, if we want to test this particular bug how to proceed on it sir?

Thanks in Advance,

Regards,
A.Anup

Comment 9

[Anup Kumar [:reznord]]

Attachment: Patch work done on the bug. Lists all the changes which I have done to the code.

In the following attachment I have added a file by name "diff.txt" in which all the changes to the code are saved. Hope I will get a review as soon as possible. 

Thanks in Advance,

Regards,
A.Anup.

Comment 10

[Anup Kumar [:reznord]]

Attachment: In this I have made a few changes to the code and uploaded.

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 788070 [details] [diff] [review]
In this I have made a few changes to the code and uploaded.

This is not what was intended, unfortunately. When we talk about removing the #ifdef, we really mean removing the entire #ifdef line, and the corresponding #endif. This won't compile as written. Furthermore, this is removing #ifdefs that are unrelated to PR_LOG, which is outside the scope of the changes talked about.

Comment 12

[Josh Matthews [:jdm]]

Make sure to always build Firefox again before submitting a patch; that's a good way to ensure that you're not going to break anything, regardless as to whether you know how to test the changes or not.

Comment 13

[Anup Kumar [:reznord]]

Attachment: Consists of the changes made as per the details in the bug.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Anup, as we discussed on irc, you should just flag the mentor for the bug, who is mak.  You may not get a reply immediately. I understand you are doing this for a school assignment, but these things do take time. Please be patient!

Comment 15

[Anup Kumar [:reznord]]

Attachment: contains some more modifications comparing to the first patch.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Anup, please don't request checkin on patches that haven't been reviewed yet.

Comment 17

[Anup Kumar [:reznord]]

It's been a very long time that is why I have tried for a checkin. Sorry for that.

Comment 18

[Anup Kumar [:reznord]]

It's been a very long time I submitted a patch, that is why I have tried for a checkin. 

Sorry for that

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 788655 [details] [diff] [review]
contains some more modifications comparing to the first patch.

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

Apologize for the long delay, I assume this is the most recent patch to review.
When attaching newer version of the patches, please obsolete the previous ones.

I went through and commented each single case, but mostly they are similare or the same.
The basic rule here is that we fix only call points that don't do additional work apart from calling a macro, as well explained by comment 1.
So basically, this is doing a bit too much.

::: storage/public/mozIStorageValueArray.idl
@@ +82,1 @@
>      nsresult rv =

this should be changed to a DebugOnly<nsresult> otherwise it would warn an unused variable

The same should be done for all of the other changes similar to this one (so when a variable definition is the only thing inside an ifdef DEBUG). So, I'm not going to repeat the comment, but is valid for any of those cases.

::: storage/src/VacuumManager.cpp
@@ +76,3 @@
>    warnMsg.AppendLiteral(" ");
>    warnMsg.Append(message);
>    NS_WARNING(warnMsg.get());

this should not be changed, since in non debug mode we don't need to build a warnMsg

@@ +240,5 @@
> +    warnMsg.AppendLiteral(" - ");
> +    warnMsg.AppendInt(result);
> +    warnMsg.AppendLiteral(" ");
> +    warnMsg.Append(message);
> +    NS_WARNING(warnMsg.get());

same here, please revert as it was before

As a general rule, any work that is needed only in debug mode should be ifdefed.

@@ +256,4 @@
>             ("Vacuum failed with error: %d '%s'. Database was: '%s'",
>              result, message.get(), mDBFilename.get()));
>    }
> +

This piece of code is doing extra work apart the PR_LOG, so the ifdef should not be removed, see comment 1

::: storage/src/mozStorageAsyncStatement.cpp
@@ +145,3 @@
>    // We want to try and test for LIKE and that consumers are using
>    // escapeStringForLIKE instead of just trusting user input.  The idea to
>    // check to see if they are binding a parameter after like instead of just

this piece of code is doing more than just warn, so it should stay ifdef debug as before

@@ +293,5 @@
>    // Make sure we are never called on the connection's owning thread.
>    bool onOpenedThread = false;
>    (void)mDBConnection->threadOpenedOn->IsOnCurrentThread(&onOpenedThread);
>    NS_ASSERTION(!onOpenedThread,
>                 "We should only be called on the async thread!");

doing more work, should stay ifdef

::: storage/src/mozStorageAsyncStatementExecution.cpp
@@ +227,5 @@
>    mMutex.AssertNotCurrentThreadOwns();
>  
>    bool onCallingThread = false;
>    (void)mCallingThread->IsOnCurrentThread(&onCallingThread);
>    NS_ASSERTION(onCallingThread, "runEvent not running on the calling thread!");

should stay ifdef

@@ +322,2 @@
>    // Check to make sure that this statement was smart about what it did.
>    checkAndLogStatementPerformance(aStatement);

should stay ifdef

@@ +545,3 @@
>    bool onCallingThread = false;
>    (void)mCallingThread->IsOnCurrentThread(&onCallingThread);
>    NS_ASSERTION(onCallingThread, "Not canceling from the calling thread!");

should stay ifdef

::: storage/src/mozStorageAsyncStatementJSHelper.cpp
@@ +36,4 @@
>    int32_t state;
>    (void)aStatement->GetState(&state);
>    NS_ASSERTION(state == mozIStorageAsyncStatement::MOZ_STORAGE_STATEMENT_READY,
>                 "Invalid state to get the params object - all calls will fail!");

should stay ifdef

@@ +102,5 @@
>    {
>      nsISupports *supp = aWrapper->Native();
>      nsCOMPtr<mozIStorageAsyncStatement> isStatement(do_QueryInterface(supp));
>      NS_ASSERTION(isStatement, "How is this not an async statement?!");
>    }

should stay ifdef

::: storage/src/mozStorageConnection.cpp
@@ +353,5 @@
>      if (!onCallingThread) {
> +
> +      bool onAsyncThread = false;
> +      (void)mAsyncExecutionThread->IsOnCurrentThread(&onAsyncThread);
> +      MOZ_ASSERT(onAsyncThread);

should stay ifdef

@@ +636,2 @@
>    if (!gStorageLog)
>      gStorageLog = ::PR_NewLogModule("mozStorage");

should stay ifdef

@@ +811,2 @@
>    // Sanity checks to make sure we are in the proper state before calling this.
>    NS_ASSERTION(mDBConn, "Database connection is already null!");

should stay ifdef

@@ +829,5 @@
>    nsAutoCString leafName(":memory");
>    if (mDatabaseFile)
>        (void)mDatabaseFile->GetNativeLeafName(leafName);
>    PR_LOG(gStorageLog, PR_LOG_NOTICE, ("Closing connection to '%s'",
>                                        leafName.get()));

should stay ifdef

@@ +842,3 @@
>      NS_WARNING(msg);
>      ::PR_smprintf_free(msg);
>    }

should stay ifdef

::: storage/src/mozStoragePrivateHelpers.cpp
@@ +75,5 @@
>    nsAutoCString message;
>    message.AppendLiteral("SQLite returned error code ");
>    message.AppendInt(rc);
>    message.AppendLiteral(" , Storage will convert it to NS_ERROR_FAILURE");
>    NS_WARNING(message.get());

should stay ifdef

::: storage/src/mozStorageStatement.cpp
@@ +173,1 @@
>    // We want to try and test for LIKE and that consumers are using

should stay ifdef

@@ +505,4 @@
>    PR_LOG(gStorageLog, PR_LOG_DEBUG, ("Resetting statement: '%s'",
>                                       ::sqlite3_sql(mDBStatement)));
>  
>    checkAndLogStatementPerformance(mDBStatement);

only the checkAndLog should still be ifdef

@@ +583,5 @@
>        nsAutoCString errStr;
>        (void)mDBConnection->GetLastErrorString(errStr);
>        PR_LOG(gStorageLog, PR_LOG_DEBUG,
>               ("Statement::ExecuteStep error: %s", errStr.get()));
>    }

doing more work so should stay ifdef

::: storage/src/mozStorageStatementData.h
@@ +88,5 @@
> +    // method would return nullptr as a result.
> +    if (asyncThread) {
> +      bool onAsyncThread;
> +      NS_ASSERTION(NS_SUCCEEDED(asyncThread->IsOnCurrentThread(&onAsyncThread)) && onAsyncThread,
> +                   "This should only be running on the async thread!");

should stay ifdef

::: storage/src/mozStorageStatementJSHelper.cpp
@@ +51,5 @@
>        do_QueryInterface(wrapper->Native())
>      );
>      NS_ASSERTION(isStatement, "How is this not a statement?!");
>    }
> +

every changes in this file should be reverted

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 788228 [details] [diff] [review]
Consists of the changes made as per the details in the bug.

obsoleted by new version, that I just reviewed.

Comment 21

[weeska]

I just looked if i'd work on this ticket, but it seems it is already completed. Should this be closed?

Comment 22

[Josh Matthews [:jdm]]

The patch needs to be modified according to the requests in comment 19. It might be easier to start from scratch; I'm not sure.

Comment 23

[weeska]

Attachment: remove ifdefs for pr_logging where they're not needed

Comment 24

[Josh Matthews [:jdm]]

In future you want to set the review flag to ?, and mark old patches as obsolete.

Comment 25

[weeska]

Thanks, i'll do that.

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 826406 [details] [diff] [review]
remove ifdefs for pr_logging where they're not needed

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

This patch looks good, though it's missing a piece compared to the previous one, but I'm fine to have those handled in a separate patch:
- in mozIStorageValueArray.idl most of the "#ifdef DEBUG\nnsresult rv =\n#endif\n" can be replaced by DebugOnly<nsresult> rv =

Comment 27

[Marco Bonardo [:mak]]

> - in mozIStorageValueArray.idl most of the "#ifdef DEBUG\nnsresult rv
> =\n#endif\n" can be replaced by DebugOnly<nsresult> rv =

I've splitted this part to bug 939297, so this can be resolved on landing, feel free to take the new bug if you wish. Thanks.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/444714c3820a

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/444714c3820a