Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1647443002:
Delay destructing ChildDiscardableSharedMemoryManager (Closed)
DescriptionDelay destructing ChildDiscardableSharedMemoryManager
The current shutdown sequence is as follows:
1) ChildThreadImpl::Shutdown
2) ChildDiscardableSharedMemoryManager gets destructed
3) Message loop is closed
4) blink::shutdown
This is problematic because blink::shutdown triggers an Oilpan's GC,
which destructs some Resources in Blink, which calls ChildDiscardableSharedMemoryManager::Unlock.
However, the ChildDiscardableSharedMemoryManager is already destructed, so it crashes.
Thus this CL moves 2) to after 4).
BUG=581593
Committed: https://crrev.com/534ab85fcdb3210d6ac8250a146a3aa68c1079cf
Cr-Commit-Position: refs/heads/master@{#372283}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Messages
Total messages: 22 (9 generated)
haraken@chromium.org changed reviewers: + jochen@chromium.org, oilpan-reviews@chromium.org
jochen@: PTAL Enabling Oilpan exposed the crash, but I guess the potential bug had already been in non-Oilpan as well.
lgtm
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1647443002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1647443002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Description was changed from ========== Delay destructing ChildDiscardableSharedMemoryManager The current shutdown sequence is as follows: 1) ChildThreadImpl::Shutdown 2) ChildDiscardableSharedMemoryManager gets destructed 3) Message loop is closed 4) blink::shutdown This is problematic because blink::shutdown triggers an Oilpan's GC, which destructs some Resources in Blink, which calls ChildDiscardableSharedMemoryManager::Unlock. However, the ChildDiscardableSharedMemoryManager is already destructed, so it crashes. One solution would be to move 2) to after 4), but this is not doable because we have the constraints that 2) must happen before 3) and 3) must happen before 4). So this CL introduces ChildDiscardableSharedMemoryManager::Shutdown and calls it in 2). The destruction of the ChildDiscardableSharedMemoryManager is delayed until after 4) and thus fixes the crash. BUG=581593 ========== to ========== Delay destructing ChildDiscardableSharedMemoryManager The current shutdown sequence is as follows: 1) ChildThreadImpl::Shutdown 2) ChildDiscardableSharedMemoryManager gets destructed 3) Message loop is closed 4) blink::shutdown This is problematic because blink::shutdown triggers an Oilpan's GC, which destructs some Resources in Blink, which calls ChildDiscardableSharedMemoryManager::Unlock. However, the ChildDiscardableSharedMemoryManager is already destructed, so it crashes. Thus this CL moves 2) to after 4). BUG=581593 ==========
haraken@chromium.org changed reviewers: + reveman@chromium.org
OK, now all tests pass. reveman@: PTAL. You moved ChildDiscardableSharedMemoryManager's destruction to a time when the message loop is still valid, but it caused other crashes (see the bug). It looks like that the timing is a bit too early. This CL delays the timing a bit. It moves the destruction to a time before RenderThreadImpl gets destructed but after all threads that are using discardable memory have shut down.
On 2016/01/28 at 08:40:02, haraken wrote: > OK, now all tests pass. > > reveman@: PTAL. > > You moved ChildDiscardableSharedMemoryManager's destruction to a time when the message loop is still valid, but it caused other crashes (see the bug). It looks like that the timing is a bit too early. > > This CL delays the timing a bit. It moves the destruction to a time before RenderThreadImpl gets destructed but after all threads that are using discardable memory have shut down. Hm, all threads using discardable having been shut down is not a guarantee that ChildDiscardableSharedMemoryManager has released all segments. Segments are kept around for a short amount of time before they are released to avoid unnecessary round-trips to the browser process. ChildDiscardableSharedMemoryManager needs a working message loop to release the segments but it looks like that is not going to be the case anymore after this patch lands. Can we move ChildThreadImpl::Shutdown to where you call ChildThreadImpl::ShutdownDiscardableSharedMemoryManager in this patch and also move main_message_loop_.reset() below the call to ChildThreadImpl::Shutdown?
On 2016/01/28 23:50:58, reveman wrote: > On 2016/01/28 at 08:40:02, haraken wrote: > > OK, now all tests pass. > > > > reveman@: PTAL. > > > > You moved ChildDiscardableSharedMemoryManager's destruction to a time when the > message loop is still valid, but it caused other crashes (see the bug). It looks > like that the timing is a bit too early. > > > > This CL delays the timing a bit. It moves the destruction to a time before > RenderThreadImpl gets destructed but after all threads that are using > discardable memory have shut down. > > Hm, all threads using discardable having been shut down is not a guarantee that > ChildDiscardableSharedMemoryManager has released all segments. Segments are kept > around for a short amount of time before they are released to avoid unnecessary > round-trips to the browser process. ChildDiscardableSharedMemoryManager needs a > working message loop to release the segments but it looks like that is not going > to be the case anymore after this patch lands. > > Can we move ChildThreadImpl::Shutdown to where you call > ChildThreadImpl::ShutdownDiscardableSharedMemoryManager in this patch and also > move main_message_loop_.reset() below the call to ChildThreadImpl::Shutdown? There is another shutdown ordering problem :/ - blink::shutdown() runs Oilpan's GCs and they must collect all garbage. To guarantee the fact, ChildThreadImpl::Shutdown() must be called before blink::shutdown() to release all pointers to Chromium to Blink. - main_message_loop_.reset() must be called before blink::shutdown(). See the comment above the renderer_scheduler_->Shutdown() in render_thread_impl.cc. Any idea?
On 2016/01/28 at 23:56:00, haraken wrote: > On 2016/01/28 23:50:58, reveman wrote: > > On 2016/01/28 at 08:40:02, haraken wrote: > > > OK, now all tests pass. > > > > > > reveman@: PTAL. > > > > > > You moved ChildDiscardableSharedMemoryManager's destruction to a time when the > > message loop is still valid, but it caused other crashes (see the bug). It looks > > like that the timing is a bit too early. > > > > > > This CL delays the timing a bit. It moves the destruction to a time before > > RenderThreadImpl gets destructed but after all threads that are using > > discardable memory have shut down. > > > > Hm, all threads using discardable having been shut down is not a guarantee that > > ChildDiscardableSharedMemoryManager has released all segments. Segments are kept > > around for a short amount of time before they are released to avoid unnecessary > > round-trips to the browser process. ChildDiscardableSharedMemoryManager needs a > > working message loop to release the segments but it looks like that is not going > > to be the case anymore after this patch lands. > > > > Can we move ChildThreadImpl::Shutdown to where you call > > ChildThreadImpl::ShutdownDiscardableSharedMemoryManager in this patch and also > > move main_message_loop_.reset() below the call to ChildThreadImpl::Shutdown? > > There is another shutdown ordering problem :/ > > - blink::shutdown() runs Oilpan's GCs and they must collect all garbage. To guarantee the fact, ChildThreadImpl::Shutdown() must be called before blink::shutdown() to release all pointers to Chromium to Blink. > > - main_message_loop_.reset() must be called before blink::shutdown(). See the comment above the renderer_scheduler_->Shutdown() in render_thread_impl.cc. > > Any idea? Could we just run the message loop until idle before shutting down blink and then destroy it after calling ChildThreadImpl::Shutdown()?
On 2016/01/29 01:56:57, reveman wrote: > On 2016/01/28 at 23:56:00, haraken wrote: > > On 2016/01/28 23:50:58, reveman wrote: > > > On 2016/01/28 at 08:40:02, haraken wrote: > > > > OK, now all tests pass. > > > > > > > > reveman@: PTAL. > > > > > > > > You moved ChildDiscardableSharedMemoryManager's destruction to a time when > the > > > message loop is still valid, but it caused other crashes (see the bug). It > looks > > > like that the timing is a bit too early. > > > > > > > > This CL delays the timing a bit. It moves the destruction to a time before > > > RenderThreadImpl gets destructed but after all threads that are using > > > discardable memory have shut down. > > > > > > Hm, all threads using discardable having been shut down is not a guarantee > that > > > ChildDiscardableSharedMemoryManager has released all segments. Segments are > kept > > > around for a short amount of time before they are released to avoid > unnecessary > > > round-trips to the browser process. ChildDiscardableSharedMemoryManager > needs a > > > working message loop to release the segments but it looks like that is not > going > > > to be the case anymore after this patch lands. > > > > > > Can we move ChildThreadImpl::Shutdown to where you call > > > ChildThreadImpl::ShutdownDiscardableSharedMemoryManager in this patch and > also > > > move main_message_loop_.reset() below the call to ChildThreadImpl::Shutdown? > > > > There is another shutdown ordering problem :/ > > > > - blink::shutdown() runs Oilpan's GCs and they must collect all garbage. To > guarantee the fact, ChildThreadImpl::Shutdown() must be called before > blink::shutdown() to release all pointers to Chromium to Blink. > > > > - main_message_loop_.reset() must be called before blink::shutdown(). See the > comment above the renderer_scheduler_->Shutdown() in render_thread_impl.cc. > > > > Any idea? > > Could we just run the message loop until idle before shutting down blink and > then destroy it after calling ChildThreadImpl::Shutdown()? This could work. Update the CL. PTAL (assuming that the try bots pass).
lgtm
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1647443002/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1647443002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1647443002/160001
Message was sent while issue was closed.
Description was changed from ========== Delay destructing ChildDiscardableSharedMemoryManager The current shutdown sequence is as follows: 1) ChildThreadImpl::Shutdown 2) ChildDiscardableSharedMemoryManager gets destructed 3) Message loop is closed 4) blink::shutdown This is problematic because blink::shutdown triggers an Oilpan's GC, which destructs some Resources in Blink, which calls ChildDiscardableSharedMemoryManager::Unlock. However, the ChildDiscardableSharedMemoryManager is already destructed, so it crashes. Thus this CL moves 2) to after 4). BUG=581593 ========== to ========== Delay destructing ChildDiscardableSharedMemoryManager The current shutdown sequence is as follows: 1) ChildThreadImpl::Shutdown 2) ChildDiscardableSharedMemoryManager gets destructed 3) Message loop is closed 4) blink::shutdown This is problematic because blink::shutdown triggers an Oilpan's GC, which destructs some Resources in Blink, which calls ChildDiscardableSharedMemoryManager::Unlock. However, the ChildDiscardableSharedMemoryManager is already destructed, so it crashes. Thus this CL moves 2) to after 4). BUG=581593 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Delay destructing ChildDiscardableSharedMemoryManager The current shutdown sequence is as follows: 1) ChildThreadImpl::Shutdown 2) ChildDiscardableSharedMemoryManager gets destructed 3) Message loop is closed 4) blink::shutdown This is problematic because blink::shutdown triggers an Oilpan's GC, which destructs some Resources in Blink, which calls ChildDiscardableSharedMemoryManager::Unlock. However, the ChildDiscardableSharedMemoryManager is already destructed, so it crashes. Thus this CL moves 2) to after 4). BUG=581593 ========== to ========== Delay destructing ChildDiscardableSharedMemoryManager The current shutdown sequence is as follows: 1) ChildThreadImpl::Shutdown 2) ChildDiscardableSharedMemoryManager gets destructed 3) Message loop is closed 4) blink::shutdown This is problematic because blink::shutdown triggers an Oilpan's GC, which destructs some Resources in Blink, which calls ChildDiscardableSharedMemoryManager::Unlock. However, the ChildDiscardableSharedMemoryManager is already destructed, so it crashes. Thus this CL moves 2) to after 4). BUG=581593 Committed: https://crrev.com/534ab85fcdb3210d6ac8250a146a3aa68c1079cf Cr-Commit-Position: refs/heads/master@{#372283} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/534ab85fcdb3210d6ac8250a146a3aa68c1079cf Cr-Commit-Position: refs/heads/master@{#372283} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
