1208906 - Fix a conditional statement in nsNativeThemeWin::GetThemePartAndState

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P5)

Description

[(no longer active)]

Found by Viva64.

Comment 1

[(no longer active)]

Attachment: Fix a conditional statement in nsNativeThemeWin::GetThemePartAndState

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26396d426123

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32076ec3f4c0

Comment 4

[Nigel Babu [:nigelb]]

The followup caused windows reftest bustages like this - https://treeherder.mozilla.org/logviewer.html#?job_id=14778516&repo=mozilla-inbound. I'm going to back out the original patch and the followup.

Comment 5

[Nigel Babu [:nigelb]]

Backed out in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/069515c93b96
https://hg.mozilla.org/integration/mozilla-inbound/rev/94eb4bc018b4

Comment 7

[Jorg K (CEST = GMT+2)]

Sorry, this patch here says:
} else (isCheckbox && GetIndeterminate(aFrame)) {

The patch in bug 1209141 said:
} else if (isCheckbox && GetIndeterminate(aFrame)) {

Comment 8

[(no longer active)]

(In reply to Nigel Babu [:nigelb] from comment #4)
> The followup caused windows reftest bustages like this -
> https://treeherder.mozilla.org/logviewer.html#?job_id=14778516&repo=mozilla-
> inbound. I'm going to back out the original patch and the followup.

Looks like this indeed changes the behavior in a way that we rely on.  Someone needs to debug this, I won't have time.

Comment 9

[Stefan Sitter [:ssitter]]

A similar check can be found later in the same file exposing the same logic. The element can be checked or unchecked. And if the element is a checkbox it could be indeterminate too. Indeterminate state takes precedence over checked/unchecked state:
https://dxr.mozilla.org/comm-central/source/mozilla/widget/windows/nsNativeThemeWin.cpp#3144

To me it looks like the code is correct and the static analyzer finding can be treated as false positive. Maybe there could be comment added explaining why the code does what it does. Or maybe the code could be re-ordered to not trigger static analyzer finding. Or maybe nothing should be done.

Comment 10

[Venkatesh (:vpit3833)]

Is this bug still open?  How can I start working on this if yes?

Comment 11

[Jim Mathies [:jimm]]

Sure, go for it.

Comment 12

[Venkatesh (:vpit3833)]

(In reply to Jim Mathies [:jimm] from comment #11)
> Sure, go for it.

Thank you.

The two if statements are checking different things but writing to the same variable.  Both if statements write/overwrite inputState.  So, is the diff below good for it?

diff --git a/widget/windows/nsNativeThemeWin.cpp b/widget/windows/nsNativeThemeWin.cpp
--- a/widget/windows/nsNativeThemeWin.cpp
+++ b/widget/windows/nsNativeThemeWin.cpp
@@ -907,7 +907,7 @@ nsNativeThemeWin::GetThemePartAndState(n
       } else {
         if (GetCheckedOrSelected(aFrame, !isCheckbox)) {
           inputState = CHECKED;
-        } if (isCheckbox && GetIndeterminate(aFrame)) {
+        } else if (isCheckbox && GetIndeterminate(aFrame)) {
           inputState = INDETERMINATE;
         }

Comment 13

[Stefan Sitter [:ssitter]]

The proposed patch is identical to the one that caused problems 3 years ago, see comment 7 and comment 8.

Did anybody look into my comment 9? Due to file changes the checkbox code is now located at
https://dxr.mozilla.org/comm-central/source/mozilla/widget/windows/nsNativeThemeWin.cpp#894
https://dxr.mozilla.org/comm-central/source/mozilla/widget/windows/nsNativeThemeWin.cpp#3082

Comment 14

[Venkatesh (:vpit3833)]

(In reply to Stefan Sitter [:ssitter] from comment #13)
> The proposed patch is identical to the one that caused problems 3 years ago,
> see comment 7 and comment 8.
> 
> Did anybody look into my comment 9? Due to file changes the checkbox code is
> now located at

Just looking.

> https://dxr.mozilla.org/comm-central/source/mozilla/widget/windows/
> nsNativeThemeWin.cpp#894
> https://dxr.mozilla.org/comm-central/source/mozilla/widget/windows/
> nsNativeThemeWin.cpp#3082

Adding a comment on why the code is written that way seems the simpler of the options.  Why indeterminate is allowed to overwrite checked state could be written as comment.  'indeterminate takes precedence over checkedness.' as seen at https://dxr.mozilla.org/comm-central/source/mozilla/widget/windows/nsNativeThemeWin.cpp#3082 is not any more clear to me than what I can already read from the code.  How should I find out?

How different is mozilla-unified from mozilla-central or comm-central?  I have mozilla-unified from when I downloaded the sources first time.

Comment 15

[Stefan Sitter [:ssitter]]

I am not familiar with the code myself. This is the reason why a raised the questions in comment 9 hoping to get some answers from the experts.

I used to work on Thunderbird and therefore my bookmarked DXR link is to comm-central repository. But the content of mozilla/ is same as Firefox mozilla-central repository. If you work on Firefox use the latter one.

Comment 16

[Eric Rahm [:erahm]]

ni for comment 14.

Comment 17

[pratim.bhosale]

Hello sir, I am new to this platform of open source contribution and I don't have much experience in it. Is the bug still open for a fix?

Comment 18

[Jim Mathies [:jimm]]

(In reply to pratim.bhosale from comment #17)
> Hello sir, I am new to this platform of open source contribution and I don't
> have much experience in it. Is the bug still open for a fix?

sure! please read over the comments in this bug for some background. someone also tried to fix this but ran into some problems with test failures.

To start I'd suggest getting a local Windows firefox build going - 
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites

Comment 19

[Devika Sugathan]

Hello sir,
I am an absolute beginner in this platform and would like to work on this. Could you please help me with where to start after building the environment.

Comment 20

[Deepika]

Hey..Can anyone help me how should I start working on this issue?

Comment 21

[Alex]

I'd like to be assigned this project

Comment 22

[Saurabhr]

I would like to work on this, I am new to this so would like a little help.

Comment 23

[Viraj Malia]

Hi everyone,

I'm new here but have set up everything to start working on the bug (AFAIK). The obsolete attachment gave me an idea as to where to look within the file. Due to changes in code over time, the conditional statements are placed yet again at different line numbers. As of Nov 6th, 2018, it is line 930-950.

My question is - Is there a way to figure out what the bug or problem is, exactly? I believe that Viva64(a static code-analyzer) found this bug. I don't know how Viva64 works but does it show or mention what might be the issue with such a code construct?

If the answer to my questions is no, then the only way to go about this bug is to read and understand what the code is trying to do and figure out what might be causing it.

Lastly, the code that I see as of the mentioned date looks quite different than the obsolete attachment and all the other links in the comments are dead. Any insight on whether the bug is solved or still open and any ideas would be appreciated.

Thank you all!

Comment 24

[Adrian Kaczmarek]

Hi everybody,
I would like to start working on this bug.
I might need some guidance as this is going to be my first bug.

Comment 25

[Adrian Kaczmarek]

Attachment: [mq]: 1208906

Removed one never changed bool variable and its usage in the ternary operators and if statements.

Comment 26

[P Kausthubh S]

Hello, I am completely new to contributing to open source and believe this bug would be a great place to start. I have already built a debug version of FF from the link in comment 9. Could you help me through the process of picking up this bug and fixing it? Thank you.

Comment 27

[Vaishnavi Kaulagi]

Can I try this one? I'm also completely new to open source development.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Instead of asking if you can contribute, just try to submit a patch! It should work better :)
https://phabricator.services.mozilla.com/D13721 from Adrian seems to be an excellent start, to take in account Ehsan's feedback and we should be good.

Comment 29

[Cosm]

Attachment: 1208906.patch

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 9105820 [details] [diff] [review]
1208906.patch

Not sure you have permissions to approve a patch :)

Comment 31

[Cosm]

Comment on attachment 9105820 [details] [diff] [review]
1208906.patch

Sorry, I thought that I was asking for a review. XD

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 9105820 [details] [diff] [review]
1208906.patch

It was review=? but we don't use bugzilla for code review anymore. 
We use phabricator instead https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#user-guide

Comment 33

[Cosm]

Attachment: 1208906.patch

Comment 34

[Cosm]

Attachment: Bug 1208906 - Remove an unnecessary conditional;r=ehsan,jmathies

Comment 35

[sanchit.arora.2002]

Hi, I'm just starting to contribute to Mozilla. Can I try this bug?

Comment 36

[Sylvestre Ledru [:Sylvestre]]

sure, please take in account the various comments

Comment 37

[Ajitesh13]

Can i take this bug ?

Comment 38

[Mahak]

Attachment: Bug 1208906 - Fix a conditional statement in nsNativeThemeWin::GetThemePartAndState.

Depends on D61447

Comment 39

[Mahak]

@Sylvestre can you please review it ?

Comment 40

[Sylvestre Ledru [:Sylvestre]]

Nope, you should try with someone who gave feedback in that bug.

Comment 41

[Mahak]

@Ehsan Could you please review it ?

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d18b71477f43
Fix a conditional statement in nsNativeThemeWin::GetThemePartAndState. r=Ehsan

Comment 43

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/d18b71477f43