Fix a conditional statement in nsNativeThemeWin::GetThemePartAndState

NEW
Unassigned

Status

()

defect
P5
normal
4 years ago
8 days ago

People

(Reporter: Ehsan, Unassigned, Mentored)

Tracking

({good-first-bug})

Trunk
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: tpi:+[lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

Found by Viva64.
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.
Flags: needinfo?(ehsan)
Blocks: 710966
Duplicate of this bug: 1209141
Sorry, this patch here says:
} else (isCheckbox && GetIndeterminate(aFrame)) {

The patch in bug 1209141 said:
} else if (isCheckbox && GetIndeterminate(aFrame)) {
(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.
Flags: needinfo?(ehsan)
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.
Mentor: jmathies
Component: Widget → Widget: Win32
Keywords: good-first-bug
OS: All → Windows
Priority: -- → P5
Version: unspecified → Trunk
Whiteboard: tpi:+
Is this bug still open?  How can I start working on this if yes?
Flags: needinfo?(jmathies)
Attachment #8666524 - Attachment is obsolete: true
Flags: needinfo?(jmathies)
Sure, go for it.
(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;
         }
Flags: needinfo?(jmathies)
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
(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.
Flags: needinfo?(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.
Flags: needinfo?(ssitter)
ni for comment 14.
Whiteboard: tpi:+ → tpi:+[lang=c++]
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?
(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
Flags: needinfo?(jmathies)
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.
Hey..Can anyone help me how should I start working on this issue?
I'd like to be assigned this project
I would like to work on this, I am new to this so would like a little help.
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!
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.
Posted file [mq]: 1208906
Removed one never changed bool variable and its usage in the ternary operators and if statements.
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.

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

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.

You need to log in before you can comment on or make changes to this bug.