Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2118473002:
😴 Skip some KeepAlive registrations on shutdown. (Closed)
Description[bgmode] Skip some KeepAlive registrations on shutdown.
Registering a KeepAlive during shtudown is forbidden and hits
a CHECK, so we should avoid starting some operations that do
it.
This CL adds checks to avoid such situations:
- push notifications received from GCM while shutting down
will be dropped early instead of causing a crash and being
dropped anyway.
- Extension functions using chrome_details validate before
running that the browser process is not shutting down.
BUG=625646
Committed: https://crrev.com/43f25e4fc136ddba3b516c329ac375ce9185b6b3
Cr-Commit-Position: refs/heads/master@{#406001}
Patch Set 1 #Patch Set 2 : [bgmode] Make KeepAlive registration less strict. #Patch Set 3 : [bgmode] Make KeepAlive registration less strict. #
Total comments: 1
Patch Set 4 : Check that we are not shutting down in sensitive places instead #Patch Set 5 : Check that we are not shutting down in sensitive places instead #Patch Set 6 : #Patch Set 7 : [more generic anti extension crash check #
Total comments: 7
Patch Set 8 : address comments #Patch Set 9 : Support Extension Functions though ChromeDetails, add tests #Patch Set 10 : rebase #Patch Set 11 : use ExtensionsBrowserClient #
Total comments: 8
Patch Set 12 : address comments #
Total comments: 3
Patch Set 13 : address comments #Messages
Total messages: 42 (16 generated)
dgn@chromium.org changed reviewers: + sky@chromium.org
We received crash reports related to posted tasks attempting to register KeepAlives while the browser is shutting down. It is not always clear where they are initiated from, and sometimes that comes from 3p code (extensions for example). Looking back at the old code, I realize that there is a bit I didn't include: The WillKeepAlive check on new registrations: https://codereview.chromium.org/1778873002/diff/140001/chrome/browser/lifetim... This CL aims to restore a similar behaviour. - Classes that used to depend on AddRefModule() still register Strict KeepAlives, the other ones don't. - Test-specific KeepAlives are strict too.
https://codereview.chromium.org/2118473002/diff/40001/chrome/browser/lifetime... File chrome/browser/lifetime/keep_alive_types.h (right): https://codereview.chromium.org/2118473002/diff/40001/chrome/browser/lifetime... chrome/browser/lifetime/keep_alive_types.h:54: enum class KeepAliveStrictOption { DISABLED, ENABLED }; This name and comment are very confusing, it doesn't at all convey when you would use one vs the other. Seems like what you are trying to convey is what should happen if called during shutdown. Perhaps something like: IGNORE_IF_IN_SHUTDOWN, PROCESS_DURING_SHUTDOWN. Although ideally we wouldn't have this. Don't the crash reports show a stack that could isolate where the request is coming from?
On 2016/06/30 17:57:40, sky wrote: > https://codereview.chromium.org/2118473002/diff/40001/chrome/browser/lifetime... > File chrome/browser/lifetime/keep_alive_types.h (right): > > https://codereview.chromium.org/2118473002/diff/40001/chrome/browser/lifetime... > chrome/browser/lifetime/keep_alive_types.h:54: enum class KeepAliveStrictOption > { DISABLED, ENABLED }; > This name and comment are very confusing, it doesn't at all convey when you > would use one vs the other. Seems like what you are trying to convey is what > should happen if called during shutdown. Perhaps something like: > IGNORE_IF_IN_SHUTDOWN, PROCESS_DURING_SHUTDOWN. > > Although ideally we wouldn't have this. Don't the crash reports show a stack > that could isolate where the request is coming from? The ones I saw are messages received from extensions' renderer thread, calling Chrome code to show notifications or similar things, and we crash because we are shutting down. I don't see a good way to register keepalives across processes. That's why I looked into letting those be ignored as it was done before.
On 2016/06/30 21:00:39, dgn wrote: > On 2016/06/30 17:57:40, sky wrote: > > > https://codereview.chromium.org/2118473002/diff/40001/chrome/browser/lifetime... > > File chrome/browser/lifetime/keep_alive_types.h (right): > > > > > https://codereview.chromium.org/2118473002/diff/40001/chrome/browser/lifetime... > > chrome/browser/lifetime/keep_alive_types.h:54: enum class > KeepAliveStrictOption > > { DISABLED, ENABLED }; > > This name and comment are very confusing, it doesn't at all convey when you > > would use one vs the other. Seems like what you are trying to convey is what > > should happen if called during shutdown. Perhaps something like: > > IGNORE_IF_IN_SHUTDOWN, PROCESS_DURING_SHUTDOWN. > > > > Although ideally we wouldn't have this. Don't the crash reports show a stack > > that could isolate where the request is coming from? > > The ones I saw are messages received from extensions' renderer thread, calling > Chrome code to show notifications or similar things, and we crash because we are > shutting down. I don't see a good way to register keepalives across processes. > That's why I looked into letting those be ignored as it was done before. Specifically, they were tasks posted to the UI thread message loop that would run after shutdown has started, because we call QuitWhenIdle() on the message loop. Seeing as there was no guarantee when posting the task that it would run at all, I figured that ignoring the KeepAlive would not be worse.
On Thu, Jun 30, 2016 at 2:00 PM, <dgn@chromium.org> wrote: > On 2016/06/30 17:57:40, sky wrote: >> > https://codereview.chromium.org/2118473002/diff/40001/chrome/browser/lifetime... >> File chrome/browser/lifetime/keep_alive_types.h (right): >> >> > https://codereview.chromium.org/2118473002/diff/40001/chrome/browser/lifetime... >> chrome/browser/lifetime/keep_alive_types.h:54: enum class > KeepAliveStrictOption >> { DISABLED, ENABLED }; >> This name and comment are very confusing, it doesn't at all convey when >> you >> would use one vs the other. Seems like what you are trying to convey is >> what >> should happen if called during shutdown. Perhaps something like: >> IGNORE_IF_IN_SHUTDOWN, PROCESS_DURING_SHUTDOWN. >> >> Although ideally we wouldn't have this. Don't the crash reports show a >> stack >> that could isolate where the request is coming from? > > The ones I saw are messages received from extensions' renderer thread, > calling > Chrome code to show notifications or similar things, and we crash because we > are > shutting down. I don't see a good way to register keepalives across > processes. > That's why I looked into letting those be ignored as it was done before. Is there a single code location for that? By which I mean is there a central place the IPC is received that you could early out if shutting down? Assuming not, I'm ok with the change as long as you come up with something that conveys what it does better. -Scott > > https://codereview.chromium.org/2118473002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> >> Although ideally we wouldn't have this. Don't the crash reports show a > >> stack > >> that could isolate where the request is coming from? > > > > The ones I saw are messages received from extensions' renderer thread, > > calling > > Chrome code to show notifications or similar things, and we crash because we > > are > > shutting down. I don't see a good way to register keepalives across > > processes. > > That's why I looked into letting those be ignored as it was done before. > > Is there a single code location for that? By which I mean is there a > central place the IPC is received that you could early out if shutting > down? Assuming not, I'm ok with the change as long as you come up with > something that conveys what it does better. There are 2 places I know crashes can come from. For other KeepAlives, they can be created only when another one exists for some reason, for example because they happen from actions performed in the browser so, the Browser's KA will be up. I rewrote the patch to isolate those. PTAL.
sky@chromium.org changed reviewers: + johnme@chromium.org
LGTM - but I'm adding johnme@ as owner for the push_messaging change as I'm not sure if there are pitfalls in the early out in PushMessagingServiceImpl::OnMessage.
dgn@chromium.org changed reviewers: + peter@chromium.org, rdevlin.cronin@chromium.org - johnme@chromium.org
rdevlin.cronin@: PTAL at the extension changes. Also adding peter@ for push_messaging review, since we discussed it offline last week.
push and notifications lgtm, thanks! https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_function.cc (right): https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function.cc:106: return RespondNow(Error(error_)); Is this potentially developer-visible at this point in the process' lifetime? If so, it would be good to be more descriptive about *what* is shutting down— my gut feel upon receiving this message would be the extension/app. What about "The browser is shutting down."?
johnme@chromium.org changed reviewers: + johnme@chromium.org
push_messaging lgtm
https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_function.cc (right): https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function.cc:106: return RespondNow(Error(error_)); On 2016/07/05 12:42:10, Peter Beverloo wrote: > Is this potentially developer-visible at this point in the process' lifetime? If > so, it would be good to be more descriptive about *what* is shutting down— my > gut feel upon receiving this message would be the extension/app. > > What about "The browser is shutting down."? ChromeAsyncExtensionFunction is just one of many ExtensionFunction variants. If your intention is to do this for all extension functions, this should be done somewhere in ExtensionFunction, rather than ChromeAsyncExtensionFunction. If you only need this for a specific extension function, then it show go there (though I'd also be curious why only one needs this treatment). Additionally, ExtensionFunctions can run very asynchronously. So let's say Run() is called and we're not shutting down, but then we post a task for something else to happen, and then we start shutting down. The task will be called, but I don't think we really check IsShuttingDown() in any of them. The right-est fix for this would probably be to unrefcount ExtensionFunction and have it use weak ptrs for its tasks, and then have it owned by something reasonable that will be destroyed naturally in the course of shut down. But that will be a massive refactoring.
Description was changed from ========== [bgmode] Ignore KeepAlive registration when shutting down. BUG=623727 ========== to ========== [bgmode] Ignore KeepAlive registration when shutting down. BUG=625646 ==========
https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_function.cc (right): https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function.cc:106: return RespondNow(Error(error_)); On 2016/07/06 16:54:23, Devlin wrote: > On 2016/07/05 12:42:10, Peter Beverloo wrote: > > Is this potentially developer-visible at this point in the process' lifetime? > If > > so, it would be good to be more descriptive about *what* is shutting down— my > > gut feel upon receiving this message would be the extension/app. > > > > What about "The browser is shutting down."? > > ChromeAsyncExtensionFunction is just one of many ExtensionFunction variants. If > your intention is to do this for all extension functions, this should be done > somewhere in ExtensionFunction, rather than ChromeAsyncExtensionFunction. If > you only need this for a specific extension function, then it show go there > (though I'd also be curious why only one needs this treatment). > > Additionally, ExtensionFunctions can run very asynchronously. So let's say > Run() is called and we're not shutting down, but then we post a task for > something else to happen, and then we start shutting down. The task will be > called, but I don't think we really check IsShuttingDown() in any of them. > > The right-est fix for this would probably be to unrefcount ExtensionFunction and > have it use weak ptrs for its tasks, and then have it owned by something > reasonable that will be destroyed naturally in the course of shut down. But > that will be a massive refactoring. Thanks for the explanation. I'm not too keen on starting a massive refactoring now. The functions I noticed to be a potential issue are WindowsCreateFunction, TabsCreateFunction, BrowserOpenTabFunction (creates new browser, other functions seem to either not create any or properly check for a given window id), NotificationsCreateFunction (creates new notifications = keepAlive). The related code is synchronous until the point where we care, so checking that we are shutting down in ExtensionFunction would fix the problem. But if that is inherently going to be unreliable for other cases, checking for these functions specifically (through extension_tab_util.cc for example) could be fine too.
https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_function.cc (right): https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function.cc:106: return RespondNow(Error(error_)); On 2016/07/07 15:20:17, dgn wrote: > On 2016/07/06 16:54:23, Devlin wrote: > > On 2016/07/05 12:42:10, Peter Beverloo wrote: > > > Is this potentially developer-visible at this point in the process' > lifetime? > > If > > > so, it would be good to be more descriptive about *what* is shutting down— > my > > > gut feel upon receiving this message would be the extension/app. > > > > > > What about "The browser is shutting down."? > > > > ChromeAsyncExtensionFunction is just one of many ExtensionFunction variants. > If > > your intention is to do this for all extension functions, this should be done > > somewhere in ExtensionFunction, rather than ChromeAsyncExtensionFunction. If > > you only need this for a specific extension function, then it show go there > > (though I'd also be curious why only one needs this treatment). > > > > Additionally, ExtensionFunctions can run very asynchronously. So let's say > > Run() is called and we're not shutting down, but then we post a task for > > something else to happen, and then we start shutting down. The task will be > > called, but I don't think we really check IsShuttingDown() in any of them. > > > > The right-est fix for this would probably be to unrefcount ExtensionFunction > and > > have it use weak ptrs for its tasks, and then have it owned by something > > reasonable that will be destroyed naturally in the course of shut down. But > > that will be a massive refactoring. > > Thanks for the explanation. I'm not too keen on starting a massive refactoring > now. > > The functions I noticed to be a potential issue are WindowsCreateFunction, > TabsCreateFunction, BrowserOpenTabFunction (creates new browser, other functions > seem to either not create any or properly check for a given window id), > NotificationsCreateFunction (creates new notifications = keepAlive). The related > code is synchronous until the point where we care, so checking that we are > shutting down in ExtensionFunction would fix the problem. > > But if that is inherently going to be unreliable for other cases, checking for > these functions specifically (through extension_tab_util.cc for example) could > be fine too. If this solves the immediate problem, we can avoid the longer term solution for now (since this is strictly better than the current situation). However, I'd still like to put this a little higher up (e.g. in ExtensionFunction somewhere rather than ChromeAsyncExtensionFunction) so that it gets better exposure. Additionally, a TODO that summarizes the important pieces here (mostly that any posted tasks won't be checked and the suggestion about weak ptrs that can be cleared on shutdown) would be good.
PTAL https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_function.cc (right): https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function.cc:106: return RespondNow(Error(error_)); On 2016/07/08 01:01:01, Devlin wrote: > On 2016/07/07 15:20:17, dgn wrote: > > On 2016/07/06 16:54:23, Devlin wrote: > > > On 2016/07/05 12:42:10, Peter Beverloo wrote: > > > > Is this potentially developer-visible at this point in the process' > > lifetime? > > > If > > > > so, it would be good to be more descriptive about *what* is shutting down— > > my > > > > gut feel upon receiving this message would be the extension/app. > > > > > > > > What about "The browser is shutting down."? > > > > > > ChromeAsyncExtensionFunction is just one of many ExtensionFunction variants. > > > If > > > your intention is to do this for all extension functions, this should be > done > > > somewhere in ExtensionFunction, rather than ChromeAsyncExtensionFunction. > If > > > you only need this for a specific extension function, then it show go there > > > (though I'd also be curious why only one needs this treatment). > > > > > > Additionally, ExtensionFunctions can run very asynchronously. So let's say > > > Run() is called and we're not shutting down, but then we post a task for > > > something else to happen, and then we start shutting down. The task will be > > > called, but I don't think we really check IsShuttingDown() in any of them. > > > > > > The right-est fix for this would probably be to unrefcount ExtensionFunction > > and > > > have it use weak ptrs for its tasks, and then have it owned by something > > > reasonable that will be destroyed naturally in the course of shut down. But > > > that will be a massive refactoring. > > > > Thanks for the explanation. I'm not too keen on starting a massive refactoring > > now. > > > > The functions I noticed to be a potential issue are WindowsCreateFunction, > > TabsCreateFunction, BrowserOpenTabFunction (creates new browser, other > functions > > seem to either not create any or properly check for a given window id), > > NotificationsCreateFunction (creates new notifications = keepAlive). The > related > > code is synchronous until the point where we care, so checking that we are > > shutting down in ExtensionFunction would fix the problem. > > > > But if that is inherently going to be unreliable for other cases, checking for > > these functions specifically (through extension_tab_util.cc for example) could > > be fine too. > > If this solves the immediate problem, we can avoid the longer term solution for > now (since this is strictly better than the current situation). However, I'd > still like to put this a little higher up (e.g. in ExtensionFunction somewhere > rather than ChromeAsyncExtensionFunction) so that it gets better exposure. > Additionally, a TODO that summarizes the important pieces here (mostly that any > posted tasks won't be checked and the suggestion about weak ptrs that can be > cleared on shutdown) would be good. Done. I could not make the check in ExtensionFunction because it's not in //chrome. I used ChromeEFDetails instead.
https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_function.cc (right): https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function.cc:106: return RespondNow(Error(error_)); On 2016/07/12 15:58:19, dgn wrote: > On 2016/07/08 01:01:01, Devlin wrote: > > On 2016/07/07 15:20:17, dgn wrote: > > > On 2016/07/06 16:54:23, Devlin wrote: > > > > On 2016/07/05 12:42:10, Peter Beverloo wrote: > > > > > Is this potentially developer-visible at this point in the process' > > > lifetime? > > > > If > > > > > so, it would be good to be more descriptive about *what* is shutting > down— > > > my > > > > > gut feel upon receiving this message would be the extension/app. > > > > > > > > > > What about "The browser is shutting down."? > > > > > > > > ChromeAsyncExtensionFunction is just one of many ExtensionFunction > variants. > > > > > If > > > > your intention is to do this for all extension functions, this should be > > done > > > > somewhere in ExtensionFunction, rather than ChromeAsyncExtensionFunction. > > If > > > > you only need this for a specific extension function, then it show go > there > > > > (though I'd also be curious why only one needs this treatment). > > > > > > > > Additionally, ExtensionFunctions can run very asynchronously. So let's > say > > > > Run() is called and we're not shutting down, but then we post a task for > > > > something else to happen, and then we start shutting down. The task will > be > > > > called, but I don't think we really check IsShuttingDown() in any of them. > > > > > > > > The right-est fix for this would probably be to unrefcount > ExtensionFunction > > > and > > > > have it use weak ptrs for its tasks, and then have it owned by something > > > > reasonable that will be destroyed naturally in the course of shut down. > But > > > > that will be a massive refactoring. > > > > > > Thanks for the explanation. I'm not too keen on starting a massive > refactoring > > > now. > > > > > > The functions I noticed to be a potential issue are WindowsCreateFunction, > > > TabsCreateFunction, BrowserOpenTabFunction (creates new browser, other > > functions > > > seem to either not create any or properly check for a given window id), > > > NotificationsCreateFunction (creates new notifications = keepAlive). The > > related > > > code is synchronous until the point where we care, so checking that we are > > > shutting down in ExtensionFunction would fix the problem. > > > > > > But if that is inherently going to be unreliable for other cases, checking > for > > > these functions specifically (through extension_tab_util.cc for example) > could > > > be fine too. > > > > If this solves the immediate problem, we can avoid the longer term solution > for > > now (since this is strictly better than the current situation). However, I'd > > still like to put this a little higher up (e.g. in ExtensionFunction somewhere > > rather than ChromeAsyncExtensionFunction) so that it gets better exposure. > > Additionally, a TODO that summarizes the important pieces here (mostly that > any > > posted tasks won't be checked and the suggestion about weak ptrs that can be > > cleared on shutdown) would be good. > > Done. I could not make the check in ExtensionFunction because it's not in > //chrome. I used ChromeEFDetails instead. Instead you can use ExtensionsBrowserClient. We do this here, too: https://cs.chromium.org/chromium/src/extensions/browser/extension_function.cc... Then you don't have to architect quite as much here. Also note that you can only put this in a UIThreadExtensionFunction as it can only be accessed on that thread, whereas this would crash for IOThreadExtension functions.
PTAL. I left the test in //chrome as I would prefer using testing_browser_process and a normally initialized ExtensionsBrowserClient rather than mocking that too. https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_function.cc (right): https://codereview.chromium.org/2118473002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function.cc:106: return RespondNow(Error(error_)); On 2016/07/12 16:19:38, Devlin wrote: > On 2016/07/12 15:58:19, dgn wrote: > > On 2016/07/08 01:01:01, Devlin wrote: > > > On 2016/07/07 15:20:17, dgn wrote: > > > > On 2016/07/06 16:54:23, Devlin wrote: > > > > > On 2016/07/05 12:42:10, Peter Beverloo wrote: > > > > > > Is this potentially developer-visible at this point in the process' > > > > lifetime? > > > > > If > > > > > > so, it would be good to be more descriptive about *what* is shutting > > down— > > > > my > > > > > > gut feel upon receiving this message would be the extension/app. > > > > > > > > > > > > What about "The browser is shutting down."? > > > > > > > > > > ChromeAsyncExtensionFunction is just one of many ExtensionFunction > > variants. > > > > > > > If > > > > > your intention is to do this for all extension functions, this should be > > > done > > > > > somewhere in ExtensionFunction, rather than > ChromeAsyncExtensionFunction. > > > If > > > > > you only need this for a specific extension function, then it show go > > there > > > > > (though I'd also be curious why only one needs this treatment). > > > > > > > > > > Additionally, ExtensionFunctions can run very asynchronously. So let's > > say > > > > > Run() is called and we're not shutting down, but then we post a task for > > > > > something else to happen, and then we start shutting down. The task > will > > be > > > > > called, but I don't think we really check IsShuttingDown() in any of > them. > > > > > > > > > > The right-est fix for this would probably be to unrefcount > > ExtensionFunction > > > > and > > > > > have it use weak ptrs for its tasks, and then have it owned by something > > > > > reasonable that will be destroyed naturally in the course of shut down. > > But > > > > > that will be a massive refactoring. > > > > > > > > Thanks for the explanation. I'm not too keen on starting a massive > > refactoring > > > > now. > > > > > > > > The functions I noticed to be a potential issue are WindowsCreateFunction, > > > > TabsCreateFunction, BrowserOpenTabFunction (creates new browser, other > > > functions > > > > seem to either not create any or properly check for a given window id), > > > > NotificationsCreateFunction (creates new notifications = keepAlive). The > > > related > > > > code is synchronous until the point where we care, so checking that we are > > > > shutting down in ExtensionFunction would fix the problem. > > > > > > > > But if that is inherently going to be unreliable for other cases, checking > > for > > > > these functions specifically (through extension_tab_util.cc for example) > > could > > > > be fine too. > > > > > > If this solves the immediate problem, we can avoid the longer term solution > > for > > > now (since this is strictly better than the current situation). However, > I'd > > > still like to put this a little higher up (e.g. in ExtensionFunction > somewhere > > > rather than ChromeAsyncExtensionFunction) so that it gets better exposure. > > > Additionally, a TODO that summarizes the important pieces here (mostly that > > any > > > posted tasks won't be checked and the suggestion about weak ptrs that can be > > > cleared on shutdown) would be good. > > > > Done. I could not make the check in ExtensionFunction because it's not in > > //chrome. I used ChromeEFDetails instead. > > Instead you can use ExtensionsBrowserClient. We do this here, too: > https://cs.chromium.org/chromium/src/extensions/browser/extension_function.cc... > > Then you don't have to architect quite as much here. Also note that you can > only put this in a UIThreadExtensionFunction as it can only be accessed on that > thread, whereas this would crash for IOThreadExtension functions. Oh wow it makes it much simpler indeed. Thanks! I'm embarrassed that I didn't see it before. Anyway, done.
overall, looks pretty good! https://codereview.chromium.org/2118473002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_function_unittest.cc (right): https://codereview.chromium.org/2118473002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function_unittest.cc:14: void SuccessCallback(AsyncExtensionFunction::ResponseType type, nit: put these in an anonymous namespace. https://codereview.chromium.org/2118473002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function_unittest.cc:14: void SuccessCallback(AsyncExtensionFunction::ResponseType type, prefer ExtensionFunction::ResponseType https://codereview.chromium.org/2118473002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function_unittest.cc:18: EXPECT_EQ(ExtensionFunction::ResponseType::SUCCEEDED, type); It would be good to assert that these functions did, in fact, run. If, for instance, we ever have an asynchronous step between RespondNow() and the response actually being called, these tests would succeed without ever testing anything. https://codereview.chromium.org/2118473002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function_unittest.cc:28: class ChromeExtensionFunctionUnitTest : public ExtensionServiceTestBase {}; nit: Prefer using ChromeExtensionFunctionUnitTest = ExtensionServiceTestBase;
Thanks for the quick review! PTAL https://codereview.chromium.org/2118473002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_function_unittest.cc (right): https://codereview.chromium.org/2118473002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function_unittest.cc:14: void SuccessCallback(AsyncExtensionFunction::ResponseType type, On 2016/07/13 20:23:16, Devlin wrote: > prefer ExtensionFunction::ResponseType Done. https://codereview.chromium.org/2118473002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function_unittest.cc:14: void SuccessCallback(AsyncExtensionFunction::ResponseType type, On 2016/07/13 20:23:16, Devlin wrote: > prefer ExtensionFunction::ResponseType Done. https://codereview.chromium.org/2118473002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function_unittest.cc:18: EXPECT_EQ(ExtensionFunction::ResponseType::SUCCEEDED, type); On 2016/07/13 20:23:16, Devlin wrote: > It would be good to assert that these functions did, in fact, run. If, for > instance, we ever have an asynchronous step between RespondNow() and the > response actually being called, these tests would succeed without ever testing > anything. Done. https://codereview.chromium.org/2118473002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function_unittest.cc:28: class ChromeExtensionFunctionUnitTest : public ExtensionServiceTestBase {}; On 2016/07/13 20:23:16, Devlin wrote: > nit: Prefer > using ChromeExtensionFunctionUnitTest = ExtensionServiceTestBase; Added TearDown to the class.
https://codereview.chromium.org/2118473002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_function_unittest.cc (right): https://codereview.chromium.org/2118473002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function_unittest.cc:16: int g_callback_runs = 0; global variables are pretty fragile in unittests that can run in parallel. I think this would be simpler by doing something like: void SuccessCallback(bool* did_respond, ...) { EXPECT_EQ(...); *did_respond = true; } and then use set_response_callback(base::Bind((should_succeed ? &SuccessCallback : &FailureCallback), &did_respond_)); where did_respond_ can be a bool in ValidationFunction and then you can EXPECT_TRUE(function->did_respond()) in the test body. That way, we keep all the state in objects that we know will be properly initialized/cleaned up. https://codereview.chromium.org/2118473002/diff/220001/chrome/browser/notific... File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/2118473002/diff/220001/chrome/browser/notific... chrome/browser/notifications/message_center_notification_manager.cc:151: // out of KeepAlives drive-by: end comment in a '.'
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [bgmode] Ignore KeepAlive registration when shutting down. BUG=625646 ========== to ========== [bgmode] Skip some KeepAlive registrations on shutdown. Registering a KeepAlive during shtudown is forbidden and hits a CHECK, so we should avoid starting some operations that do it. This CL now checks if we are shutting down when receiving a push notification from GCM and running synchronous chrome extension functions BUG=625646 ==========
Description was changed from ========== [bgmode] Skip some KeepAlive registrations on shutdown. Registering a KeepAlive during shtudown is forbidden and hits a CHECK, so we should avoid starting some operations that do it. This CL now checks if we are shutting down when receiving a push notification from GCM and running synchronous chrome extension functions BUG=625646 ========== to ========== [bgmode] Skip some KeepAlive registrations on shutdown. Registering a KeepAlive during shtudown is forbidden and hits a CHECK, so we should avoid starting some operations that do it. This CL now checks if we are shutting down when receiving a push notification from GCM and running synchronous chrome extension functions BUG=625646 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2118473002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_function_unittest.cc (right): https://codereview.chromium.org/2118473002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function_unittest.cc:16: int g_callback_runs = 0; On 2016/07/15 17:59:39, Devlin wrote: > global variables are pretty fragile in unittests that can run in parallel. I > think this would be simpler by doing something like: > > void SuccessCallback(bool* did_respond, > ...) { > EXPECT_EQ(...); > *did_respond = true; > } > > and then use > set_response_callback(base::Bind((should_succeed ? &SuccessCallback : > &FailureCallback), &did_respond_)); > > where did_respond_ can be a bool in ValidationFunction > > and then you can EXPECT_TRUE(function->did_respond()) in the test body. > > That way, we keep all the state in objects that we know will be properly > initialized/cleaned up. Thanks! I didn't think about using Bind like that. Really nice.
Awesome, lgtm! It looks like your cl description is a little bit out of date, but otherwise this is good.
Description was changed from ========== [bgmode] Skip some KeepAlive registrations on shutdown. Registering a KeepAlive during shtudown is forbidden and hits a CHECK, so we should avoid starting some operations that do it. This CL now checks if we are shutting down when receiving a push notification from GCM and running synchronous chrome extension functions BUG=625646 ========== to ========== [bgmode] Skip some KeepAlive registrations on shutdown. Registering a KeepAlive during shtudown is forbidden and hits a CHECK, so we should avoid starting some operations that do it. This CL adds checks to avoid such situations: - push notifications received from GCM while shutting down will be dropped early instead of causing a crash and being dropped anyway. - Extension functions using chrome_details validate before running that the browser process is not shutting down. BUG=625646 ==========
On 2016/07/18 15:08:15, Devlin wrote: > Awesome, lgtm! > > It looks like your cl description is a little bit out of date, but otherwise > this is good. Thanks! I just updated it.
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, peter@chromium.org, johnme@chromium.org Link to the patchset: https://codereview.chromium.org/2118473002/#ps240001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [bgmode] Skip some KeepAlive registrations on shutdown. Registering a KeepAlive during shtudown is forbidden and hits a CHECK, so we should avoid starting some operations that do it. This CL adds checks to avoid such situations: - push notifications received from GCM while shutting down will be dropped early instead of causing a crash and being dropped anyway. - Extension functions using chrome_details validate before running that the browser process is not shutting down. BUG=625646 ========== to ========== [bgmode] Skip some KeepAlive registrations on shutdown. Registering a KeepAlive during shtudown is forbidden and hits a CHECK, so we should avoid starting some operations that do it. This CL adds checks to avoid such situations: - push notifications received from GCM while shutting down will be dropped early instead of causing a crash and being dropped anyway. - Extension functions using chrome_details validate before running that the browser process is not shutting down. BUG=625646 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [bgmode] Skip some KeepAlive registrations on shutdown. Registering a KeepAlive during shtudown is forbidden and hits a CHECK, so we should avoid starting some operations that do it. This CL adds checks to avoid such situations: - push notifications received from GCM while shutting down will be dropped early instead of causing a crash and being dropped anyway. - Extension functions using chrome_details validate before running that the browser process is not shutting down. BUG=625646 ========== to ========== [bgmode] Skip some KeepAlive registrations on shutdown. Registering a KeepAlive during shtudown is forbidden and hits a CHECK, so we should avoid starting some operations that do it. This CL adds checks to avoid such situations: - push notifications received from GCM while shutting down will be dropped early instead of causing a crash and being dropped anyway. - Extension functions using chrome_details validate before running that the browser process is not shutting down. BUG=625646 Committed: https://crrev.com/43f25e4fc136ddba3b516c329ac375ce9185b6b3 Cr-Commit-Position: refs/heads/master@{#406001} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/43f25e4fc136ddba3b516c329ac375ce9185b6b3 Cr-Commit-Position: refs/heads/master@{#406001} |
