Assertion failure when inspecting a page with workers

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Yury Semikhatsky]

ASSERTION FAILED: m_heap.operationInProgress == NoOperation
(/Users/yurys/WebKitGit/JavaScriptCore/runtime/Collector.cpp:398 void* JSC::Heap::allocate(size_t))

Steps to reproduce:
1. In Debug build navigate to apple.com
2. Open Web Inspector, make sure scripts tab is selected and debugging is on.
3. Navigate to a page with workers

Result:
Pages hangs with the assertion failure.

[Andrey Kosyakov]

Created attachment 51018 [details]
Move worker destruction notification from AbstractWorker destructor to worker's proxy async methods so these are not called from within JS GC.

[Andrey Kosyakov]

David,
could you please have a look at the correctness of this from the worker lifecycle prospective? I had to move inspector notification away from worker destructor, as it would ultimately cause attempts to execute JS, which hits JSC reenterability problem  by trying to allocate on JSC heap while another heap operation (which caused workers destruction) is in progress.

[Dmitry Titov]

There is an unfortunate proliferation of id()... At a minimum it'd be nice to give those ids somewhat more descriptive name, like inspectorId(). Also, why don't we just use an address of the object (casted to void*) as an id? It seems it is used as a key in some map anyways.

If it's possible to just use the address as id, then no wiring and storing of id is needed at all...

[Dmitry Titov]

Comment on attachment 51018 [details]
Move worker destruction notification from AbstractWorker destructor to worker's proxy async methods so these are not called from within JS GC.

I can look at it from worker's perspective...

In general, the change is in the right direction because those "proxy" classes control lifetime of workers and their contexts.

However, this patch will work for Dedicated workers, but perhaps not for Shared ones... The Worker object and WorkerContext have different lifetime - former is bounded by GC, while the latter can terminate earlier (via WorkerContext.close() or later (especially true for shared workers). I'm not sure which lifetime you want to capture. It might be that Worker object is still alive but context is destroyed ("worker is terminated"). Also, the worker context of a shared worker can continue 'working' while the SharedWorker object is collected, if there is another SharedWorker object somewhere connected to the same worker. 

WorkerMessagingProxy is dealing with DedicatedWorkers. Shared ones are controlled by SharedWorkerRepository-derived objects.

Also, it seems it should be named "didDestroyWorker" since both the worker and worker context can be destroyed by the time this call is made.

> Index: WebCore/ChangeLog

The ChangeLog says basically nothing about the change. It'd be better to get some enough info on "why" and "what" changes.
That way it's easier to understand what was changing in the project while looking at ChangeLog or (more often) trac.webkit.org.

I think that id() is also not needed - simply passing 'this' as Id could suffice.It would be nice to remove the id field.

I'm going to r- because it seems the patch needs updating.

[Andrey Kosyakov]

Created attachment 51283 [details]
Send worker creation/destruction notifications to inspector frontend asynchronously

Dmitry,
After an offline discussion with pfeldman I decided to take slightly different approach -- I'm leaving workers side intact and moving asynchronous tasks creation/execution to the inspector code (I think this is fair, as the problem we're trying to resolve appears because inspector front-end executes JS to deliver these notifications).
I also removed explicit id field from workers (please note it wouldn't be a good idea with old design, as we used to create workers synchronously but destroy asynchronously, which would create problems in case same address is re-used when another worker is created. It's not a problem now, as creation notifications are also async and hence serialized with destruction).
Pre your suggestion, willDestroyWorker() is renamed to didDestroyWorker() (old name was appropriate for old version, but clearly isn't now).
Obviously, this version only tracks client-side worker objects, but this should be sufficient for inspector needs -- we only care for objects that inspected page is explicitly using, so the fact that a shared worker may exist after it's not referenced by the page is not so important.

[Dmitry Titov]

Comment on attachment 51283 [details]
Send worker creation/destruction notifications to inspector frontend asynchronously

Awesome, thanks a lot!

> Index: WebCore/inspector/InjectedScriptHost.h
> -    void willDestroyWorker(long id);
> +    void didDestroyWorker(long id);

These might need to be "long long" now, to be able to roundtrip the intptr_t on 64-bit. "long long" is supported by idl and JSValue.

> Index: WebCore/inspector/InspectorController.cpp

>  #if ENABLE(WORKERS)
> -void InspectorController::didCreateWorker(long id, const String& url, bool isSharedWorker)
> +class PostWorkerNotificationToFrontendTask : public ScriptExecutionContext::Task {
> +public:
> +    static PassOwnPtr<PostWorkerNotificationToFrontendTask> create(RefPtr<InspectorWorkerResource> worker, bool isCreated)

The parameter should be PassRefPtr. (http://webkit.org/coding/RefPtr.html)

Also, normally WebKit style is to create and use an enum rather then bool parameter - this makes callsite more readable. In this case, there could be WorkerCreated and WorkerDestroyed values.

> +    PostWorkerNotificationToFrontendTask(RefPtr<InspectorWorkerResource> worker, bool isCreated)

Also should be PassRefPtr.

> +void InspectorController::postWorkerNotificationToFrontend(RefPtr<InspectorWorkerResource> worker, bool isCreated)

Here, just raw pointer InspectorWorkerResource* would be just fine since this function does not take ownership or affects lifetime of the passed resource and relies on the caller to keep it alive.

> +void InspectorController::didCreateWorker(intptr_t id, const String& url, bool isSharedWorker)

I would add another note about bool parameters and named enum values but this is code that already existed before the patch, so no need to change it. :-)

I'd r+ with "fix it on landing" note but it feels there are a few possible changes so let me r- and look forward for an updated patch!

[Andrey Kosyakov]

Created attachment 51421 [details]
Send worker creation/destruction notifications to inspector frontend asynchronously

Dmitry,
thanks for the feedback! I've implemented most of your recommendations with the exception of "long long" for IDs in InjectedScriptHost -- the idea is, these IDs are for a different type of workers actually -- the "fake" ones, implemented in JS, that we inject when user attempts to debug workers. These IDs never store anything that we get from AbstractWorker::asID() and are rather generated on the inspector side (InjectedScriptHost::nextWorkerId()), so their life-cycle is 
InjectedScriptHost -> JS fake workers implementation -> InspectorController -> InspectorFrontend and hence they need to use an integral type that is not wider than one used in InsectorController & InspectorFrontend. Using long long would actually cause truncation on 32-bit platforms (assuming sizeof(intptr_t) == 4 && sizeof(long long) == 8). This is still a bit fishy for the case of sizeof(long) > sizeof(intptr_t), but I believe it only happens on 16 bit platforms, so we hardly care.

[Dmitry Titov]

Comment on attachment 51421 [details]
Send worker creation/destruction notifications to inspector frontend asynchronously

Looks good! 

One tiny nit:

> +void InspectorController::postWorkerNotificationToFrontend(const InspectorWorkerResource& worker, InspectorController::WorkerAction action)
> +{
> +    if (!m_frontend)
> +        return;
> +    switch (action) {
> +    case InspectorController::WorkerCreated:
> +        m_frontend->didCreateWorker(worker);
> +        break;
> +    case InspectorController::WorkerDestroyed:
> +        m_frontend->didDestroyWorker(worker);
> +        break;
> +    default:
> +        ASSERT_NOT_REACHED();

WebKit usually does not have this 'default' case since if the value is enum and there is no 'default', the compiler verifies all values of the enum are used. If there is desire to check for some integer value outside of enum range, the 'brake' could be replaced by 'return' and the ASSERT_NOT_REACHED() added after the whole switch. This way, both checks are active.

Also: if you want to use the commit-queue, mark the corresponding flag as '?' as well, this way the reviewer could see your intention to use the bot and flip it to + with r+.

[Andrey Kosyakov]

Created attachment 51437 [details]
Send worker creation/destruction notifications to inspector frontend asynchronously

[Dmitry Titov]

Comment on attachment 51437 [details]
Send worker creation/destruction notifications to inspector frontend asynchronously

r=me

[WebKit Commit Bot]

Comment on attachment 51437 [details]
Send worker creation/destruction notifications to inspector frontend asynchronously

Clearing flags on attachment: 51437

Committed r56404: <http://trac.webkit.org/changeset/56404>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.