Closed Bug 1339051 Opened 3 years ago Closed 3 years ago

Data race in iJIT_GetNewMethodID js/src/vtune/jitprofiling.c

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jseward, Assigned: sstangl)

Details

Attachments

(2 files, 1 obsolete file)

Attached file TSan complaint
I think this might be fallout from bug 1332466.  TSan reports 
races as shown in the attachment, when starting the browser.
Ah, good catch. I should have checked: VTune internals are not threadsafe.

This patch moves the DLL/SO load to initialization time and protects the VTune internals with a Big Lock.
Attachment #8837815 - Flags: review?(hv1989)
Assignee: nobody → sstangl
Fixes small logic error. My VTune trial license expired and so I can't test it, but I'll get in contact with Intel to fix that.
Attachment #8837815 - Attachment is obsolete: true
Attachment #8837815 - Flags: review?(hv1989)
Attachment #8837825 - Flags: review?(hv1989)
Comment on attachment 8837825 [details] [diff] [review]
0001-Bug-1339051-Make-VTune-JIT-integration-threadsafe.-r.patch

Review of attachment 8837825 [details] [diff] [review]:
-----------------------------------------------------------------

1) Please remove all double newlines to one newline in VTuneWrapper.cpp
2) I think it would be better to wrap the Locks in VTuneWrapper.cpp in {} to make sure they are only initialized at that place. Else they can be hoisted to the top of the function IIRC.
   => as a side-note: Wouldn't it be better to create a wrapper function for iJIT_NotifyEvent that does locking and calls iJIT_NotifyEvent ??
Attachment #8837825 - Flags: review?(hv1989) → review+
Priority: -- → P1
Pushed by sstangl@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12d7b6083d81
Make VTune JIT integration threadsafe. r=h4writer
https://hg.mozilla.org/mozilla-central/rev/12d7b6083d81
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.