Bug 67937 – [Chromium] WebWorkerClientImpl leak in fast/js/instanceof-XMLHttpRequest.html

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Julien Chaffraix]

Upstream bug of http://code.google.com/p/chromium/issues/detail?id=77766.

The analysis is that we are not delete'ing the right context for Chromium as we wrap the WorkerMessagingProxy into a WebWorkerClientImpl.

Patch forthcoming.

[Julien Chaffraix]

Created attachment 107061 [details]
Proposed change: Delete the proper context(s).

[Dmitry Lomov]

I do not think this is the right fix. 

The way chromium port is structured is this:
- WorkerMessagingProxy implements Worker{Context,Object,Loader}Proxies in port-independent way.
- Chromium port wraps this implementation in a way specific to chromium in WebWorkerClientImpl class.
This patch however introduces reverse dependency, where WorkerMessagingProxy now "knows" about it being decorated in chromium.

The better way fix this would be to replicate disposal logic that is implemented in WorkerMessagingProxy (on WorkerObjectDestroyed, post task to worker thread to delete this).

I can own this and work on a patch that implements this

[Dmitry Titov]

Julien, thanks a lot for the analysis!

I am going to remove r? from the patch and assign the bug to Dmitry for the follow up. Please speak up if needed.

[David Levin]

Note: This only affects Chrome 15+ since we just started using this code path with Workers being in process.

[Dmitry Lomov]

(In reply to comment #3)
> Julien, thanks a lot for the analysis!

+1 - sorry for forgetting to acknowledge your hard work!

[Julien Chaffraix]

> The way chromium port is structured is this:
> - WorkerMessagingProxy implements Worker{Context,Object,Loader}Proxies in port-independent way.
> - Chromium port wraps this implementation in a way specific to chromium in WebWorkerClientImpl class.
> This patch however introduces reverse dependency, where WorkerMessagingProxy now "knows" about it being decorated in chromium.

Indeed! I completely missed that.

> The better way fix this would be to replicate disposal logic that is implemented in WorkerMessagingProxy (on WorkerObjectDestroyed, post task to worker thread to delete this).

It makes sense. I fear about the duplication of code. Also understand that in this case, we did not need to post a task on the right thread to delete this. We just go ahead and delete this on the main thread as we did not start the thread.

> I can own this and work on a patch that implements this

Fair enough.

>> Julien, thanks a lot for the analysis!
> +1 - sorry for forgetting to acknowledge your hard work!

Sure, let me know if I can be of help.

[Dmitry Lomov]

(In reply to comment #6)

> > The better way fix this would be to replicate disposal logic that is implemented in WorkerMessagingProxy (on WorkerObjectDestroyed, post task to worker thread to delete this).
> 
> It makes sense. I fear about the duplication of code. Also understand that in this case, we did not need to post a task on the right thread to delete this. We just go ahead and delete this on the main thread as we did not start the thread.

The reason for posting to main thread is that WebWorkerClientImpl destructor (implicitly) destructs RefPtr and this is a thread-affined operation.

[Dmitry Lomov]

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

[Stephen Chenney]

https://code.google.com/p/chromium/issues/detail?id=230619