1127158 - Incorrect flag math in nsSecureBrowserUIImpl.cpp?

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Jason Duell]

What's up with all the '-=' operations on testFlags here?

   https://dxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp?from=nsSecureBrowserUIImpl.cpp#839

It looks like they should all be "& ~(FLAG)",  not "-= FLAG".  (These are boolean flags we're trying to filter out, so subtracting doesn't make sense.)

Also we don't ever user 'testArgs' so not sure of the point of that variable.

The same boolean flag math error applies to the 'f' variable as well:

   https://dxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp?from=nsSecureBrowserUIImpl.cpp#858

Comment 1

[Jason Duell]

make that "&= ~(FLAG)", of course.

Comment 2

[Justin Dolske [:Dolske]]

Hmm, I think it's _technically_ correct, because it's always checking that the flag is set before subtracting it.

But that seems like a horrible, horrible antipattern and should definitely be changed. Too easy to screw up and is conceptually wrong.

Comment 3

[:Cykesiopka]

Hmm, this is all debug code. I wonder if anyone even uses it.

Comment 4

[:Cykesiopka]

(In reply to :Cykesiopka from comment #3)
> Hmm, this is all debug code. I wonder if anyone even uses it.

I'm going to assume no here and just remove this code.

Reasoning:
 - It's brittle, as mentioned in previous comments.
 - nsSecureBrowserUIImpl.cpp is already a huge pain to read. We don't need more clutter.
 - As more code gets landed all over the tree to support features, stuff like this need to go away so rising compile times are mitigated.

If you do use this code, please maintain a local patch instead.

Comment 5

[:Cykesiopka]

Attachment: MozReview Request: Bug 1127158 - Remove brittle debug only flag math in nsSecureBrowserUIImpl.cpp.

Review commit: https://reviewboard.mozilla.org/r/43791/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43791/

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8737396 [details]
MozReview Request: Bug 1127158 - Remove brittle debug only flag math in nsSecureBrowserUIImpl.cpp.

https://reviewboard.mozilla.org/r/43791/#review40743

Yeah, let's just get rid of this.

Comment 7

[:Cykesiopka]

Thanks for the review!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a45bc62f95768a9c74c0a66268ef7a137ed0423

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cdccb1dd539

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8cdccb1dd539