Bug 70833 - SerializedScriptValue: lazy initialization of static nullValue not threadsafe
Summary: SerializedScriptValue: lazy initialization of static nullValue not threadsafe
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: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-25 11:53 PDT by Joshua Bell
Modified: 2012-05-16 18:35 PDT (History)
10 users (show)

See Also:


Attachments
Patch (6.43 KB, patch)
2012-05-04 16:43 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2011-10-25 11:53:24 PDT
For the origin of the change:
https://bugs.webkit.org/show_bug.cgi?id=41372#c18

For concerns with the implementation, noted while creating a similar method:
https://bugs.webkit.org/show_bug.cgi?id=60197#c13

Short version - this pattern does not look threadsafe:

Source/WebCore/binding/v8/SerializedScriptValue.cpp(1932):
> SerializedScriptValue* SerializedScriptValue::nullValue()
> {
>     DEFINE_STATIC_LOCAL(RefPtr<SerializedScriptValue>, nullValue, (0));
>     if (!nullValue) {
> 	Writer writer;
>         writer.writeNull();
> 	String wireData = StringImpl::adopt(writer.data());
> 	nullValue = adoptRef(new SerializedScriptValue(wireData));
>     }
>     return nullValue.get();
> }

If this will be called from multiple threads, caching should move to the callers. If it will not be called from multiple threads there should be an assertion. Since SSV is used from Workers, the former is more likely to be true.
Comment 1 David Levin 2011-10-25 13:14:48 PDT
(In reply to comment #0)
> For the origin of the change:
> https://bugs.webkit.org/show_bug.cgi?id=41372#c18
> 
> For concerns with the implementation, noted while creating a similar method:
> https://bugs.webkit.org/show_bug.cgi?id=60197#c13
> 
> Short version - this pattern does not look threadsafe:
> 
> Source/WebCore/binding/v8/SerializedScriptValue.cpp(1932):
> > SerializedScriptValue* SerializedScriptValue::nullValue()
> > {
> >     DEFINE_STATIC_LOCAL(RefPtr<SerializedScriptValue>, nullValue, (0));
> >     if (!nullValue) {
> > 	Writer writer;
> >         writer.writeNull();
> > 	String wireData = StringImpl::adopt(writer.data());
> > 	nullValue = adoptRef(new SerializedScriptValue(wireData));
> >     }
> >     return nullValue.get();
> > }
> 
> If this will be called from multiple threads, caching should move to the callers. If it will not be called from multiple threads there should be an assertion. Since SSV is used from Workers, the former is more likely to be true.

But I don't think this method is used from Workers (At least not by postMessage -- I have no idea about idb).
Comment 2 Joshua Bell 2011-10-25 15:01:57 PDT
(In reply to comment #1)
> 
> But I don't think this method is used from Workers (At least not by postMessage -- I have no idea about idb).

SerializedScriptValue::nullValue() is used by IDB and Worker support for IDB is being added by dgrogan.
Comment 3 anton muhin 2011-10-26 08:50:39 PDT
Guys, I believe Vitaly is an initial implementer and he can provide a lot of details.

I do agree that caching looks somewhat fishy.  I'd rather drop it for now and reintroduce in thread safe manner later if need be.

Vitaly, any thoughts?
Comment 4 Joshua Bell 2012-05-04 16:43:05 PDT
Created attachment 140363 [details]
Patch
Comment 5 Joshua Bell 2012-05-04 16:45:29 PDT
IndexedDB is only using this method from one thread at the moment (even with workers), but that may change soon.

ojan@ - any idea who would be good to review the dom/Document change in this, and/or evaluate the need for caching in call sites?
Comment 6 Ojan Vafai 2012-05-04 16:56:07 PDT
(In reply to comment #5)
> ojan@ - any idea who would be good to review the dom/Document change in this, and/or evaluate the need for caching in call sites?

Best is to look at the blame for the code in question and add whoever modified/reviewed the most recent changes. At first glance, this change looks fine, but the nullValue thing needs explanation from someone who knows why the code was written that way to begin with.
Comment 7 Joshua Bell 2012-05-16 17:25:22 PDT
I followed up with Vitaly - he doesn't recall any performance reason for the caching.

It looks like the source of the non-threadsafe change is:

http://trac.webkit.org/changeset/77558/trunk/Source/WebCore/bindings/v8/SerializedScriptValue.cpp

The old code initialized the value inside the DEFINE_STATIC_LOCAL() macro. The new code wanted do more than a single call, so initialized the static to null then used an (unsafe) if (!nullValue) { ... } block to do the initialization.

An alternative to my patch would be to introduce a createNull() method that is non-threadsafe and call it from DEFINE_STATIC_LOCAL(). However, caching could just as easily move to the callers.

haraken@ - could you review the change?
Comment 8 Kentaro Hara 2012-05-16 17:33:45 PDT
Comment on attachment 140363 [details]
Patch

Let me r+ it based on the following observations:

1. "I followed up with Vitaly - he doesn't recall any performance reason for the caching."
2. booleanValue() and numberValue() are not caching the values.

If there were any performance concern, it would be best to cache the value in V8BindingsPerIsolateData (You can access V8BindingsPerIsolateData just by isolate->GetData()).
Comment 9 WebKit Review Bot 2012-05-16 18:35:26 PDT
Comment on attachment 140363 [details]
Patch

Clearing flags on attachment: 140363

Committed r117377: <http://trac.webkit.org/changeset/117377>
Comment 10 WebKit Review Bot 2012-05-16 18:35:32 PDT
All reviewed patches have been landed.  Closing bug.