Bug 67937

Summary: [Chromium] WebWorkerClientImpl leak in fast/js/instanceof-XMLHttpRequest.html
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: WebCore Misc.Assignee: Dmitry Lomov <dslomov>
Status: RESOLVED WONTFIX    
Severity: Normal CC: dimich, dslomov, levin, schenney, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed change: Delete the proper context(s). none

Description Julien Chaffraix 2011-09-12 09:14:07 PDT 
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.
Comment 1 Julien Chaffraix 2011-09-12 10:25:13 PDT 
Created attachment 107061 [details]
Proposed change: Delete the proper context(s).
Comment 2 Dmitry Lomov 2011-09-12 11:49:15 PDT 
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
Comment 3 Dmitry Titov 2011-09-12 12:08:56 PDT 
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.
Comment 4 David Levin 2011-09-12 12:10:31 PDT 
Note: This only affects Chrome 15+ since we just started using this code path with Workers being in process.
Comment 5 Dmitry Lomov 2011-09-12 12:16:37 PDT 
(In reply to comment #3)
> Julien, thanks a lot for the analysis!

+1 - sorry for forgetting to acknowledge your hard work!
Comment 6 Julien Chaffraix 2011-09-12 13:11:26 PDT 
> 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.
Comment 7 Dmitry Lomov 2011-09-16 14:46:00 PDT 
(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.
Comment 8 Dmitry Lomov 2011-10-21 11:39:29 PDT 
*** Bug 70599 has been marked as a duplicate of this bug. ***