128150 - race condition in property cache

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Mike Epstein]

PROPERTY_CACHE_FILL increments cache->fills before calling PCE_STORE.  There is 
no protection against PCE_STORE racing with PCE_LOAD on another thread.  That 
call to PCE_LOAD could execute entirely with a single value of cache->fills and 
load an inconsistent property cache entry.  We have seen this happen in 
practice several times.

I'm attaching a patch which creates a second counter; one counter is 
incremented before PCE_STORE, the other after.  PCE_LOAD checks that both 
counters are equal and that they have not changed during the load.  A few 
caveats about the patch:
(1) It is untested, alas.  (We're not currently running with an up-to-date JS 
engine, but the bug is still present in tip of tree.)
(2) It might not correctly handle multiprocessor situations in all cases, but 
it does constitute a uniprocessor fix.

Comment 1

[Mike Epstein]

Attachment: Untested patch

Comment 2

[Brendan Eich [:brendan]]

Those macros predate JS_THREADSAFE, and then I got tired.  My secret shame!

Shaver's helping.  I think we should get a fix in 0.9.9.

/be

Comment 3

[Asa Dotzler [:asa]]

shaver, brendan, is this really an 0.9.9 stopper? If so can we get the patch
tested, reviewed and it in today?

Comment 4

[Brendan Eich [:brendan]]

Still want this for 0.9.9, but my brain has turned to quacamole.  Sleep, then
comment....

/be

Comment 5

[Brendan Eich [:brendan]]

Can't justify holding 0.9.9 for this if it's not biting the big M (it is biting
tellme.com, shaver and I are still working on the patch, it's a top priority for
mozilla1.0).

/be

Comment 6

[Brendan Eich [:brendan]]

Ok, shaver got me some MMX-based love for modern gcc/x86 platforms.  Cc'ing
jdunn and wtc -- can you guys help by identifying existing intrinsics or OS
headers to include for atomic double-word (two pointers in a row, wrapped in a
struct) loads and stores?

/be

Comment 7

[Wan-Teh Chang]

Brendan,

What you want to do is usually possible in a 32-bit OS running
on top of a 64-bit processor.  Unfortunately, NSPR's PR_Atomic
routines operate on 32-bit integers, so I am not familiar with
atomic double-word loads and stores.

Comment 8

[Brendan Eich [:brendan]]

Attachment: hack-patch for gcc/x86-with-mmx only

Testing welcome, this obviously needs #ifdef'ing.

/be

Comment 9

[Asa Dotzler [:asa]]

brendan, is this an RC2 stopper? If it is can we get reviews and super-reviews
quickly and get this into the tree Wednesday? If it's not a stopper can you
remove the blocks 138000?

Comment 10

[Brendan Eich [:brendan]]

I'll have a better patch today, have this in the trunk as soon as reviewers and
platform buddies vouch for it.

/be

Comment 11

[Bradley Baetz (:bbaetz)]

Your configure checks needs to check that the assembler is new enough
understands these instructions, too.

Since all mozilla builds build for i386 by default, most people won't use this
in any event, though.

Comment 12

[chris hofmann]

is this going to get rc3 love in the next day or two?

Comment 13

[Brendan Eich [:brendan]]

js1.5 will be mozilla1.0.1, yay.

/be

Comment 14

[Brendan Eich [:brendan]]

Moving out a bit.  The js1.5 release (after an rc5 release candidate) should
come off off the 1.0 branch on or before 1.0.2 freezing, I say.  The trunk and
branch should be in sync for js/src/js*.[chmt]* and build files.

/be

Comment 15

[Brendan Eich [:brendan]]

Fixing TM.

/be

Comment 16

[Brendan Eich [:brendan]]

Hoping for some progress here soon.

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: remove property cache

The property cache is incorrect on MP systems, bloaty in its jsinterp.c macrology, and much less effective than polymorphic inline caches, which I'm working on in the context of bug 365851.

This will regress performance for some benchmarks, but it will be interesting to see whether it affects any of the Mozilla tinderbox "T" metrics.

/be

Comment 18

[Igor Bukanov]

Comment on attachment 250873 [details] [diff] [review]
remove property cache

I like patches dominated by munuses.

Comment 19

[Brendan Eich [:brendan]]

Fixed on trunk:

js/src/jsapi.c rev 3.303
js/src/jscntxt.h rev 3.135
js/src/jsgc.c rev 3.198
js/src/jsinterp.c rev 3.318
js/src/jsinterp.h rev 3.52
js/src/jsobj.c rev 3.316

I'll note tinderbox Tp/Ts/Tdhtml/Txul results in a bit.

/be

Comment 20

[Brendan Eich [:brendan]]

Forgot to prune two sprop locals, now unused:

js/src/jsobj.c rev 3.317

/be

Comment 21

[Brendan Eich [:brendan]]

No big bad jumps.  Slight Tdhtml ding?  Cc'ing bz in case he can eyeball quickly and see something serious.  Again I'm working on not just restoring lost perf by eliminating the direct runtime-wide property cache -- I'm hoping to optimize calls and gets and sets quite a bit.

/be

Comment 22

[Boris Zbarsky [:bzbarsky]]

Doesn't seem to be much of an effect on Tdhtml, actually, though I'm still getting used to what numbers I can expect from this box.  Sadly, the test is pretty noisy.  :(