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

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript Engine: JIT
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jseward, Assigned: sstangl)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Created attachment 8836699 [details]
TSan complaint

I think this might be fallout from bug 1332466.  TSan reports 
races as shown in the attachment, when starting the browser.
(Assignee)

Comment 1

a year ago
Created attachment 8837815 [details] [diff] [review]
0001-Bug-1339051-Make-VTune-JIT-integration-threadsafe.-r.patch

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)

Updated

a year ago
Assignee: nobody → sstangl
(Assignee)

Comment 2

a year ago
Created attachment 8837825 [details] [diff] [review]
0001-Bug-1339051-Make-VTune-JIT-integration-threadsafe.-r.patch

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

Comment 4

a year ago
Pushed by sstangl@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12d7b6083d81
Make VTune JIT integration threadsafe. r=h4writer

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/12d7b6083d81
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.