Bug 67937 - [Chromium] WebWorkerClientImpl leak in fast/js/instanceof-XMLHttpRequest.html
Summary: [Chromium] WebWorkerClientImpl leak in fast/js/instanceof-XMLHttpRequest.html
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dmitry Lomov
URL:
Keywords:
: 70599 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-09-12 09:14 PDT by Julien Chaffraix
Modified: 2013-04-11 15:15 PDT (History)
5 users (show)

See Also:


Attachments
Proposed change: Delete the proper context(s). (4.50 KB, patch)
2011-09-12 10:25 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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. ***