Bug 38623 – JSCallbackData is deleted on the Main thread, not the Context thread.

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

[Eric U.]

JSCallbackData objects should always be deleted on the thread that created them, which is the context thread.  For the Document, they're the same thing, but when Workers get Database access, this causes problems.  Also, we need to make sure that their deletion happens before the WorkerThread's runloop exits.

This was the bug that caused the GTK bots to assert when the async-db-on-workers patch went in.  Once this fix is in, we can try submitting that patch again.

[Eric U.]

Created attachment 55196 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 55196 [details]
Patch

WebCore/bindings/js/JSCallbackData.h:69
 +      ThreadIdentifier m_thread;
Does this size increase have any negative effects we should be aware of?

WebCore/bindings/js/JSCallbackData.h:74
 +      static PassOwnPtr<DeleteCallbackDataTask> create(JSCallbackData *data)
Data* data, style error.

WebCore/bindings/js/JSCallbackData.h:79
 +      virtual void performTask(ScriptExecutionContext *)
Context*, style error.

WebCore/bindings/js/JSCallbackData.h:86
 +      DeleteCallbackDataTask(JSCallbackData *data) : m_data(data) {}
Data* data, style error.

WebCore/bindings/js/JSCallbackData.h:88
 +      JSCallbackData *m_data;
Data* m_data, style error.


WebCore/bindings/js/JSCustomVoidCallback.cpp: 
 +      callOnMainThread(JSCallbackData::deleteData, m_data);
I'm confused.  Callbacks can end up executing on any thread?  But the data may not have been allocated on that same thread?

WebCore/bindings/js/JSCustomVoidCallback.h:53
 +      JSCustomVoidCallback(JSC::JSObject* callback, JSDOMGlobalObject*);
Was it previously ignoring the JSDOMGlobalObject* and not storing it anywhere?

WebCore/bindings/js/JSCustomVoidCallback.h:56
 +      ScriptExecutionContext* m_scriptExecutionContext;
I'm surprised we need this additional pointer and that we don't already store the execution context in some manner.

WebCore/bindings/scripts/CodeGeneratorJS.pm:1975
 +      push(@headerContent, "    ScriptExecutionContext* m_scriptExecutionContext;\n");
How is the execution context separate from the isolated world?  I believe you it is.  I'm just slightly surprised we can't derive one from the other, since the isolated world (last I knew) was tied to the global object.  And the ExecutionContext shoudl also be tied to the global object, or no?

WebCore/bindings/scripts/CodeGeneratorJS.pm:2006
 +      push(@implContent, "    , m_isolatedWorld(globalObject->world())\n");
Yeah, I  guess I'm confused why we don't just store a global object pointer.  In fact, I would expect we might already do so.  Most DOM bindings carry around a global object pointer...

I'm just not convinced this change is 100% right yet, but that's more of a feeling than fact since my JSC knowledge is a bit dated these days.

[Eric U.]

(In reply to comment #2)
> (From update of attachment 55196 [details])
> WebCore/bindings/js/JSCallbackData.h:69
>  +      ThreadIdentifier m_thread;
> Does this size increase have any negative effects we should be aware of?

I'd be really surprised if it did, but just in case I've wrapped it in NDEBUG.

I fixed all the style errors--sorry about that; was posting a patch as I ran out the door, always a bad idea.

> WebCore/bindings/js/JSCustomVoidCallback.cpp: 
>  +      callOnMainThread(JSCallbackData::deleteData, m_data);
> I'm confused.  Callbacks can end up executing on any thread?  But the data may
> not have been allocated on that same thread?

No, they're always allocated and executed on the context thread.  But they may get deleted on the Database thread, so we have to post to the context thread at cleanup time.

> WebCore/bindings/js/JSCustomVoidCallback.h:53
>  +      JSCustomVoidCallback(JSC::JSObject* callback, JSDOMGlobalObject*);
> Was it previously ignoring the JSDOMGlobalObject* and not storing it anywhere?

Right.  Well, sort of--it gets stashed inside the JSCallbackData, but we don't store it directly.

> WebCore/bindings/js/JSCustomVoidCallback.h:56
>  +      ScriptExecutionContext* m_scriptExecutionContext;
> I'm surprised we need this additional pointer and that we don't already store
> the execution context in some manner.
>
> WebCore/bindings/scripts/CodeGeneratorJS.pm:1975
>  +      push(@headerContent, "    ScriptExecutionContext*
> m_scriptExecutionContext;\n");
> How is the execution context separate from the isolated world?  I believe you
> it is.  I'm just slightly surprised we can't derive one from the other, since
> the isolated world (last I knew) was tied to the global object.  And the
> ExecutionContext shoudl also be tied to the global object, or no?

The JSCallbackData's globalObject has a SEC pointer, but we can't trust it.  If the page is being torn down, it can get nulled out before we need it.  I'm not sure exactly when, but I was able to crash some tests trying it out.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:2006
>  +      push(@implContent, "    , m_isolatedWorld(globalObject->world())\n");
> Yeah, I  guess I'm confused why we don't just store a global object pointer. 
> In fact, I would expect we might already do so.  Most DOM bindings carry around
> a global object pointer...
> 
> I'm just not convinced this change is 100% right yet, but that's more of a
> feeling than fact since my JSC knowledge is a bit dated these days.

[Eric U.]

Created attachment 55282 [details]
Patch

[Dmitry Titov]

Comment on attachment 55282 [details]
Patch

WebCore/workers/WorkerThread.cpp:168
 +          workerContext->clearScript();
This is very intentional move of a line.  I guess it needs to be here to let all the destructor tasks to run before destroying the heap... Could you put some descriptive comment to this effect if that's indeed why you move this line here? It makes it very important to keep it here.

[Eric U.]

Created attachment 55317 [details]
Patch

[Eric U.]

Comment on attachment 55317 [details]
Patch

Added a comment as per Dmitry's request.

[Dmitry Titov]

Comment on attachment 55317 [details]
Patch

Looks reasonable. r=me.

[WebKit Commit Bot]

Comment on attachment 55317 [details]
Patch

Clearing flags on attachment: 55317

Committed r58998: <http://trac.webkit.org/changeset/58998>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.