[webkit-reviews] review denied: [Bug 36213] Assertion failure when inspecting a page with workers : [Attachment 51018] Move worker destruction notification from AbstractWorker destructor to worker's proxy async methods so these are not called from within JS GC.

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 36213: Assertion failure when inspecting a page with workers
https://bugs.webkit.org/show_bug.cgi?id=36213

Attachment 51018: Move worker destruction notification from AbstractWorker
destructor to worker's proxy async methods so these are not called from within
JS GC.
https://bugs.webkit.org/attachment.cgi?id=51018&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
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.