Limit Chrome To Mobile cloud print access; refinements etc.

Limit Chrome To Mobile cloud print access; refinements etc.
Check the user's account info before contacting cloud print.
Only ping cloud print for users of that service.

Make ChromeToMobileService a RefcountedProfileKeyedService.
Add and check |temp_dir_valid_| on GenerateSnapshot.
Fix a JSON DictionaryValue leak from ParseResponseJSON use.
Make |mobiles_| a ScopedVector of DeepCopy DictionaryValues.

Call new ChromeToMobileService::HasDevices for encapsulation.
Call RequestMobileListUpdate from bubbles, not their callers.
Mock RequestMobileListUpdate so tests do not send requests.
Store the service, not the profile, as a bubble class member.
Skip IsSyncAccessible on updates (the first check suffices).

Auto-close on Mac via performSelector, not PostDelayedTask.
cancelPreviousPerformRequestsWithTarget on windowWillClose.
Call OnChanged after UpdateChromeToMobileEnabled for Mac.
(the icon wasn't showing up immediately on Release builds)

BUG=102709, 118691
TEST=Less cloud print traffic. No apparent behavior changes.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129779

[msw, 2012-03-28 02:32:17]

Please take a look; thanks!
Sky: All except chrome/browser/ui/cocoa (unless you care to).
Shess: chrome/browser/ui/cocoa

[nyquist, 2012-03-28 06:26:07]

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/chrom...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/chrom...
chrome/browser/chrome_to_mobile_service.cc:50: const int
kSearchRequestDelayHours = 24;
With this new limitation, I thought we could reduce this to almost nothing?
We should take into account misbehaving clients with spawn-new-windows-in-a-loop
kind of behavior, but other than that, I thought the only-search-on-new-window
and on-usage-of-popup was enough?

[sky, 2012-03-28 15:49:34]

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/chrom...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/chrom...
chrome/browser/chrome_to_mobile_service.cc:372: DictionaryValue* json_data =
NULL;
Do you need to delete json_data?

[Scott Hess - ex-Googler, 2012-03-28 15:58:57]

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/chrom...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/chrom...
chrome/browser/chrome_to_mobile_service.cc:190: base::Unretained(this)));
I find myself wondering how you know that |this| will still exist when
CreateUniqueTempDir() is called on it?  Likewise wondering how the temp_dir_
access in GenerateSnapshot() isn't racy?

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/ui/co...
File chrome/browser/ui/cocoa/chrome_to_mobile_bubble_controller.mm (right):

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/ui/co...
chrome/browser/ui/cocoa/chrome_to_mobile_bubble_controller.mm:200: [self
performSelector:@selector(cancel:) withObject:nil afterDelay:kCloseS];
Make sure that something like:
  [NSObject cancelPreviousPerformRequestsWithTarget:self];
is being called in a sensible place, otherwise you'll see zombie-message crashes
when people manually close before the auto-close.  Commonly it would either be
in -dealloc or the close path, -windowWillClose: probably makes the most sense
in this case.

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/ui/co...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/ui/co...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:707:
ChromeToMobileServiceFactory::GetForProfile(profile_)->HasDevices();
+1!

[msw, 2012-03-29 01:18:15]

Addressed comments, PTAL; thanks!

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/chrom...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/chrom...
chrome/browser/chrome_to_mobile_service.cc:50: const int
kSearchRequestDelayHours = 24;
On 2012/03/28 06:26:07, nyquist wrote:
> With this new limitation, I thought we could reduce this to almost nothing?
> We should take into account misbehaving clients with
spawn-new-windows-in-a-loop
> kind of behavior, but other than that, I thought the only-search-on-new-window
> and on-usage-of-popup was enough?

Let's consider reducing this delay in a follow-up. Right now I'd rather strictly
decrease the QPS, so there's less risk of impacting cloud print (I'm planning to
merge this to M19).

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/chrom...
chrome/browser/chrome_to_mobile_service.cc:190: base::Unretained(this)));
On 2012/03/28 15:58:57, shess wrote:
> I find myself wondering how you know that |this| will still exist when
> CreateUniqueTempDir() is called on it?  Likewise wondering how the temp_dir_
> access in GenerateSnapshot() isn't racy?

This class is now RefCountedThreadSafe with a |temp_dir_valid_| flag.

Also, signal that snapshot generation failed if the temp dir is invalid in
GenerateSnapshot. In that case, bubbles will show error text alongside the
checkbox and leave them disabled and unchecked. Users can still send the URL
(without a snapshot), and the bubble still otherwise functions normally.

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/chrom...
chrome/browser/chrome_to_mobile_service.cc:372: DictionaryValue* json_data =
NULL;
On 2012/03/28 15:49:34, sky wrote:
> Do you need to delete json_data?

Fixed this and Handle[Search|Submit]Response.

It now uses JSONReader directly for better memory handling.
|mobiles_| is now a ScopedVector of DeepCopy'ed DictionaryValues.

Thanks for catching that! I was following the pattern of
CloudPrintURLFetcher::OnURLFetchComplete, I'll ping the cloud print guys about
their apparent leak, and I'll probably move ParseResponseJSON back to its
original location in a followup.

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/ui/co...
File chrome/browser/ui/cocoa/chrome_to_mobile_bubble_controller.mm (right):

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/ui/co...
chrome/browser/ui/cocoa/chrome_to_mobile_bubble_controller.mm:200: [self
performSelector:@selector(cancel:) withObject:nil afterDelay:kCloseS];
On 2012/03/28 15:58:57, shess wrote:
> Make sure that something like:
>   [NSObject cancelPreviousPerformRequestsWithTarget:self];
> is being called in a sensible place, otherwise you'll see zombie-message
crashes
> when people manually close before the auto-close.  Commonly it would either be
> in -dealloc or the close path, -windowWillClose: probably makes the most sense
> in this case.

Done; this worked in my previous manual testing, but explicit cancellation is
even better; thanks for the tip!

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/ui/co...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://chromiumcodereview.appspot.com/9863054/diff/7005/chrome/browser/ui/co...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:707:
ChromeToMobileServiceFactory::GetForProfile(profile_)->HasDevices();
On 2012/03/28 15:58:57, shess wrote:
> +1!

:)

[sky, 2012-03-29 15:57:07]

LGTM

[Scott Hess - ex-Googler, 2012-03-29 22:26:17]

LGTM, except I'm still not enthused about accessing the data from two threads. 
But since that was a pre-existing condition, I think that could be solved in a
follow-up.

https://chromiumcodereview.appspot.com/9863054/diff/17001/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/9863054/diff/17001/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:309: }
I'm not sure we're on the same page, here.

One problem is whether |this| can go away while the task is in-flight. 
Ref-counted handles that problem.

The other problem is that the two threads can see the bits of temp_dir_ in
inconsistent orders, and temp_dir_valid_ doesn't address that (because the
threads can see bits of temp_dir_valid_ in inconsistent order WRT temp_dir_).

One option would be PostTaskAndReply(), with the task on the other thread handle
temp_dir_, and the reply task on the local thread setting temp_dir_valid_.  The
posting of the reply thread would provide the barrier that make temp_dir_ valid
on both threads.

Does that make sense?  You could probably also do it by passing around an
Owned() pointer to a ScopedTempDir (static function in the background,
foreground task steals created item, no need for the ref-counted stuff).  You
could do it with a lock.

Too bad FileUtilProxy can't be used.  There is CreateTemporary() to create
temporary files.

[It may be reasonable to bust this problem out of this CL so it doesn't block. 
The race condition is narrow, IMHO.]

[msw, 2012-03-29 23:21:48]

Filed crbug.com/121003; will land soon and may request merge.

https://chromiumcodereview.appspot.com/9863054/diff/17001/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/9863054/diff/17001/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:309: }
> threads can see bits of temp_dir_valid_ in inconsistent order 
Yes, but I erred on the side of caution: |temp_dir_valid_| should never be true
before |temp_dir_| is valid; at worst we'll skip snapshot generation when it
could have been possible.

> One option would be PostTaskAndReply()
This was my first instinct, but that doesn't support the blocking pool yet
(afaik), so I'd have to use the (bad) FILE thread. Maybe that's okay and I
should go this route in a followup?

> Too bad FileUtilProxy can't be used.  There is CreateTemporary()
Ooh! I wasn't aware of this, I'd like to redesign the snapshot writing and
destruction with this helper (if the feature survives).

> bust this problem out of this CL so it doesn't block.
SGTM, I've filed crbug.com/121003 to follow up (in M20, cool?).

[commit-bot: I haz the power, 2012-03-29 23:22:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/9863054/17001

[Scott Hess - ex-Googler, 2012-03-29 23:51:08]

https://chromiumcodereview.appspot.com/9863054/diff/17001/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/9863054/diff/17001/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:309: }
On 2012/03/29 23:21:48, msw wrote:
> > threads can see bits of temp_dir_valid_ in inconsistent order 
> Yes, but I erred on the side of caution: |temp_dir_valid_| should never be
true
> before |temp_dir_| is valid; at worst we'll skip snapshot generation when it
> could have been possible.

The issue is that you are setting the values in a different thread from where
you are reading them, with no explicit memory barrier.  Thus you don't guarantee
that the UI thread sees changes in the right order.  It is possible (*) that the
UI thread could see temp_dir_valid_ as true before it sees the temp_dir_
changes.

(*) On x86, the stores will be observed in the order made.  On ARM, they can be
observed in any order.

> > One option would be PostTaskAndReply()
> This was my first instinct, but that doesn't support the blocking pool yet
> (afaik), so I'd have to use the (bad) FILE thread. Maybe that's okay and I
> should go this route in a followup?

If the blocking pool can't be bothered to support this really useful function,
I'd just use the FILE thread, bad or otherwise :-).  Or add an AndReply()
variant to the blocking pool.  Either makes good sense to me.

[commit-bot: I haz the power, 2012-03-30 02:07:47]

Try job failure for 9863054-17001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[msw, 2012-03-30 03:51:40]

It seems that most/all win_rel trybot browser_tests are failing this same way
:-/
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/15967
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/15964
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/15961
I'm going to land manually.