1451198 - Valgrind: multiple instances of Conditional jump or move depends on uninitialised value on launch

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Tyson Smith [:tsmith]]

Attachment: valgrind.txt

Found with mozilla-central changeset: 411539:00bdc9451be6
I also used Valgrind built from commit: 54145019b045fffde625447b64f3a91f663de718

See the attached log for all the reports. These errors are triggered on launch with a clean profile.

Launch command used: valgrind -q --leak-check=no --suppressions=valgrind.supp --read-inline-info=yes --show-mismatched-frees=no --show-possibly-lost=no --smc-check=all-non-file --track-origins=yes --vex-iropt-register-updates=allregs-at-mem-access /home/user/code/mozilla-central/objdir-ff-valgrind/dist/bin/firefox

Comment 1

[Tyson Smith [:tsmith]]

Attachment: valgrind.supp

Comment 2

[Tyson Smith [:tsmith]]

Attachment: mozconfig

Comment 3

[Tyson Smith [:tsmith]]

gcc (Ubuntu 7.2.0-8ubuntu3.2) 7.2.0 was used to perform the firefox build.

Comment 4

[Jan de Mooij [:jandem]]

Very likely from bug 1448589, it added instrumentation to the GC allocators.

I looked at the first report - it's probably this: when we allocate an unboxed object, we initialize the unboxed GC pointers to nullptr (necessary for GC) but we don't initialize int32/double/bool properties. Then convertToNative can read these fields and convert to Int32Value/DoubleValue/BooleanValue, and then Valgrind complains when the post barrier checks if the Value is an object or string. It never will be, but the Value does potentially contain some garbage payload.

Ideally we would initialize these fields too, or else mark these fields as defined for Valgrind/MSan (maybe in convertToNative or something).

Comment 5

[Jason Orendorff [:jorendorff]]

This makes it basically impossible to use Valgrind with Gecko for the time being, right? -> P1.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Move GetUnboxedValue and SetUnboxedValue to cpp file

These are only used in UnboxedObject.cpp so let's move them there.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Add annotation

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Add annotation

Comment 9

[Jan de Mooij [:jandem]]

Ted maybe you can steal the reviews if you get to it before Brian - this is blocking people from landing patches in bug 1454999.

Comment 10

[Brian Hackett [Laid off!]]

Sorry for the late reviews.  My internet access will be rather intermittent the next few months so feel free to steal or reassign things as necessary for time critical bugs.

Comment 11

[Jan de Mooij [:jandem]]

No problem!

Comment 12

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f017a1dc4a9f6b1631787aa7bb6b13352f8c3af
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c222e89103d4c70c58bf1955ab81cef8e8b62e1

Comment 14

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/5f017a1dc4a9
https://hg.mozilla.org/mozilla-central/rev/6c222e89103d