Closed Bug 1451198 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: tsmith, Assigned: jandem)

References

Details

Attachments

(5 files, 1 obsolete file)

Attached file 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
Attached file valgrind.supp
Attached file mozconfig
gcc (Ubuntu 7.2.0-8ubuntu3.2) 7.2.0 was used to perform the firefox build.
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).
Component: JavaScript Engine: JIT → JavaScript Engine
This makes it basically impossible to use Valgrind with Gecko for the time being, right? -> P1.
Priority: -- → P1
Blocks: 1454999
These are only used in UnboxedObject.cpp so let's move them there.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8969191 - Flags: review?(bhackett1024)
Attached patch Part 2 - Add annotation (obsolete) — Splinter Review
Attachment #8969193 - Flags: review?(bhackett1024)
Attachment #8969193 - Attachment is obsolete: true
Attachment #8969193 - Flags: review?(bhackett1024)
Attachment #8969194 - Flags: review?(bhackett1024)
Ted maybe you can steal the reviews if you get to it before Brian - this is blocking people from landing patches in bug 1454999.
Flags: needinfo?(tcampbell)
Attachment #8969191 - Flags: review?(bhackett1024) → review+
Attachment #8969194 - Flags: review?(bhackett1024) → review+
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.
No problem!
Flags: needinfo?(tcampbell)
Duplicate of this bug: 1454999
https://hg.mozilla.org/mozilla-central/rev/5f017a1dc4a9
https://hg.mozilla.org/mozilla-central/rev/6c222e89103d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.