Bug 135060 - Reduce the chances of a race condition when sharing SharedBuffer
Summary: Reduce the chances of a race condition when sharing SharedBuffer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pratik Solanki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-18 12:15 PDT by Pratik Solanki
Modified: 2014-07-20 23:16 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.05 KB, patch)
2014-07-18 12:25 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2014-07-18 12:15:13 PDT
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>
Comment 1 Pratik Solanki 2014-07-18 12:25:32 PDT
Created attachment 235136 [details]
Patch
Comment 2 Geoffrey Garen 2014-07-18 13:02:38 PDT
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.
Comment 3 Pratik Solanki 2014-07-18 13:55:38 PDT
(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.
Comment 4 Pratik Solanki 2014-07-18 18:23:30 PDT
So, would it be okay to make this change? Or should the patch be r-?
Comment 5 Darin Adler 2014-07-19 22:49:45 PDT
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.
Comment 6 Darin Adler 2014-07-20 20:58:50 PDT
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.
Comment 7 WebKit Commit Bot 2014-07-20 21:30:38 PDT
Comment on attachment 235136 [details]
Patch

Clearing flags on attachment: 235136

Committed r171289: <http://trac.webkit.org/changeset/171289>
Comment 8 WebKit Commit Bot 2014-07-20 21:30:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Pratik Solanki 2014-07-20 23:16:35 PDT
(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!