Bug 99178 – [JSC] DOM URL is flaky when workers are used

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Allan Sandfeld Jensen]

When the DOM URL Constructor is used in both a main thread and a worker thread, the results becomes unstable. Specifically the methods will be likely to become undefined in one of the contexts (for JSC in V8 it seems to crash).

The reason seems to be that HashTable::createTable in Lookup.h registers the method names, only in the current context, once another context accesses the same table, it will be unable to lookup the identifiers used to access the methods.

[Allan Sandfeld Jensen]

Created attachment 168420 [details]
TestCase

A simple test-case. Window.URL.createObjectURL will be undefined in the main-thread because the table for JSDOMURL was created in the worker thread context.

[Allan Sandfeld Jensen]

Besides the attached test-case, this bug affects the existing tests fast/files/workers/worker-apply-blob-url-to-xhr.html and fast/files/workers/worker-read-blob-async.html

[Allan Sandfeld Jensen]

Created attachment 168423 [details]
Patch

A proof-of-concept patch. The patch solves the issue, but I am guessing this solution could cause preformance regressions. Putting it for review anyway to get feedback on this tricky issue.

[Allan Sandfeld Jensen]

Clarifying the descriptions. What happens is that the method names such "createObjectURL" becomes different StringImpl pointers in different threads. This is probably a good idea since StringImpl is not thread-safe. Unfortunately the HashTable uses the StringImpl pointer as the key in it table. This means it will fail to find the correct methods when asked to do a lookup with a StringImpl* from another thread than the one the table was instantiated in.

The reason it causes flakiness is one, that the table is instantiated lazy, and two, that these tables can survive page-reload. Making it depend on both timing and what has been run before.

[Geoffrey Garen]

Your instinct is right that comparing Strings instead of StringImpl*s is a performance issue.

Typically, we solve threading problems with strings by making an isolatedCopy() before moving a string to another thread. Can you explain why that technique doesn't work in this case?

[Allan Sandfeld Jensen]

(In reply to comment #5)
> Your instinct is right that comparing Strings instead of StringImpl*s is a performance issue.
> 
> Typically, we solve threading problems with strings by making an isolatedCopy() before moving a string to another thread. Can you explain why that technique doesn't work in this case?

There are no string moved from one thread to another here. This is a special case of a DOM object that is shared between different threads. As far as I can tell there are almost no other global DOM objects shared the same way, the only ones I could find with a quick scan were the element factories and the Notifications API.

[Geoffrey Garen]

> There are no string moved from one thread to another here. This is a special case of a DOM object that is shared between different threads.

OK. It's invalid in WebKit to share a DOM object between threads. In addition to this specific JavaScript issue, all AtomicString usage will be broken, string reference counting will be broken, and reference counting of the DOM url object will be broken.

[Allan Sandfeld Jensen]

(In reply to comment #7)
> > There are no string moved from one thread to another here. This is a special case of a DOM object that is shared between different threads.
> 
> OK. It's invalid in WebKit to share a DOM object between threads. In addition to this specific JavaScript issue, all AtomicString usage will be broken, string reference counting will be broken, and reference counting of the DOM url object will be broken.

Yeah, it thought it was kinda of crazy too, but since is the first time I am in contact with this code I do not know what thoughts went into the current design. The commit that changed it from being dynamically allocated in the workers, state that it was changed to being a static object due to the specification; but I don't think that necessarily should require us to share the method tables between all users of the object.

[Alexey Proskuryakov]

Comment on attachment 168423 [details]
Patch

As you are new to this code, I'll also mention that JSNoStaticTables is what prevents sharing tables between objects and workers and ones on main thread.

Sharing a single object just isn't going to work for more reasons that just method tables, as Geoff explained.

[Allan Sandfeld Jensen]

(In reply to comment #9)
> (From update of attachment 168423 [details])
> As you are new to this code, I'll also mention that JSNoStaticTables is what prevents sharing tables between objects and workers and ones on main thread.
> 

From inspecting the generated code it looks like JSNoStaticTables ensures that JSDOMURL::getOwnPropertySlot goes through separate tables for each GlobalData. The problem is then that these methods are accessed through JSDOMURLConstructor::getOwnPropertySlot which does not look up separate tables for each GlobalDAta.

Compare:

bool JSDOMURLConstructor::getOwnPropertySlot(JSCell* cell, ExecState* exec, PropertyName propertyName, PropertySlot& slot)
{
    return getStaticPropertySlot<JSDOMURLConstructor, JSDOMWrapper>(exec, &JSDOMURLConstructorTable, jsCast<JSDOMURLConstructor*>(cell), propertyName, slot);
}

bool JSDOMURL::getOwnPropertySlot(JSCell* cell, ExecState* exec, PropertyName propertyName, PropertySlot& slot)
{
    JSDOMURL* thisObject = jsCast<JSDOMURL*>(cell);
    ASSERT_GC_OBJECT_INHERITS(thisObject, &s_info);
    return getStaticValueSlot<JSDOMURL, Base>(exec, getJSDOMURLTable(exec), thisObject, propertyName, slot);
}

If I instrument the source-code, I never see calls to JSDOMURL::getOwnPropertySlot, but accessing URL.createObjectURL will instead access JSValue JSDOMURL::getConstructor and then  JSDOMURLConstructor::getOwnPropertySlot.

[Allan Sandfeld Jensen]

Created attachment 168658 [details]
Patch

[Allan Sandfeld Jensen]

The attached patch trivially extends the JSNoStaticTable mechanism to also create separate static tables for constructor objects, and it solves this bug, but I need someone more experienced in this code to tell if there was a reason this had not already been done to constructor objects, or if it was just an oversight.

[Alexey Proskuryakov]

Historically, this was an oversight - I know because I wrote it. But I'm not qualified enough to review.

[Eric Seidel (no email)]

I'm certain one of the JSC ninjas can set you straight here.

[Allan Sandfeld Jensen]

Ping, are none of JSC reviewers among the CC'ed able to review this patch?

[Eric Seidel (no email)]

Add Papa JSC.

[Geoffrey Garen]

Comment on attachment 168658 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 168658 [details]
Patch

Clearing flags on attachment: 168658

Committed r132973: <http://trac.webkit.org/changeset/132973>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #18)
> (From update of attachment 168658 [details])
> Clearing flags on attachment: 168658
> 
> Committed r132973: <http://trac.webkit.org/changeset/132973>

It broke the bindings tests. Could you fix it?

[Allan Sandfeld Jensen]

Needs to have a fixup on the bindings tests, since they compare generated CPP output instead of functionality change.

[Allan Sandfeld Jensen]

Committed r133007: <http://trac.webkit.org/changeset/133007>

[Roger Fong]

Test fails on Apple Windows port.
-PASS: window.URL.createObjectURL defined
-PASS: window.URL.revokeObjectURL defined
-PASS: URL.createObjectURL defined
-PASS: URL.revokeObjectURL defined
-PASS: window.URL.createObjectURL defined
-PASS: window.URL.revokeObjectURL defined
+CONSOLE MESSAGE: line 28: TypeError: 'undefined' is not an object (evaluating 'window.URL.createObjectURL')
+FAIL: Timed out waiting for notifyDone to be called
 
Looks like window.URL is null? I can't test further because DRT seems to be broken on Windows right now :/

[Zan Dobersek]

*** Bug 100136 has been marked as a duplicate of this bug. ***

[Allan Sandfeld Jensen]

(In reply to comment #23)
> Test fails on Apple Windows port.
> -PASS: window.URL.createObjectURL defined
> -PASS: window.URL.revokeObjectURL defined
> -PASS: URL.createObjectURL defined
> -PASS: URL.revokeObjectURL defined
> -PASS: window.URL.createObjectURL defined
> -PASS: window.URL.revokeObjectURL defined
> +CONSOLE MESSAGE: line 28: TypeError: 'undefined' is not an object (evaluating 'window.URL.createObjectURL')
> +FAIL: Timed out waiting for notifyDone to be called
> 
> Looks like window.URL is null? I can't test further because DRT seems to be broken on Windows right now :/

Maybe you do not have BLOB enabled? In that case you need to skip this test.

[Roger Fong]

Ah yup, that would be it. Thanks, I'll add it on.