Bug 158280

Summary: Structure::previousID() races with Structure::allocateRareData()
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
possibly the patch
none
performance
none
the patch mark.lam: review+

Description Filip Pizlo 2016-06-01 15:46:20 PDT
It's such an uncommon race, but it's totally devastating: we'll get a RareData pointer but thing it's a Structure.  This passes most assertions - for example it passes cell validation - but later causes crashes in AI.
Comment 1 Filip Pizlo 2016-06-01 15:52:07 PDT
Created attachment 280275 [details]
possibly the patch

I haven't benchmarked it yet.
Comment 2 WebKit Commit Bot 2016-06-01 15:53:42 PDT
Attachment 280275 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/PutByIdVariant.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Locker.h:48:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Filip Pizlo 2016-06-01 18:55:22 PDT
Created attachment 280292 [details]
performance

Performance is not so great - there are slow-downs.  I'll test some more, but I think that I want to go with a lock-free solution.  It should be easy.
Comment 4 Filip Pizlo 2016-06-01 20:14:31 PDT
Ran again, and the regression disappeared.  Nonetheless, I have a better approach, patch coming soon.
Comment 5 Filip Pizlo 2016-06-01 20:16:44 PDT
Created attachment 280299 [details]
the patch
Comment 6 Mark Lam 2016-06-01 20:50:13 PDT
Comment on attachment 280299 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280299&action=review

r=me.  Is there a test case for this race condition?

> Source/JavaScriptCore/runtime/Structure.h:290
> +        // This is so written because it's used concurrently.
> +        JSCell* cell = m_previousOrRareData.get();
> +        if (isRareData(cell))
> +            return static_cast<StructureRareData*>(cell)->previousID();
> +        return static_cast<Structure*>(cell);

Can you make it more explicit as to what makes this safe for concurrency?  I believe that it is the part that pre-fetches the cell before operating on it.  How about having the comment say something like:

// We must pre-fetch the cell from m_previousOrRareData before examining it. The m_previousOrRareData field may change on us due to another thread writing to it, but the local copy won't.

> Source/JavaScriptCore/runtime/Structure.h:703
> +    bool isRareData(JSCell* cell) const

I think this should be a static method instead of a const instance method.  You're not accessing any members of the instance here.
Comment 7 Filip Pizlo 2016-06-01 21:11:05 PDT
(In reply to comment #6)
> Comment on attachment 280299 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280299&action=review
> 
> r=me.  Is there a test case for this race condition?

No.  It does cause occasional crashes in run-javascriptcore-tests.

> 
> > Source/JavaScriptCore/runtime/Structure.h:290
> > +        // This is so written because it's used concurrently.
> > +        JSCell* cell = m_previousOrRareData.get();
> > +        if (isRareData(cell))
> > +            return static_cast<StructureRareData*>(cell)->previousID();
> > +        return static_cast<Structure*>(cell);
> 
> Can you make it more explicit as to what makes this safe for concurrency?  I
> believe that it is the part that pre-fetches the cell before operating on
> it.  

I don't think that's right. A pre-fetch is an instruction that tells the CPU to initiate loading a cache line because it's actually used.

This is nothing special.  It's safe for concurrency because there's only one load of a mutable field.

> How about having the comment say something like:
> 
> // We must pre-fetch the cell from m_previousOrRareData before examining it.
> The m_previousOrRareData field may change on us due to another thread
> writing to it, but the local copy won't.

I'll elaborate that it's because there's only one load of mutable state.

> 
> > Source/JavaScriptCore/runtime/Structure.h:703
> > +    bool isRareData(JSCell* cell) const
> 
> I think this should be a static method instead of a const instance method. 
> You're not accessing any members of the instance here.

No, it calls this->previousID(), which is an instance method.
Comment 8 Mark Lam 2016-06-01 21:19:30 PDT
Comment on attachment 280299 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280299&action=review

>>> Source/JavaScriptCore/runtime/Structure.h:290
>>> +        return static_cast<Structure*>(cell);
>> 
>> Can you make it more explicit as to what makes this safe for concurrency?  I believe that it is the part that pre-fetches the cell before operating on it.  How about having the comment say something like:
>> 
>> // We must pre-fetch the cell from m_previousOrRareData before examining it. The m_previousOrRareData field may change on us due to another thread writing to it, but the local copy won't.
> 
> I don't think that's right. A pre-fetch is an instruction that tells the CPU to initiate loading a cache line because it's actually used.
> 
> This is nothing special.  It's safe for concurrency because there's only one load of a mutable field.

My use of "pre-fetch" wasn't the best choice of words.  I meant the same thing i.e. reading the field only once instead of twice: once for the check, once for the return value.

>>> Source/JavaScriptCore/runtime/Structure.h:703
>>> +    bool isRareData(JSCell* cell) const
>> 
>> I think this should be a static method instead of a const instance method.  You're not accessing any members of the instance here.
> 
> No, it calls this->previousID(), which is an instance method.

Oh ... I see.  Actually, it calls this->structureID() which is the instance method.
Comment 9 Filip Pizlo 2016-06-01 21:25:29 PDT
Landed in http://trac.webkit.org/changeset/201590