Bug 99178

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.

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

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.

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.

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?

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

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

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

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.

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

Created attachment 168658 [details] Patch

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.

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

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

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

Add Papa JSC.

Comment on attachment 168658 [details] Patch r=me

Comment on attachment 168658 [details] Patch Clearing flags on attachment: 168658 Committed r132973: <http://trac.webkit.org/changeset/132973>

All reviewed patches have been landed. Closing bug.

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

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

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

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 :/

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

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

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