Closed Bug 1162691 Opened 5 years ago Closed 5 years ago

Remove usage of |#ifdef PR_LOGGING| from security

Categories

(Core :: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(2 files)

In bug 1161238 we plan on removing |--disable-logging| which makes |#ifdef PR_LOGGING| redundant.
Check that logging is enabled before performing potentially expensive
operations.
Attachment #8602927 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
PR_LOGGING is now always defined, we can remove #ifdefs checking for it.
Attachment #8602928 - Flags: review?(nfroyd)
Attachment #8602928 - Flags: review?(nfroyd) → review+
Comment on attachment 8602927 [details] [diff] [review]
Part 2: Wrap expensive calls in PR_LOG_TEST

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

r=me with the changes below.

::: security/manager/boot/src/nsSecureBrowserUIImpl.cpp
@@ +680,5 @@
>    uint32_t loadFlags = 0;
>    aRequest->GetLoadFlags(&loadFlags);
>  
> +  if (PR_LOG_TEST(gSecureDocLog, PR_LOG_DEBUG)
> +      &&

I don't think this change is worth it; testing flags isn't particularly expensive and the PR_LOG below will already check this condition.

@@ +694,5 @@
>             ("SecureUI:%p: OnStateChange: SOMETHING STARTS FOR TOPMOST DOCUMENT\n", this));
>    }
>  
> +  if (PR_LOG_TEST(gSecureDocLog, PR_LOG_DEBUG)
> +      &&

Ditto.
Attachment #8602927 - Flags: review?(nfroyd) → review+
Will remove the two tests before I land.
https://hg.mozilla.org/mozilla-central/rev/3cdce28ffcc6
https://hg.mozilla.org/mozilla-central/rev/b969ae256202
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.