Bug 70833 – SerializedScriptValue: lazy initialization of static nullValue not threadsafe

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

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.

[David Levin]

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

[Joshua Bell]

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

[anton muhin]

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?

[Joshua Bell]

Created attachment 140363 [details]
Patch

[Joshua Bell]

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?

[Ojan Vafai]

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

[Joshua Bell]

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?

[Kentaro Hara]

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

[WebKit Review Bot]

Comment on attachment 140363 [details]
Patch

Clearing flags on attachment: 140363

Committed r117377: <http://trac.webkit.org/changeset/117377>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.