1316011 - [findbugs] [VO] Increment of volatile field org.mozilla.gecko.sync.synchronizer.SerialRecordConsumer.counter in org.mozilla.gecko.sync.synchronizer.SerialRecordConsumer.stored()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Increment of volatile field org.mozilla.gecko.sync.synchronizer.SerialRecordConsumer.counter in org.mozilla.gecko.sync.synchronizer.SerialRecordConsumer.stored()

"This code increments a volatile field. Increments of volatile fields aren't atomic. If more than one thread is incrementing the field at the same time, increments could be lost."

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

To start, set up a build environment - you can see the instructions here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

Then, you'll need to upload a patch - see:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "sebastian" and you can find me and other helpful folks in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Comment 2

[archit garg[:archit]]

Hi
I want to work on fixing this bug.
What do you suggest I do? Should I change the variable counter from Long to AtomicLong?

Comment 3

[mddrill]

Hi, I cannot find this file in mozilla-central. I'm looking in mozilla-central/mobile/android/base/java/org/gecko/ but there is no sync directory. Am I looking in the right directory?

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to mddrill from comment #3)
> Hi, I cannot find this file in mozilla-central. I'm looking in
> mozilla-central/mobile/android/base/java/org/gecko/ but there is no sync
> directory. Am I looking in the right directory?

It's here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/synchronizer/SerialRecordConsumer.java

Comment 5

[archit garg[:archit]]

:Sebastian you haven't given me any response yet!!

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to archit garg[:archit] from comment #5)
> :Sebastian you haven't given me any response yet!!

Oh, sorry, I did miss that. Make sure to use the "need more information" flag and I won't be able to miss it. :)

> What do you suggest I do? Should I change the variable counter from Long to

Yeah, this is one option. I see that most access is synchronized. If all access is synchronized then we can just remove the 'volatile' keyword because all threads will see the uncached value anyways.

Comment 7

[Owais Kazi]

Hi sebastian!
I want to work on this bug if nobody is working on it

Comment 8

[Owais Kazi]

Attachment: Bug 1316011-Removed use of volatile keyword and thread synchronization is done using AtomicLong Part 2.

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

Comment 9

[Owais Kazi]

Attachment: Bug 1316011-Removed use of volatile keyword and thread synchronization is done using AtomicLong

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

Comment 10

[Owais Kazi]

Hi sebastian!
Can you please review my code https://reviewboard-hg.mozilla.org/gecko/rev/73d8cea7d424?
I have removed volatile keyword using AtomicLong and synchronization of counter is done!

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8818228 [details]
Bug 1316011-Removed use of volatile keyword and thread synchronization is done using AtomicLong Part 2.

https://reviewboard.mozilla.org/r/98374/#review99362

This patch looks like it does not belong here.

If you only want to push a single commit to reviewboard (and assuming you are using mercurial) then you can use "hg push -c CHANGESET".

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8818229 [details]
Bug 1316011-Removed use of volatile keyword and thread synchronization is done using AtomicLong

https://reviewboard.mozilla.org/r/98376/#review99364

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/synchronizer/SerialRecordConsumer.java:86
(Diff revision 1)
> -    long counterNow = this.counter;
> -    Logger.info(LOG_TAG, "Consumer is done. Processed " + counterNow + ((counterNow == 1) ? " record." : " records."));
> +    AtomicLong counterNow = this.counter;
> +    Logger.info(LOG_TAG, "Consumer is done. Processed " + counterNow + ((counterNow.get() == 1) ? " record." : " records."));
>      delegate.consumerIsDone(stopImmediately);

It looks like the idea here is to cache the current value inside the method so that the value does not change between logging it and deciding between "record" and "records".

Either get the value once and keep the rest of the code:
> long counterNow = counter.get();

Or simplify the log message and just append "record(s)". It's only a log message after all.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

I pushed the current version of the patch to "try" so that we can see if the tests catch any errors with the new code:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5fab76461be

Comment 14

[Owais Kazi]

Hi sebastian!
So should i create a new patch or wait for the tests to catch any errors?

Comment 15

[Owais Kazi]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5fab76461be&selectedJob=32823613 It is 100% done can you elaborate me with the results please

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

The failures are related to the other attached patch. Just update the patch (See comment 12) and push it. After that I'll push your updated version to try again.

Comment 17

[Owais Kazi]

Comment on attachment 8818228 [details]
Bug 1316011-Removed use of volatile keyword and thread synchronization is done using AtomicLong Part 2.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98374/diff/1-2/

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8818228 [details]
Bug 1316011-Removed use of volatile keyword and thread synchronization is done using AtomicLong Part 2.

https://reviewboard.mozilla.org/r/98374/#review99454

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8818228 [details]
Bug 1316011-Removed use of volatile keyword and thread synchronization is done using AtomicLong Part 2.

https://reviewboard.mozilla.org/r/98374/#review99456

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18108585b3d5

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8818228 [details]
Bug 1316011-Removed use of volatile keyword and thread synchronization is done using AtomicLong Part 2.

https://reviewboard.mozilla.org/r/98374/#review99458

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18108585b3d5

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82753b1778fa
Removed use of volatile keyword and thread synchronization is done using AtomicLong Part 2. r=sebastian

Comment 22

[Owais Kazi]

Hi sebastian!
Is the bug complete now?

Comment 23

[Richard Newman [:rnewman]]

With all due respect to everyone involved, this bug should probably be WONTFIXed and the patch backed out. Using an AtomicLong here, for a value that's only present for logging, is overkill. And more thorough analysis would show that there is likely to be no concurrency issue here -- after all, this isn't called SerialRecordConsumer for nothing.

Automated error finding tools should not cause knee-jerk patches to stable code.

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Sorry, this is my mistake. I should have analyzed the code more - just because of the synchronization blocks I was assuming this was multi-threaded code (and 'counter' was defined as volatile?). I'll reopen and back out if needed.

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/82753b1778fa

Comment 26

[Owais Kazi]

Is this bug fixed now?

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c60635c5a851ccd47d0325b0eaab18792ae28964
Backed out changeset 82753b1778fa (bug 1316011)

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c60635c5a851 also backed out from central

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1316011 - findbugs: Enable VO_VOLATILE_INCREMENT check and ignore false positive in SerialRecordConsumer.

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

Comment 30

[Julian Chu [:walkingice]]

Comment on attachment 8824126 [details]
Bug 1316011 - findbugs: Enable VO_VOLATILE_INCREMENT check and ignore false positive in SerialRecordConsumer.

https://reviewboard.mozilla.org/r/102676/#review104110

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/482d24b7c611
findbugs: Enable VO_VOLATILE_INCREMENT check and ignore false positive in SerialRecordConsumer. r=walkingice

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/482d24b7c611