abort SendFinancialPing if process is shutting down

abort SendFinancialPing if process is shutting down


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

[bcwhite, 2012-11-23 16:46:40]

Glenn, you were a suggested OWNER for this; what do you think?

It's a hack, but it's an _existing_ hack.  There are some plans to re-work RLZ
to run on a different thread that would then be immune to this shut-down race
condition.  Is this an acceptable fix util then?

-- Brian

[Glenn Wilson, 2012-11-27 01:17:49]

lgtm

It's been a long time since I've committed code to this directory -- Roger &
Nico are far more familiar with this code, so I'd want one of their approvals in
lieu of mine.

Otherwise, FWIW, LGTM.

https://codereview.chromium.org/11419137/diff/1/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

https://codereview.chromium.org/11419137/diff/1/chrome/browser/rlz/rlz.cc#new...
chrome/browser/rlz/rlz.cc:140: return false;
is SendFinancialPing the right depth for this?  Should it be higher up the chain
in RLZTracker::PingNowImpl (or higher) instead?

[Roger Tawa OOO till Jul 10th, 2012-11-27 21:26:04]

lgtm

Let commit this for now to close the crash window.

[commit-bot: I haz the power, 2012-11-27 21:31:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bcwhite@chromium.org/11419137/1

[commit-bot: I haz the power, 2012-11-27 22:11:37]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[bcwhite, 2012-11-27 22:14:23]

Looks like a broken build:
e:\b\build\slave\win\build\src\net\tools\dump_cache\upgrade_win.h(8) : error
C2146: syntax error : missing ';' before identifier 'CreateServer'

That's not anywhere near my patch.  Will try again later...

[commit-bot: I haz the power, 2012-11-29 15:39:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bcwhite@chromium.org/11419137/1

[commit-bot: I haz the power, 2012-11-29 22:00:12]

Change committed as 170247

[jam, 2012-12-03 15:46:05]

https://chromiumcodereview.appspot.com/11419137/diff/1/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

https://chromiumcodereview.appspot.com/11419137/diff/1/chrome/browser/rlz/rlz...
chrome/browser/rlz/rlz.cc:57: extern bool g_exited_main_message_loop;
this is wrong. internal variables of content shouldn't be accessed from chrome.

if you follow where this is set in content, you can see callbacks to the
embedder that chrome can use to keep track of this. i.e. right below the method
calls BrowserMainLoop::ShutdownThreadsAndCleanUp which calls the embedder's
PostMainMessageLoopRun() (implemented in
ChromeBrowserMainParts::PostMainMessageLoopRun).

[bcwhite, 2012-12-03 15:54:54]

I'll roll it back.

[Ivan Korotkov, 2012-12-03 19:26:07]

Cool, thanks

[jam, 2012-12-03 21:32:02]

On 2012/12/03 19:26:07, Ivan Korotkov wrote:
> Cool, thanks

thanks