Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2163683004:
Ignore mojo connection errors during shutdown in Geolocation. (Closed)
|
Created:
4 years, 5 months ago by Sam McNally Modified:
4 years, 4 months ago CC:
chromium-reviews, blink-reviews, mvanouwerkerk+watch_chromium.org, mlamouri+watch-blink_chromium.org, timvolodine, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIgnore mojo connection errors during shutdown in Geolocation.
BUG=628999
Committed: https://crrev.com/29010a784ba04202699665087996cdbc1143c21c
Cr-Commit-Position: refs/heads/master@{#411555}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #Patch Set 3 : #Messages
Total messages: 36 (18 generated)
The CQ bit was checked by sammc@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
The CQ bit was checked by sammc@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...
sammc@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right): https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: if (!Platform::current()) It looks nasty to have this kind of check in Blink. Would there be any way to prevent the onerror callbacks from getting called after blink::shutdown is called?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right): https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: if (!Platform::current()) On 2016/07/20 07:15:01, haraken wrote: > > It looks nasty to have this kind of check in Blink. Would there be any way to > prevent the onerror callbacks from getting called after blink::shutdown is > called? > This does seem a little out of place. It seems that either 1) we should put this type of checks in many more places throughout Blink or 2) a more generic solution in a single place would be cleaner.
On 2016/07/20 09:40:50, Michael van Ouwerkerk wrote: > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right): > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: if > (!Platform::current()) > On 2016/07/20 07:15:01, haraken wrote: > > > > It looks nasty to have this kind of check in Blink. Would there be any way to > > prevent the onerror callbacks from getting called after blink::shutdown is > > called? > > > > This does seem a little out of place. It seems that either 1) we should put this > type of checks in many more places throughout Blink or 2) a more generic > solution in a single place would be cleaner. We normally clear a content-side object before content/ calls blink::shutdown so that the content-side object doesn't call back Blink after blink::shutdown. It's a responsibility of content/ to make sure that it doesn't call in Blink after calling blink::shutdown.
On 2016/07/20 09:43:52, haraken wrote: > On 2016/07/20 09:40:50, Michael van Ouwerkerk wrote: > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right): > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: if > > (!Platform::current()) > > On 2016/07/20 07:15:01, haraken wrote: > > > > > > It looks nasty to have this kind of check in Blink. Would there be any way > to > > > prevent the onerror callbacks from getting called after blink::shutdown is > > > called? > > > > > > > This does seem a little out of place. It seems that either 1) we should put > this > > type of checks in many more places throughout Blink or 2) a more generic > > solution in a single place would be cleaner. > > We normally clear a content-side object before content/ calls blink::shutdown so > that the content-side object doesn't call back Blink after blink::shutdown. It's > a responsibility of content/ to make sure that it doesn't call in Blink after > calling blink::shutdown. Mojo doesn't work that way. Mojo bindings that are still active at message loop shutdown receive an error; content/ isn't involved. If blink doesn't want to receive callbacks after it shuts down, it needs to reset or delete its InterfacePtrs at or before shutdown, or at least invalidate any Persistent pointers so they can provide WeakPtr-style semantics.
On 2016/08/04 07:30:49, Sam McNally wrote: > On 2016/07/20 09:43:52, haraken wrote: > > On 2016/07/20 09:40:50, Michael van Ouwerkerk wrote: > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp (right): > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: if > > > (!Platform::current()) > > > On 2016/07/20 07:15:01, haraken wrote: > > > > > > > > It looks nasty to have this kind of check in Blink. Would there be any way > > to > > > > prevent the onerror callbacks from getting called after blink::shutdown is > > > > called? > > > > > > > > > > This does seem a little out of place. It seems that either 1) we should put > > this > > > type of checks in many more places throughout Blink or 2) a more generic > > > solution in a single place would be cleaner. > > > > We normally clear a content-side object before content/ calls blink::shutdown > so > > that the content-side object doesn't call back Blink after blink::shutdown. > It's > > a responsibility of content/ to make sure that it doesn't call in Blink after > > calling blink::shutdown. > > Mojo doesn't work that way. Mojo bindings that are still active at message loop > shutdown receive an error; content/ isn't involved. If blink doesn't want to > receive callbacks after it shuts down, it needs to reset or delete its > InterfacePtrs at or before shutdown, or at least invalidate any Persistent > pointers so they can provide WeakPtr-style semantics. Is there any way to shutdown all outstanding mojo interfaces from content/? Specifically, I want to shutdown all mojo interfaces just before calling blink::shutdown in RenderThreadImpl::Shutdown(). (On the other hand, I'm now trying to stop calling blink::shutdown because blink::shutdown causes a lot of complicated issues like this.)
sammc@chromium.org changed reviewers: + rockot@chromium.org, yzshen@chromium.org
On 2016/08/04 07:51:06, haraken wrote: > On 2016/08/04 07:30:49, Sam McNally wrote: > > On 2016/07/20 09:43:52, haraken wrote: > > > On 2016/07/20 09:40:50, Michael van Ouwerkerk wrote: > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: if > > > > (!Platform::current()) > > > > On 2016/07/20 07:15:01, haraken wrote: > > > > > > > > > > It looks nasty to have this kind of check in Blink. Would there be any > way > > > to > > > > > prevent the onerror callbacks from getting called after blink::shutdown > is > > > > > called? > > > > > > > > > > > > > This does seem a little out of place. It seems that either 1) we should > put > > > this > > > > type of checks in many more places throughout Blink or 2) a more generic > > > > solution in a single place would be cleaner. > > > > > > We normally clear a content-side object before content/ calls > blink::shutdown > > so > > > that the content-side object doesn't call back Blink after blink::shutdown. > > It's > > > a responsibility of content/ to make sure that it doesn't call in Blink > after > > > calling blink::shutdown. > > > > Mojo doesn't work that way. Mojo bindings that are still active at message > loop > > shutdown receive an error; content/ isn't involved. If blink doesn't want to > > receive callbacks after it shuts down, it needs to reset or delete its > > InterfacePtrs at or before shutdown, or at least invalidate any Persistent > > pointers so they can provide WeakPtr-style semantics. > > Is there any way to shutdown all outstanding mojo interfaces from content/? > Specifically, I want to shutdown all mojo interfaces just before calling > blink::shutdown in RenderThreadImpl::Shutdown(). > > (On the other hand, I'm now trying to stop calling blink::shutdown because > blink::shutdown causes a lot of complicated issues like this.) I don't know of anything currently, but it seems possible to add one. +rockot and yzshen for more opinions.
On 2016/08/05 at 00:47:20, sammc wrote: > On 2016/08/04 07:51:06, haraken wrote: > > On 2016/08/04 07:30:49, Sam McNally wrote: > > > On 2016/07/20 09:43:52, haraken wrote: > > > > On 2016/07/20 09:40:50, Michael van Ouwerkerk wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp > > (right): > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: if > > > > > (!Platform::current()) > > > > > On 2016/07/20 07:15:01, haraken wrote: > > > > > > > > > > > > It looks nasty to have this kind of check in Blink. Would there be any > > way > > > > to > > > > > > prevent the onerror callbacks from getting called after blink::shutdown > > is > > > > > > called? > > > > > > > > > > > > > > > > This does seem a little out of place. It seems that either 1) we should > > put > > > > this > > > > > type of checks in many more places throughout Blink or 2) a more generic > > > > > solution in a single place would be cleaner. > > > > > > > > We normally clear a content-side object before content/ calls > > blink::shutdown > > > so > > > > that the content-side object doesn't call back Blink after blink::shutdown. > > > It's > > > > a responsibility of content/ to make sure that it doesn't call in Blink > > after > > > > calling blink::shutdown. > > > > > > Mojo doesn't work that way. Mojo bindings that are still active at message > > loop > > > shutdown receive an error; content/ isn't involved. If blink doesn't want to > > > receive callbacks after it shuts down, it needs to reset or delete its > > > InterfacePtrs at or before shutdown, or at least invalidate any Persistent > > > pointers so they can provide WeakPtr-style semantics. > > > > Is there any way to shutdown all outstanding mojo interfaces from content/? > > Specifically, I want to shutdown all mojo interfaces just before calling > > blink::shutdown in RenderThreadImpl::Shutdown(). > > > > (On the other hand, I'm now trying to stop calling blink::shutdown because > > blink::shutdown causes a lot of complicated issues like this.) > > I don't know of anything currently, but it seems possible to add one. > > +rockot and yzshen for more opinions. It's certainly possible to add one. It would essentially be a global registry of mojo::Watcher instances, and shutting them all down would just mean signaling them all with MOJO_RESULT_ABORTED, as we do on MessageLoop destruction anyway. It seems a bit heavy-handed to do this though. Is it really necessary?
On 2016/08/08 15:43:51, Ken Rockot wrote: > On 2016/08/05 at 00:47:20, sammc wrote: > > On 2016/08/04 07:51:06, haraken wrote: > > > On 2016/08/04 07:30:49, Sam McNally wrote: > > > > On 2016/07/20 09:43:52, haraken wrote: > > > > > On 2016/07/20 09:40:50, Michael van Ouwerkerk wrote: > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: if > > > > > > (!Platform::current()) > > > > > > On 2016/07/20 07:15:01, haraken wrote: > > > > > > > > > > > > > > It looks nasty to have this kind of check in Blink. Would there be > any > > > way > > > > > to > > > > > > > prevent the onerror callbacks from getting called after > blink::shutdown > > > is > > > > > > > called? > > > > > > > > > > > > > > > > > > > This does seem a little out of place. It seems that either 1) we > should > > > put > > > > > this > > > > > > type of checks in many more places throughout Blink or 2) a more > generic > > > > > > solution in a single place would be cleaner. > > > > > > > > > > We normally clear a content-side object before content/ calls > > > blink::shutdown > > > > so > > > > > that the content-side object doesn't call back Blink after > blink::shutdown. > > > > It's > > > > > a responsibility of content/ to make sure that it doesn't call in Blink > > > after > > > > > calling blink::shutdown. > > > > > > > > Mojo doesn't work that way. Mojo bindings that are still active at message > > > loop > > > > shutdown receive an error; content/ isn't involved. If blink doesn't want > to > > > > receive callbacks after it shuts down, it needs to reset or delete its > > > > InterfacePtrs at or before shutdown, or at least invalidate any Persistent > > > > pointers so they can provide WeakPtr-style semantics. > > > > > > Is there any way to shutdown all outstanding mojo interfaces from content/? > > > Specifically, I want to shutdown all mojo interfaces just before calling > > > blink::shutdown in RenderThreadImpl::Shutdown(). > > > > > > (On the other hand, I'm now trying to stop calling blink::shutdown because > > > blink::shutdown causes a lot of complicated issues like this.) > > > > I don't know of anything currently, but it seems possible to add one. > > > > +rockot and yzshen for more opinions. > > It's certainly possible to add one. It would essentially be a global registry of > mojo::Watcher instances, and shutting them all down would just mean signaling > them all with MOJO_RESULT_ABORTED, as we do on MessageLoop destruction anyway. > It seems a bit heavy-handed to do this though. Is it really necessary? If we need a graceful shutdown sequence (just like today), it's necessary. However, we're aiming at deprecating the graceful shutdown sequence (i.e., remove blink::shutdown, message_loop_.reset() etc). So this wouldn't be a place we should invest.
On 2016/08/08 23:36:51, haraken wrote: > On 2016/08/08 15:43:51, Ken Rockot wrote: > > On 2016/08/05 at 00:47:20, sammc wrote: > > > On 2016/08/04 07:51:06, haraken wrote: > > > > On 2016/08/04 07:30:49, Sam McNally wrote: > > > > > On 2016/07/20 09:43:52, haraken wrote: > > > > > > On 2016/07/20 09:40:50, Michael van Ouwerkerk wrote: > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > > File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: > if > > > > > > > (!Platform::current()) > > > > > > > On 2016/07/20 07:15:01, haraken wrote: > > > > > > > > > > > > > > > > It looks nasty to have this kind of check in Blink. Would there be > > any > > > > way > > > > > > to > > > > > > > > prevent the onerror callbacks from getting called after > > blink::shutdown > > > > is > > > > > > > > called? > > > > > > > > > > > > > > > > > > > > > > This does seem a little out of place. It seems that either 1) we > > should > > > > put > > > > > > this > > > > > > > type of checks in many more places throughout Blink or 2) a more > > generic > > > > > > > solution in a single place would be cleaner. > > > > > > > > > > > > We normally clear a content-side object before content/ calls > > > > blink::shutdown > > > > > so > > > > > > that the content-side object doesn't call back Blink after > > blink::shutdown. > > > > > It's > > > > > > a responsibility of content/ to make sure that it doesn't call in > Blink > > > > after > > > > > > calling blink::shutdown. > > > > > > > > > > Mojo doesn't work that way. Mojo bindings that are still active at > message > > > > loop > > > > > shutdown receive an error; content/ isn't involved. If blink doesn't > want > > to > > > > > receive callbacks after it shuts down, it needs to reset or delete its > > > > > InterfacePtrs at or before shutdown, or at least invalidate any > Persistent > > > > > pointers so they can provide WeakPtr-style semantics. > > > > > > > > Is there any way to shutdown all outstanding mojo interfaces from > content/? > > > > Specifically, I want to shutdown all mojo interfaces just before calling > > > > blink::shutdown in RenderThreadImpl::Shutdown(). > > > > > > > > (On the other hand, I'm now trying to stop calling blink::shutdown because > > > > blink::shutdown causes a lot of complicated issues like this.) > > > > > > I don't know of anything currently, but it seems possible to add one. > > > > > > +rockot and yzshen for more opinions. > > > > It's certainly possible to add one. It would essentially be a global registry > of > > mojo::Watcher instances, and shutting them all down would just mean signaling > > them all with MOJO_RESULT_ABORTED, as we do on MessageLoop destruction anyway. > > It seems a bit heavy-handed to do this though. Is it really necessary? > > If we need a graceful shutdown sequence (just like today), it's necessary. > However, we're aiming at deprecating the graceful shutdown sequence (i.e., > remove blink::shutdown, message_loop_.reset() etc). So this wouldn't be a place > we should invest. Given these plans, I'd like to land this CL (+ TODOs explaining it) as a workaround for the current problem, to be removed when shutdown is not graceful.
On 2016/08/10 23:59:23, Sam McNally wrote: > On 2016/08/08 23:36:51, haraken wrote: > > On 2016/08/08 15:43:51, Ken Rockot wrote: > > > On 2016/08/05 at 00:47:20, sammc wrote: > > > > On 2016/08/04 07:51:06, haraken wrote: > > > > > On 2016/08/04 07:30:49, Sam McNally wrote: > > > > > > On 2016/07/20 09:43:52, haraken wrote: > > > > > > > On 2016/07/20 09:40:50, Michael van Ouwerkerk wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > > > File third_party/WebKit/Source/modules/geolocation/Geolocation.cpp > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > > > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: > > if > > > > > > > > (!Platform::current()) > > > > > > > > On 2016/07/20 07:15:01, haraken wrote: > > > > > > > > > > > > > > > > > > It looks nasty to have this kind of check in Blink. Would there > be > > > any > > > > > way > > > > > > > to > > > > > > > > > prevent the onerror callbacks from getting called after > > > blink::shutdown > > > > > is > > > > > > > > > called? > > > > > > > > > > > > > > > > > > > > > > > > > This does seem a little out of place. It seems that either 1) we > > > should > > > > > put > > > > > > > this > > > > > > > > type of checks in many more places throughout Blink or 2) a more > > > generic > > > > > > > > solution in a single place would be cleaner. > > > > > > > > > > > > > > We normally clear a content-side object before content/ calls > > > > > blink::shutdown > > > > > > so > > > > > > > that the content-side object doesn't call back Blink after > > > blink::shutdown. > > > > > > It's > > > > > > > a responsibility of content/ to make sure that it doesn't call in > > Blink > > > > > after > > > > > > > calling blink::shutdown. > > > > > > > > > > > > Mojo doesn't work that way. Mojo bindings that are still active at > > message > > > > > loop > > > > > > shutdown receive an error; content/ isn't involved. If blink doesn't > > want > > > to > > > > > > receive callbacks after it shuts down, it needs to reset or delete its > > > > > > InterfacePtrs at or before shutdown, or at least invalidate any > > Persistent > > > > > > pointers so they can provide WeakPtr-style semantics. > > > > > > > > > > Is there any way to shutdown all outstanding mojo interfaces from > > content/? > > > > > Specifically, I want to shutdown all mojo interfaces just before calling > > > > > blink::shutdown in RenderThreadImpl::Shutdown(). > > > > > > > > > > (On the other hand, I'm now trying to stop calling blink::shutdown > because > > > > > blink::shutdown causes a lot of complicated issues like this.) > > > > > > > > I don't know of anything currently, but it seems possible to add one. > > > > > > > > +rockot and yzshen for more opinions. > > > > > > It's certainly possible to add one. It would essentially be a global > registry > > of > > > mojo::Watcher instances, and shutting them all down would just mean > signaling > > > them all with MOJO_RESULT_ABORTED, as we do on MessageLoop destruction > anyway. > > > It seems a bit heavy-handed to do this though. Is it really necessary? > > > > If we need a graceful shutdown sequence (just like today), it's necessary. > > However, we're aiming at deprecating the graceful shutdown sequence (i.e., > > remove blink::shutdown, message_loop_.reset() etc). So this wouldn't be a > place > > we should invest. > > Given these plans, I'd like to land this CL (+ TODOs explaining it) as a > workaround for the current problem, to be removed when shutdown is not graceful. As a workaround, I'd prefer trying the approach Rockot suggested in #19 rather than landing this CL, because this wouldn't be an issue limited to Geolocation. Rockot: How heavy would it be? We're already doing a ton of things in RenderThreadImpl::Shutdown including message_loop_.RunUntiIdle(), multiple Oilpan's GCs etc. I guess the overhead of shutting down mojo::Watcher instances would be ignorable.
On Aug 10, 2016 8:12 PM, <haraken@chromium.org> wrote: > > On 2016/08/10 23:59:23, Sam McNally wrote: > > On 2016/08/08 23:36:51, haraken wrote: > > > On 2016/08/08 15:43:51, Ken Rockot wrote: > > > > On 2016/08/05 at 00:47:20, sammc wrote: > > > > > On 2016/08/04 07:51:06, haraken wrote: > > > > > > On 2016/08/04 07:30:49, Sam McNally wrote: > > > > > > > On 2016/07/20 09:43:52, haraken wrote: > > > > > > > > On 2016/07/20 09:40:50, Michael van Ouwerkerk wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > > > > File > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > > > > > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: > > > if > > > > > > > > > (!Platform::current()) > > > > > > > > > On 2016/07/20 07:15:01, haraken wrote: > > > > > > > > > > > > > > > > > > > > It looks nasty to have this kind of check in Blink. Would > there > > be > > > > any > > > > > > way > > > > > > > > to > > > > > > > > > > prevent the onerror callbacks from getting called after > > > > blink::shutdown > > > > > > is > > > > > > > > > > called? > > > > > > > > > > > > > > > > > > > > > > > > > > > > This does seem a little out of place. It seems that either 1) we > > > > should > > > > > > put > > > > > > > > this > > > > > > > > > type of checks in many more places throughout Blink or 2) a more > > > > generic > > > > > > > > > solution in a single place would be cleaner. > > > > > > > > > > > > > > > > We normally clear a content-side object before content/ calls > > > > > > blink::shutdown > > > > > > > so > > > > > > > > that the content-side object doesn't call back Blink after > > > > blink::shutdown. > > > > > > > It's > > > > > > > > a responsibility of content/ to make sure that it doesn't call in > > > Blink > > > > > > after > > > > > > > > calling blink::shutdown. > > > > > > > > > > > > > > Mojo doesn't work that way. Mojo bindings that are still active at > > > message > > > > > > loop > > > > > > > shutdown receive an error; content/ isn't involved. If blink doesn't > > > want > > > > to > > > > > > > receive callbacks after it shuts down, it needs to reset or delete > its > > > > > > > InterfacePtrs at or before shutdown, or at least invalidate any > > > Persistent > > > > > > > pointers so they can provide WeakPtr-style semantics. > > > > > > > > > > > > Is there any way to shutdown all outstanding mojo interfaces from > > > content/? > > > > > > Specifically, I want to shutdown all mojo interfaces just before > calling > > > > > > blink::shutdown in RenderThreadImpl::Shutdown(). > > > > > > > > > > > > (On the other hand, I'm now trying to stop calling blink::shutdown > > because > > > > > > blink::shutdown causes a lot of complicated issues like this.) > > > > > > > > > > I don't know of anything currently, but it seems possible to add one. > > > > > > > > > > +rockot and yzshen for more opinions. > > > > > > > > It's certainly possible to add one. It would essentially be a global > > registry > > > of > > > > mojo::Watcher instances, and shutting them all down would just mean > > signaling > > > > them all with MOJO_RESULT_ABORTED, as we do on MessageLoop destruction > > anyway. > > > > It seems a bit heavy-handed to do this though. Is it really necessary? > > > > > > If we need a graceful shutdown sequence (just like today), it's necessary. > > > However, we're aiming at deprecating the graceful shutdown sequence (i.e., > > > remove blink::shutdown, message_loop_.reset() etc). So this wouldn't be a > > place > > > we should invest. > > > > Given these plans, I'd like to land this CL (+ TODOs explaining it) as a > > workaround for the current problem, to be removed when shutdown is not > graceful. > > As a workaround, I'd prefer trying the approach Rockot suggested in #19 rather > than landing this CL, because this wouldn't be an issue limited to Geolocation. > > Rockot: How heavy would it be? We're already doing a ton of things in > RenderThreadImpl::Shutdown including message_loop_.RunUntiIdle(), multiple > Oilpan's GCs etc. I guess the overhead of shutting down mojo::Watcher instances > would be ignorable. My main hesitation to doing that workaround is that it will be hard to undo once people start relying on the behavior. It seems to me that lifetime if these bindings objects should be managed from the top-down by application logic rather than at the bindings/MessageLoop layer. > > > > https://codereview.chromium.org/2163683004/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Aug 10, 2016 8:12 PM, <haraken@chromium.org> wrote: > > On 2016/08/10 23:59:23, Sam McNally wrote: > > On 2016/08/08 23:36:51, haraken wrote: > > > On 2016/08/08 15:43:51, Ken Rockot wrote: > > > > On 2016/08/05 at 00:47:20, sammc wrote: > > > > > On 2016/08/04 07:51:06, haraken wrote: > > > > > > On 2016/08/04 07:30:49, Sam McNally wrote: > > > > > > > On 2016/07/20 09:43:52, haraken wrote: > > > > > > > > On 2016/07/20 09:40:50, Michael van Ouwerkerk wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > > > > File > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > > > > > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: > > > if > > > > > > > > > (!Platform::current()) > > > > > > > > > On 2016/07/20 07:15:01, haraken wrote: > > > > > > > > > > > > > > > > > > > > It looks nasty to have this kind of check in Blink. Would > there > > be > > > > any > > > > > > way > > > > > > > > to > > > > > > > > > > prevent the onerror callbacks from getting called after > > > > blink::shutdown > > > > > > is > > > > > > > > > > called? > > > > > > > > > > > > > > > > > > > > > > > > > > > > This does seem a little out of place. It seems that either 1) we > > > > should > > > > > > put > > > > > > > > this > > > > > > > > > type of checks in many more places throughout Blink or 2) a more > > > > generic > > > > > > > > > solution in a single place would be cleaner. > > > > > > > > > > > > > > > > We normally clear a content-side object before content/ calls > > > > > > blink::shutdown > > > > > > > so > > > > > > > > that the content-side object doesn't call back Blink after > > > > blink::shutdown. > > > > > > > It's > > > > > > > > a responsibility of content/ to make sure that it doesn't call in > > > Blink > > > > > > after > > > > > > > > calling blink::shutdown. > > > > > > > > > > > > > > Mojo doesn't work that way. Mojo bindings that are still active at > > > message > > > > > > loop > > > > > > > shutdown receive an error; content/ isn't involved. If blink doesn't > > > want > > > > to > > > > > > > receive callbacks after it shuts down, it needs to reset or delete > its > > > > > > > InterfacePtrs at or before shutdown, or at least invalidate any > > > Persistent > > > > > > > pointers so they can provide WeakPtr-style semantics. > > > > > > > > > > > > Is there any way to shutdown all outstanding mojo interfaces from > > > content/? > > > > > > Specifically, I want to shutdown all mojo interfaces just before > calling > > > > > > blink::shutdown in RenderThreadImpl::Shutdown(). > > > > > > > > > > > > (On the other hand, I'm now trying to stop calling blink::shutdown > > because > > > > > > blink::shutdown causes a lot of complicated issues like this.) > > > > > > > > > > I don't know of anything currently, but it seems possible to add one. > > > > > > > > > > +rockot and yzshen for more opinions. > > > > > > > > It's certainly possible to add one. It would essentially be a global > > registry > > > of > > > > mojo::Watcher instances, and shutting them all down would just mean > > signaling > > > > them all with MOJO_RESULT_ABORTED, as we do on MessageLoop destruction > > anyway. > > > > It seems a bit heavy-handed to do this though. Is it really necessary? > > > > > > If we need a graceful shutdown sequence (just like today), it's necessary. > > > However, we're aiming at deprecating the graceful shutdown sequence (i.e., > > > remove blink::shutdown, message_loop_.reset() etc). So this wouldn't be a > > place > > > we should invest. > > > > Given these plans, I'd like to land this CL (+ TODOs explaining it) as a > > workaround for the current problem, to be removed when shutdown is not > graceful. > > As a workaround, I'd prefer trying the approach Rockot suggested in #19 rather > than landing this CL, because this wouldn't be an issue limited to Geolocation. > > Rockot: How heavy would it be? We're already doing a ton of things in > RenderThreadImpl::Shutdown including message_loop_.RunUntiIdle(), multiple > Oilpan's GCs etc. I guess the overhead of shutting down mojo::Watcher instances > would be ignorable. My main hesitation to doing that workaround is that it will be hard to undo once people start relying on the behavior. It seems to me that lifetime if these bindings objects should be managed from the top-down by application logic rather than at the bindings/MessageLoop layer. > > > > https://codereview.chromium.org/2163683004/ -- 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.
On 2016/08/11 04:25:49, Ken Rockot (OOO 8.11-8.16) wrote: > On Aug 10, 2016 8:12 PM, <mailto:haraken@chromium.org> wrote: > > > > On 2016/08/10 23:59:23, Sam McNally wrote: > > > On 2016/08/08 23:36:51, haraken wrote: > > > > On 2016/08/08 15:43:51, Ken Rockot wrote: > > > > > On 2016/08/05 at 00:47:20, sammc wrote: > > > > > > On 2016/08/04 07:51:06, haraken wrote: > > > > > > > On 2016/08/04 07:30:49, Sam McNally wrote: > > > > > > > > On 2016/07/20 09:43:52, haraken wrote: > > > > > > > > > On 2016/07/20 09:40:50, Michael van Ouwerkerk wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > > > > > File > > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > > > > > > > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: > > > > if > > > > > > > > > > (!Platform::current()) > > > > > > > > > > On 2016/07/20 07:15:01, haraken wrote: > > > > > > > > > > > > > > > > > > > > > > It looks nasty to have this kind of check in Blink. > Would > > there > > > be > > > > > any > > > > > > > way > > > > > > > > > to > > > > > > > > > > > prevent the onerror callbacks from getting called after > > > > > blink::shutdown > > > > > > > is > > > > > > > > > > > called? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This does seem a little out of place. It seems that > either 1) we > > > > > should > > > > > > > put > > > > > > > > > this > > > > > > > > > > type of checks in many more places throughout Blink or 2) > a more > > > > > generic > > > > > > > > > > solution in a single place would be cleaner. > > > > > > > > > > > > > > > > > > We normally clear a content-side object before content/ > calls > > > > > > > blink::shutdown > > > > > > > > so > > > > > > > > > that the content-side object doesn't call back Blink after > > > > > blink::shutdown. > > > > > > > > It's > > > > > > > > > a responsibility of content/ to make sure that it doesn't > call in > > > > Blink > > > > > > > after > > > > > > > > > calling blink::shutdown. > > > > > > > > > > > > > > > > Mojo doesn't work that way. Mojo bindings that are still > active at > > > > message > > > > > > > loop > > > > > > > > shutdown receive an error; content/ isn't involved. If blink > doesn't > > > > want > > > > > to > > > > > > > > receive callbacks after it shuts down, it needs to reset or > delete > > its > > > > > > > > InterfacePtrs at or before shutdown, or at least invalidate > any > > > > Persistent > > > > > > > > pointers so they can provide WeakPtr-style semantics. > > > > > > > > > > > > > > Is there any way to shutdown all outstanding mojo interfaces > from > > > > content/? > > > > > > > Specifically, I want to shutdown all mojo interfaces just before > > calling > > > > > > > blink::shutdown in RenderThreadImpl::Shutdown(). > > > > > > > > > > > > > > (On the other hand, I'm now trying to stop calling > blink::shutdown > > > because > > > > > > > blink::shutdown causes a lot of complicated issues like this.) > > > > > > > > > > > > I don't know of anything currently, but it seems possible to add > one. > > > > > > > > > > > > +rockot and yzshen for more opinions. > > > > > > > > > > It's certainly possible to add one. It would essentially be a global > > > registry > > > > of > > > > > mojo::Watcher instances, and shutting them all down would just mean > > > signaling > > > > > them all with MOJO_RESULT_ABORTED, as we do on MessageLoop > destruction > > > anyway. > > > > > It seems a bit heavy-handed to do this though. Is it really > necessary? > > > > > > > > If we need a graceful shutdown sequence (just like today), it's > necessary. > > > > However, we're aiming at deprecating the graceful shutdown sequence > (i.e., > > > > remove blink::shutdown, message_loop_.reset() etc). So this wouldn't > be a > > > place > > > > we should invest. > > > > > > Given these plans, I'd like to land this CL (+ TODOs explaining it) as a > > > workaround for the current problem, to be removed when shutdown is not > > graceful. > > > > As a workaround, I'd prefer trying the approach Rockot suggested in #19 > rather > > than landing this CL, because this wouldn't be an issue limited to > Geolocation. > > > > Rockot: How heavy would it be? We're already doing a ton of things in > > RenderThreadImpl::Shutdown including message_loop_.RunUntiIdle(), multiple > > Oilpan's GCs etc. I guess the overhead of shutting down mojo::Watcher > instances > > would be ignorable. > > My main hesitation to doing that workaround is that it will be hard to undo > once people start relying on the behavior. It seems to me that lifetime if > these bindings objects should be managed from the top-down by application > logic rather than at the bindings/MessageLoop layer. Hmm. Then LGTM, but please add TODO. We should really try to remove the graceful shutdown sequence.
The CQ bit was checked by sammc@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...
On 2016/08/11 12:19:10, haraken wrote: > On 2016/08/11 04:25:49, Ken Rockot (OOO 8.11-8.16) wrote: > > On Aug 10, 2016 8:12 PM, <mailto:haraken@chromium.org> wrote: > > > > > > On 2016/08/10 23:59:23, Sam McNally wrote: > > > > On 2016/08/08 23:36:51, haraken wrote: > > > > > On 2016/08/08 15:43:51, Ken Rockot wrote: > > > > > > On 2016/08/05 at 00:47:20, sammc wrote: > > > > > > > On 2016/08/04 07:51:06, haraken wrote: > > > > > > > > On 2016/08/04 07:30:49, Sam McNally wrote: > > > > > > > > > On 2016/07/20 09:43:52, haraken wrote: > > > > > > > > > > On 2016/07/20 09:40:50, Michael van Ouwerkerk wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > > > > > > File > > > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp > > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2163683004/diff/1/third_party/WebKit/Source/m... > > > > > > > > > > > > > > third_party/WebKit/Source/modules/geolocation/Geolocation.cpp:533: > > > > > if > > > > > > > > > > > (!Platform::current()) > > > > > > > > > > > On 2016/07/20 07:15:01, haraken wrote: > > > > > > > > > > > > > > > > > > > > > > > > It looks nasty to have this kind of check in Blink. > > Would > > > there > > > > be > > > > > > any > > > > > > > > way > > > > > > > > > > to > > > > > > > > > > > > prevent the onerror callbacks from getting called after > > > > > > blink::shutdown > > > > > > > > is > > > > > > > > > > > > called? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This does seem a little out of place. It seems that > > either 1) we > > > > > > should > > > > > > > > put > > > > > > > > > > this > > > > > > > > > > > type of checks in many more places throughout Blink or 2) > > a more > > > > > > generic > > > > > > > > > > > solution in a single place would be cleaner. > > > > > > > > > > > > > > > > > > > > We normally clear a content-side object before content/ > > calls > > > > > > > > blink::shutdown > > > > > > > > > so > > > > > > > > > > that the content-side object doesn't call back Blink after > > > > > > blink::shutdown. > > > > > > > > > It's > > > > > > > > > > a responsibility of content/ to make sure that it doesn't > > call in > > > > > Blink > > > > > > > > after > > > > > > > > > > calling blink::shutdown. > > > > > > > > > > > > > > > > > > Mojo doesn't work that way. Mojo bindings that are still > > active at > > > > > message > > > > > > > > loop > > > > > > > > > shutdown receive an error; content/ isn't involved. If blink > > doesn't > > > > > want > > > > > > to > > > > > > > > > receive callbacks after it shuts down, it needs to reset or > > delete > > > its > > > > > > > > > InterfacePtrs at or before shutdown, or at least invalidate > > any > > > > > Persistent > > > > > > > > > pointers so they can provide WeakPtr-style semantics. > > > > > > > > > > > > > > > > Is there any way to shutdown all outstanding mojo interfaces > > from > > > > > content/? > > > > > > > > Specifically, I want to shutdown all mojo interfaces just before > > > calling > > > > > > > > blink::shutdown in RenderThreadImpl::Shutdown(). > > > > > > > > > > > > > > > > (On the other hand, I'm now trying to stop calling > > blink::shutdown > > > > because > > > > > > > > blink::shutdown causes a lot of complicated issues like this.) > > > > > > > > > > > > > > I don't know of anything currently, but it seems possible to add > > one. > > > > > > > > > > > > > > +rockot and yzshen for more opinions. > > > > > > > > > > > > It's certainly possible to add one. It would essentially be a global > > > > registry > > > > > of > > > > > > mojo::Watcher instances, and shutting them all down would just mean > > > > signaling > > > > > > them all with MOJO_RESULT_ABORTED, as we do on MessageLoop > > destruction > > > > anyway. > > > > > > It seems a bit heavy-handed to do this though. Is it really > > necessary? > > > > > > > > > > If we need a graceful shutdown sequence (just like today), it's > > necessary. > > > > > However, we're aiming at deprecating the graceful shutdown sequence > > (i.e., > > > > > remove blink::shutdown, message_loop_.reset() etc). So this wouldn't > > be a > > > > place > > > > > we should invest. > > > > > > > > Given these plans, I'd like to land this CL (+ TODOs explaining it) as a > > > > workaround for the current problem, to be removed when shutdown is not > > > graceful. > > > > > > As a workaround, I'd prefer trying the approach Rockot suggested in #19 > > rather > > > than landing this CL, because this wouldn't be an issue limited to > > Geolocation. > > > > > > Rockot: How heavy would it be? We're already doing a ton of things in > > > RenderThreadImpl::Shutdown including message_loop_.RunUntiIdle(), multiple > > > Oilpan's GCs etc. I guess the overhead of shutting down mojo::Watcher > > instances > > > would be ignorable. > > > > My main hesitation to doing that workaround is that it will be hard to undo > > once people start relying on the behavior. It seems to me that lifetime if > > these bindings objects should be managed from the top-down by application > > logic rather than at the bindings/MessageLoop layer. > > Hmm. Then LGTM, but please add TODO. Done. > > We should really try to remove the graceful shutdown sequence.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sammc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2163683004/#ps40001 (title: " ")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Ignore mojo connection errors during shutdown in Geolocation. BUG=628999 ========== to ========== Ignore mojo connection errors during shutdown in Geolocation. BUG=628999 Committed: https://crrev.com/29010a784ba04202699665087996cdbc1143c21c Cr-Commit-Position: refs/heads/master@{#411555} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/29010a784ba04202699665087996cdbc1143c21c Cr-Commit-Position: refs/heads/master@{#411555} |
