Bug 158280 – Structure::previousID() races with Structure::allocateRareData()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 280275 [details]
possibly the patch

I haven't benchmarked it yet.

[WebKit Commit Bot]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

Ran again, and the regression disappeared.  Nonetheless, I have a better approach, patch coming soon.

[Filip Pizlo]

Created attachment 280299 [details]
the patch

[Mark Lam]

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.

[Filip Pizlo]

(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.

[Mark Lam]

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.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/201590