Delete UDPClientSocket on same thread as creation. Also refactor.

Delete UDPClientSocket on same thread as creation. Also refactor.

I investigated deriving from IOThreadExtensionFunction, but we need access to Profile in the midst of our work. Deriving from UIThreadExtensionFunction appears to be the right way to do that. It's possible that we'll want to address performance issues caused by jumping back and forth between threads, but for now this seems to work well enough.

See http://code.google.com/p/chromium/issues/detail?id=106802 for future plans to deal with lifetime more elegantly.

Also reapply http://codereview.chromium.org/8819029/, which is what led to this bug/fix.

BUG=106656
TEST=no new ones; just refactoring.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113710

[Aaron Boodman, 2011-12-07 23:16:22]

As I mentioned on IRC, I'm suspicious of any object with Profile's lifetime. It
usually turns out to be a PITA.

I think it's worth investigating whether the bookeeping could move into
socket.cc and whether socket_controller.cc could be eliminated. But overall it
is a minor issue, and there are certainly lots of other examples of this pattern
already.

If you decide to investigate this, it seems like you could handle cleanup like
so:

1) Redesign API to use a callback chain. When callback chain goes to zero,
object is dead and socket gets deleted. This is the normal case.

2) When a renderer exits (either normally or abnormally), close any sockets
still open and associated with it. This will require keeping a mapping of
extension ids to sockets, but you'll need that anyway (you don't want to let
apps access each other's sockets).

3) I believe that only leaves abnormal Chrome shutdown (I think that all
renderers shutdown when existing Chrome normally). If not, you could have a case
for normal Chrome shutdown too.

http://codereview.chromium.org/8857004/diff/3001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/socket/socket_api_controller.cc (right):

http://codereview.chromium.org/8857004/diff/3001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/socket/socket_api_controller.cc:108:
STLDeleteValues(&socket_map_);
You can use std::map<int, linked_ptr<Socket>>. Then you won't need this.

http://codereview.chromium.org/8857004/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8857004/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:486: void
ExtensionService::DeleteSocketController(
I believe that you can use MessageLoop::DeleteSoon

http://codereview.chromium.org/8857004/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/8857004/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.h:830: extensions::SocketController*
socket_controller_;
Add a comment explaining why this is a bare pointer.

[miket_OOO, 2011-12-08 00:29:21]

Made changes (except the DeleteSoon one), uploaded.

http://codereview.chromium.org/8857004/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8857004/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:486: void
ExtensionService::DeleteSocketController(
On 2011/12/07 23:16:22, Aaron Boodman wrote:
> I believe that you can use MessageLoop::DeleteSoon

If I'm reading the documentation correctly, this won't guarantee that my delete
is called on the IO thread, which is what I need.

[Aaron Boodman, 2011-12-08 00:41:50]

http://codereview.chromium.org/8857004/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8857004/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:486: void
ExtensionService::DeleteSocketController(
On 2011/12/08 00:29:22, miket wrote:
> On 2011/12/07 23:16:22, Aaron Boodman wrote:
> > I believe that you can use MessageLoop::DeleteSoon
> 
> If I'm reading the documentation correctly, this won't guarantee that my
delete
> is called on the IO thread, which is what I need.

Oh, whoops. I was thinking of DeleteTask from base/task.h (which DeleteSoon
uses).

BrowserThread::PostTask(BT::IO, FROM_HERE, new DeleteTask(socket_controller_)).

[Aaron Boodman, 2011-12-08 00:43:20]

lgtm

http://codereview.chromium.org/8857004/diff/6001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8857004/diff/6001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:478: // APIControllerController
(still working on that name).
FYI, see crbug.com/104095.

[asargent_no_longer_on_chrome, 2011-12-08 01:00:05]

lgtm

As noted in person, it's slightly unusual to construct SocketController on the
UI thread but destruct it on the IO thread, but implementing aa's suggested
ultimate fix of having the sockets' lifetime bound to the calling renderer's
should fix this.

Also, at that time you may or may not find it handy to make the socket objects
ref counted, and then you can use the BrowserThread::DeleteOnIOThread traits.
For usage, see the comment at:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/public/browse...

http://codereview.chromium.org/8857004/diff/6001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8857004/diff/6001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:478: // APIControllerController
(still working on that name).

Re:APIControllerController, you might be able to use

http://neugierig.org/software/chromium/class-name-generator.html

[miket_OOO, 2011-12-08 01:00:44]

> Oh, whoops. I was thinking of DeleteTask from base/task.h (which DeleteSoon
> uses).

Gotcha. Yes, works great. Smaller binary, good stuff.

[commit-bot: I haz the power, 2011-12-08 20:17:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/8857004/14001

[commit-bot: I haz the power, 2011-12-08 20:17:45]

Can't apply patch for file tools/heapcheck/suppressions.txt.
While running patch -p1 --forward --force;
patching file tools/heapcheck/suppressions.txt
Hunk #1 FAILED at 1785.
1 out of 1 hunk FAILED -- saving rejects to file
tools/heapcheck/suppressions.txt.rej

[commit-bot: I haz the power, 2011-12-08 20:40:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/8857004/10003

[commit-bot: I haz the power, 2011-12-08 22:02:15]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_clang, revision is 113651, job name
was 8857004-10003 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2011-12-08 22:04:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/8857004/10003

[commit-bot: I haz the power, 2011-12-09 00:40:22]

Change committed as 113710