Bug 135060 – Reduce the chances of a race condition when sharing SharedBuffer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Pratik Solanki]

We currently pass a SharedBuffer to ImageIO by creating a WebCoreSharedBufferData. This means ImageIO will access the SharedBuffer on a separate thread and this can cause data corruption bugs. While the actual fix is complex, we can at least reduce the time interval for this race condition to happen by setting the ShardBuffer size after the data is copied in SharedBuffer::append().

<rdar://problem/17729444>

[Pratik Solanki]

Created attachment 235136 [details]
Patch

[Geoffrey Garen]

Comment on attachment 235136 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        cector has finished appending.

cector => vector

> Source/WebCore/platform/SharedBuffer.cpp:362
> -    m_size += length;
>      if (m_buffer.isEmpty())
>          m_buffer.reserveInitialCapacity(length);
>      m_buffer.append(data, length);
> +    m_size += length;

What is the plan for fixing the race condition?

It's hard to evaluate whether this change is right. If you wanted to make this data thread-consistent, you would probably do something like a store barrier before setting m_size. It's not really coherent to move the setting of m_size because, in an SMP environment, all code runs out of order anyway.

[Pratik Solanki]

(In reply to comment #2)
> (From update of attachment 235136 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235136&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        cector has finished appending.
> 
> cector => vector
> 
> > Source/WebCore/platform/SharedBuffer.cpp:362
> > -    m_size += length;
> >      if (m_buffer.isEmpty())
> >          m_buffer.reserveInitialCapacity(length);
> >      m_buffer.append(data, length);
> > +    m_size += length;
> 
> What is the plan for fixing the race condition?

<rdar://17470655> tracks this. I also filed <https://bugs.webkit.org/show_bug.cgi?id=135069>.

> It's hard to evaluate whether this change is right. If you wanted to make this data thread-consistent, you would probably do something like a store barrier before setting m_size. It's not really coherent to move the setting of m_size because, in an SMP environment, all code runs out of order anyway.

I am not trying to fix the actual race condition - merely trying to reduce the time where it can happen. Testing indicates that the fix helps. It is possible that the compiler could reorder the instructions but my testing shows improvements from previous behavior. At the very least, this change should not make anything worse than before.

[Pratik Solanki]

So, would it be okay to make this change? Or should the patch be r-?

[Darin Adler]

What’s the timeline on the real fix? I’m not sure making this tweak to reduce the frequency of the bug’s symptom is a good thing to do, but it’s hard to see how it would do harm.

[Darin Adler]

Comment on attachment 235136 [details]
Patch

Lets get this change in even though it’s not a real fix. I’m still not sure about the strategy; it seems strange to do this instead of fixing the bug.

[WebKit Commit Bot]

Comment on attachment 235136 [details]
Patch

Clearing flags on attachment: 235136

Committed r171289: <http://trac.webkit.org/changeset/171289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Pratik Solanki]

(In reply to comment #5)
> What’s the timeline on the real fix? I’m not sure making this tweak to reduce the frequency of the bug’s symptom is a good thing to do, but it’s hard to see how it would do harm.

I have the fix almost working - I am running into issues with custom fonts that I am trying to figure out. The bug I see is identical to what was seen in bug 115131. CGFont/CGFontDataProvider is not happy with my code changes in WebCoreSharedBufferData.

(In reply to comment #6)
> (From update of attachment 235136 [details])
> Lets get this change in even though it’s not a real fix. I’m still not sure about the strategy; it seems strange to do this instead of fixing the bug.

This was a quick change that at least makes images load. We should have a clean way of sharing data with ImageIO but my changes started looking risky - not to mention the perf impact it might have due to more copying. I'll upload my wip patches to bug 135069 while I continue testing.

Thanks for the review!