Bug 35757 – [v8] Remove wrong assert in GC callback

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dmitry Titov]

The Workers in debug Chromuim crash when v8 decides to GC. It is a regression caused by http://trac.webkit.org/changeset/50598. It assumed V8 only runs on main thread, while workers in worker process run on a separate worker thread.
This code needs more cleanup (there are chunks of unnecessary (although probably benign) code stuck in there since old attempts to run workers on separate threads using v8 Locker. We don't do this anymore so some cleanup is in order. For now, I'll remove assert to fix the crash.

[Dmitry Titov]

Created attachment 50039 [details]
Patch.

[Adam Barth]

+antonm for an opinion

[Dmitry Titov]

More info: Jian (mostly) and I initially created the "delayed deref" maps because we were trying to use v8 in a renderer process on separate threads with v8 Lockers. That didn't really work because of GC issues and because it essentially serialized JS execution which was bad for UI thread and isn't very efficient. So we abandoned this in favor of one worker per process model, for now.

In the Worker process, the worker is running not on a main thread, but on a separate worker thread. V8 is not used on main thread in Worker process. But DOM Objects (like Event) are created and they have wrappers. Most of our worker tests are not long enough to make v8 to GC. But when it happens, it happens on a worker thread. Hence the crash in debug builds that manifests itself as flakiness of worker tests.

I'm preparing a cleanup patch to remove all the delayed deref code. It isn't used now and we'll not likely to use it in the future.

[anton muhin]

(In reply to comment #3)
> More info: Jian (mostly) and I initially created the "delayed deref" maps
> because we were trying to use v8 in a renderer process on separate threads with
> v8 Lockers. That didn't really work because of GC issues and because it
> essentially serialized JS execution which was bad for UI thread and isn't very
> efficient. So we abandoned this in favor of one worker per process model, for
> now.
> 
> In the Worker process, the worker is running not on a main thread, but on a
> separate worker thread. V8 is not used on main thread in Worker process. But
> DOM Objects (like Event) are created and they have wrappers. Most of our worker
> tests are not long enough to make v8 to GC. But when it happens, it happens on
> a worker thread. Hence the crash in debug builds that manifests itself as
> flakiness of worker tests.
> 
> I'm preparing a cleanup patch to remove all the delayed deref code. It isn't
> used now and we'll not likely to use it in the future.

Dmitry, just to check I understand it correctly.

You say that in Worker process V8 never runs on main thread.  And I assume that at least currently it always runs on some dedicated thread (otherwise things go hairy).

If it's the case, could we introduce notion of V8 thread and keep the ASSERT turning it into  ASSERT(v8Thread() == WTF::currentThread())?

[Dmitry Titov]

(In reply to comment #4)
> Dmitry, just to check I understand it correctly.
> 
> You say that in Worker process V8 never runs on main thread.  And I assume that
> at least currently it always runs on some dedicated thread (otherwise things go
> hairy).

That's exactly right.

> If it's the case, could we introduce notion of V8 thread and keep the ASSERT
> turning it into  ASSERT(v8Thread() == WTF::currentThread())?

There is an assert a couple lines below in the same function that checks we are on the same thread as the DOMData object:
ASSERT(store->domData()->owningThread() == WTF::currentThread());

If we pull it couple of lines higher, it seems it could serve the purpose here since the important thing here is to check that we are on the same thread as we were when wrappers were created... Updated patch coming.

Having a notion of a V8 thread is an interesting idea (something similar to ScritpExecutionContext::isContextThread()). I can add that in a separate patch (since it would involve replacement of various checks for main thread and DOMData::owningThread() in a few places).

Also, if we ever get more then one V8 instance in the process (it might happen sooner then a teleporter, which is another secret project :-), then perhaps it should not be a global function... But we can solve this later.

[anton muhin]

I am fine with any version.  Just an observation regarding ASSERT(store->domData()->owningThread() == WTF::currentThread()), pedantic one.

That doesn't protect as from multithreading---we might be lucky to grab the right object on the right thread, but without proper synchronization it's russian roulette.

But anyway, as I said above, I am fine with the change, esp. as you're going to work on this area.

[Dmitry Titov]

Comment on attachment 50039 [details]
Patch.

Obsoleting the patch for now, another one is comign (I'm trying to add a specific test)

[Dmitry Titov]

Created attachment 50111 [details]
Patch with a test.

Added test and moved another ASSERT up as per discussion.

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/55608

[Dmitry Titov]

Removed r+ from last patch so it doesn't show in landing queue.