Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1442543003:
Oilpan: trace ColorChooserPopupUIController::m_chromeClient (Closed)
DescriptionOilpan: trace ColorChooserPopupUIController::m_chromeClient
R=haraken
BUG=553613
Committed: https://crrev.com/c29f20c8bd73d5c6e3c0bd45e11c1bcf738ceb37
Cr-Commit-Position: refs/heads/master@{#361043}
Patch Set 1 #
Total comments: 2
Patch Set 2 : use a WeakMember<> #Patch Set 3 : add a prefinalizer #Patch Set 4 : only register with Oilpan #Patch Set 5 : leave out unrelated code tidying #
Messages
Total messages: 25 (8 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/ColorChooserPopupUIController.h (right): https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ColorChooserPopupUIController.h:78: RawPtrWillBeMember<ChromeClientImpl> m_chromeClient; It seems that the destructor is touching m_chromeClient in closePopup()...
https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/ColorChooserPopupUIController.h (right): https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/ColorChooserPopupUIController.h:78: RawPtrWillBeMember<ChromeClientImpl> m_chromeClient; On 2015/11/12 14:17:29, haraken wrote: > > It seems that the destructor is touching m_chromeClient in closePopup()... Good catch. Hmm, given that ChromeClientImpl is held in a Persistent<> by WebViewImpl, it is not too likely it will be swept in the same GC iteration. If it is, we're in the middle of a global shutdown, so having closePopup() inform this client that it is going away seems irrelevant. => Introducing a WeakMember<> here looks like the better option overall. Now done.
On 2015/11/13 07:35:01, sof wrote: > https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/web/ColorChooserPopupUIController.h (right): > > https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/web/ColorChooserPopupUIController.h:78: > RawPtrWillBeMember<ChromeClientImpl> m_chromeClient; > On 2015/11/12 14:17:29, haraken wrote: > > > > It seems that the destructor is touching m_chromeClient in closePopup()... > > Good catch. > > Hmm, given that ChromeClientImpl is held in a Persistent<> by WebViewImpl, it is > not too likely it will be swept in the same GC iteration. If it is, we're in the > middle of a global shutdown, so having closePopup() inform this client that it > is going away seems irrelevant. > > => Introducing a WeakMember<> here looks like the better option overall. Now > done. Hmm, it sounds nasty to me to use a WeakMember assuming that the m_chromeClient and the ColorChooserPopupUIController do not die in the same GC cycle. How about introducing a pre-finalizer to call closePopup()?
On 2015/11/13 11:40:07, haraken wrote: > On 2015/11/13 07:35:01, sof wrote: > > > https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... > > File third_party/WebKit/Source/web/ColorChooserPopupUIController.h (right): > > > > > https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... > > third_party/WebKit/Source/web/ColorChooserPopupUIController.h:78: > > RawPtrWillBeMember<ChromeClientImpl> m_chromeClient; > > On 2015/11/12 14:17:29, haraken wrote: > > > > > > It seems that the destructor is touching m_chromeClient in closePopup()... > > > > Good catch. > > > > Hmm, given that ChromeClientImpl is held in a Persistent<> by WebViewImpl, it > is > > not too likely it will be swept in the same GC iteration. If it is, we're in > the > > middle of a global shutdown, so having closePopup() inform this client that it > > is going away seems irrelevant. > > > > => Introducing a WeakMember<> here looks like the better option overall. Now > > done. > > Hmm, it sounds nasty to me to use a WeakMember assuming that the m_chromeClient > and the ColorChooserPopupUIController do not die in the same GC cycle. How about > introducing a pre-finalizer to call closePopup()? What's "nasty" about it? Using eager finalization or a prefinalizer is problematic, as this touches a number of other objects (Page etc), which we thereby cannot also eagerly finalizable now or later.
On 2015/11/13 11:52:57, sof wrote: > On 2015/11/13 11:40:07, haraken wrote: > > On 2015/11/13 07:35:01, sof wrote: > > > > > > https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... > > > File third_party/WebKit/Source/web/ColorChooserPopupUIController.h (right): > > > > > > > > > https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... > > > third_party/WebKit/Source/web/ColorChooserPopupUIController.h:78: > > > RawPtrWillBeMember<ChromeClientImpl> m_chromeClient; > > > On 2015/11/12 14:17:29, haraken wrote: > > > > > > > > It seems that the destructor is touching m_chromeClient in closePopup()... > > > > > > Good catch. > > > > > > Hmm, given that ChromeClientImpl is held in a Persistent<> by WebViewImpl, > it > > is > > > not too likely it will be swept in the same GC iteration. If it is, we're in > > the > > > middle of a global shutdown, so having closePopup() inform this client that > it > > > is going away seems irrelevant. > > > > > > => Introducing a WeakMember<> here looks like the better option overall. Now > > > done. > > > > Hmm, it sounds nasty to me to use a WeakMember assuming that the > m_chromeClient > > and the ColorChooserPopupUIController do not die in the same GC cycle. How > about > > introducing a pre-finalizer to call closePopup()? > > What's "nasty" about it? In general, I think it's nasty to assume that two objects do not die in the same cycle. In practice, I guess it can happen if the following scenario happens: 1) Object A becomes unreachable. 2) GC runs. Lazy sweeping gets scheduled. 3) Object B becomes unreachable. 4) Before the GC collects the object A, the next GC is kicked. Object A is marked as dead. 5) The next GC runs. Object A and object B are collected in the same GC cycle. > > Using eager finalization or a prefinalizer is problematic, as this touches a > number of other objects (Page etc), which we thereby cannot also eagerly > finalizable now or later. Eager-finalization would be problematic, but why is a pre-finalizer problematic? A pre-finalizer should be able to touch any on-heap objects.
On 2015/11/13 12:46:24, haraken wrote: > On 2015/11/13 11:52:57, sof wrote: > > On 2015/11/13 11:40:07, haraken wrote: > > > On 2015/11/13 07:35:01, sof wrote: > > > > > > > > > > https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... > > > > File third_party/WebKit/Source/web/ColorChooserPopupUIController.h > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... > > > > third_party/WebKit/Source/web/ColorChooserPopupUIController.h:78: > > > > RawPtrWillBeMember<ChromeClientImpl> m_chromeClient; > > > > On 2015/11/12 14:17:29, haraken wrote: > > > > > > > > > > It seems that the destructor is touching m_chromeClient in > closePopup()... > > > > > > > > Good catch. > > > > > > > > Hmm, given that ChromeClientImpl is held in a Persistent<> by WebViewImpl, > > it > > > is > > > > not too likely it will be swept in the same GC iteration. If it is, we're > in > > > the > > > > middle of a global shutdown, so having closePopup() inform this client > that > > it > > > > is going away seems irrelevant. > > > > > > > > => Introducing a WeakMember<> here looks like the better option overall. > Now > > > > done. > > > > > > Hmm, it sounds nasty to me to use a WeakMember assuming that the > > m_chromeClient > > > and the ColorChooserPopupUIController do not die in the same GC cycle. How > > about > > > introducing a pre-finalizer to call closePopup()? > > > > What's "nasty" about it? > > In general, I think it's nasty to assume that two objects do not die in the same > cycle. > > In practice, I guess it can happen if the following scenario happens: > > 1) Object A becomes unreachable. > 2) GC runs. Lazy sweeping gets scheduled. > 3) Object B becomes unreachable. > 4) Before the GC collects the object A, the next GC is kicked. Object A is > marked as dead. > 5) The next GC runs. Object A and object B are collected in the same GC cycle. > > > > > Using eager finalization or a prefinalizer is problematic, as this touches a > > number of other objects (Page etc), which we thereby cannot also eagerly > > finalizable now or later. > > Eager-finalization would be problematic, but why is a pre-finalizer problematic? > A pre-finalizer should be able to touch any on-heap objects. And in what state would an object be post-prefinalizer having run, does it have to support such use also?
On 2015/11/13 12:48:37, sof wrote: > On 2015/11/13 12:46:24, haraken wrote: > > On 2015/11/13 11:52:57, sof wrote: > > > On 2015/11/13 11:40:07, haraken wrote: > > > > On 2015/11/13 07:35:01, sof wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... > > > > > File third_party/WebKit/Source/web/ColorChooserPopupUIController.h > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... > > > > > third_party/WebKit/Source/web/ColorChooserPopupUIController.h:78: > > > > > RawPtrWillBeMember<ChromeClientImpl> m_chromeClient; > > > > > On 2015/11/12 14:17:29, haraken wrote: > > > > > > > > > > > > It seems that the destructor is touching m_chromeClient in > > closePopup()... > > > > > > > > > > Good catch. > > > > > > > > > > Hmm, given that ChromeClientImpl is held in a Persistent<> by > WebViewImpl, > > > it > > > > is > > > > > not too likely it will be swept in the same GC iteration. If it is, > we're > > in > > > > the > > > > > middle of a global shutdown, so having closePopup() inform this client > > that > > > it > > > > > is going away seems irrelevant. > > > > > > > > > > => Introducing a WeakMember<> here looks like the better option overall. > > Now > > > > > done. > > > > > > > > Hmm, it sounds nasty to me to use a WeakMember assuming that the > > > m_chromeClient > > > > and the ColorChooserPopupUIController do not die in the same GC cycle. How > > > about > > > > introducing a pre-finalizer to call closePopup()? > > > > > > What's "nasty" about it? > > > > In general, I think it's nasty to assume that two objects do not die in the > same > > cycle. > > > > In practice, I guess it can happen if the following scenario happens: > > > > 1) Object A becomes unreachable. > > 2) GC runs. Lazy sweeping gets scheduled. > > 3) Object B becomes unreachable. > > 4) Before the GC collects the object A, the next GC is kicked. Object A is > > marked as dead. > > 5) The next GC runs. Object A and object B are collected in the same GC cycle. > > > > > > > > Using eager finalization or a prefinalizer is problematic, as this touches a > > > number of other objects (Page etc), which we thereby cannot also eagerly > > > finalizable now or later. > > > > Eager-finalization would be problematic, but why is a pre-finalizer > problematic? > > A pre-finalizer should be able to touch any on-heap objects. > > And in what state would an object be post-prefinalizer having run, does it have > to support such use also? Maybe I'm missing your point... Would you help me understand what's a problem of calling closePopup() in ColorChooserPopupUIController's pre-finalizer?
On 2015/11/13 12:51:30, haraken wrote: > On 2015/11/13 12:48:37, sof wrote: > > On 2015/11/13 12:46:24, haraken wrote: > > > On 2015/11/13 11:52:57, sof wrote: > > > > On 2015/11/13 11:40:07, haraken wrote: > > > > > On 2015/11/13 07:35:01, sof wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... > > > > > > File third_party/WebKit/Source/web/ColorChooserPopupUIController.h > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1442543003/diff/1/third_party/WebKit/Source/w... > > > > > > third_party/WebKit/Source/web/ColorChooserPopupUIController.h:78: > > > > > > RawPtrWillBeMember<ChromeClientImpl> m_chromeClient; > > > > > > On 2015/11/12 14:17:29, haraken wrote: > > > > > > > > > > > > > > It seems that the destructor is touching m_chromeClient in > > > closePopup()... > > > > > > > > > > > > Good catch. > > > > > > > > > > > > Hmm, given that ChromeClientImpl is held in a Persistent<> by > > WebViewImpl, > > > > it > > > > > is > > > > > > not too likely it will be swept in the same GC iteration. If it is, > > we're > > > in > > > > > the > > > > > > middle of a global shutdown, so having closePopup() inform this client > > > that > > > > it > > > > > > is going away seems irrelevant. > > > > > > > > > > > > => Introducing a WeakMember<> here looks like the better option > overall. > > > Now > > > > > > done. > > > > > > > > > > Hmm, it sounds nasty to me to use a WeakMember assuming that the > > > > m_chromeClient > > > > > and the ColorChooserPopupUIController do not die in the same GC cycle. > How > > > > about > > > > > introducing a pre-finalizer to call closePopup()? > > > > > > > > What's "nasty" about it? > > > > > > In general, I think it's nasty to assume that two objects do not die in the > > same > > > cycle. > > > > > > In practice, I guess it can happen if the following scenario happens: > > > > > > 1) Object A becomes unreachable. > > > 2) GC runs. Lazy sweeping gets scheduled. > > > 3) Object B becomes unreachable. > > > 4) Before the GC collects the object A, the next GC is kicked. Object A is > > > marked as dead. > > > 5) The next GC runs. Object A and object B are collected in the same GC > cycle. > > > > > > > > > > > Using eager finalization or a prefinalizer is problematic, as this touches > a > > > > number of other objects (Page etc), which we thereby cannot also eagerly > > > > finalizable now or later. > > > > > > Eager-finalization would be problematic, but why is a pre-finalizer > > problematic? > > > A pre-finalizer should be able to touch any on-heap objects. > > > > And in what state would an object be post-prefinalizer having run, does it > have > > to support such use also? > > Maybe I'm missing your point... Would you help me understand what's a problem of > calling closePopup() in ColorChooserPopupUIController's pre-finalizer? I don't want to have it determine that adding a prefinalizer or make Page eagerly finalized will be problematic in the future, say. I think WeakMember<> is more appropriate here, overall.
> > > > > > Hmm, it sounds nasty to me to use a WeakMember assuming that the > > > > > m_chromeClient > > > > > > and the ColorChooserPopupUIController do not die in the same GC cycle. > > How > > > > > about > > > > > > introducing a pre-finalizer to call closePopup()? > > > > > > > > > > What's "nasty" about it? > > > > > > > > In general, I think it's nasty to assume that two objects do not die in > the > > > same > > > > cycle. > > > > > > > > In practice, I guess it can happen if the following scenario happens: > > > > > > > > 1) Object A becomes unreachable. > > > > 2) GC runs. Lazy sweeping gets scheduled. > > > > 3) Object B becomes unreachable. > > > > 4) Before the GC collects the object A, the next GC is kicked. Object A is > > > > marked as dead. > > > > 5) The next GC runs. Object A and object B are collected in the same GC > > cycle. > > > > > > > > > > > > > > Using eager finalization or a prefinalizer is problematic, as this > touches > > a > > > > > number of other objects (Page etc), which we thereby cannot also eagerly > > > > > finalizable now or later. > > > > > > > > Eager-finalization would be problematic, but why is a pre-finalizer > > > problematic? > > > > A pre-finalizer should be able to touch any on-heap objects. > > > > > > And in what state would an object be post-prefinalizer having run, does it > > have > > > to support such use also? > > > > Maybe I'm missing your point... Would you help me understand what's a problem > of > > calling closePopup() in ColorChooserPopupUIController's pre-finalizer? > > I don't want to have it determine that adding a prefinalizer or make Page > eagerly finalized will be problematic in the future, say. Eager finalization would have a risk of causing problems in the future (because it puts some restrictions on destruction ordering), but a pre-finalizer wouldn't have the risk, would it? > I think WeakMember<> is more appropriate here, overall. For the general/practical reasons I mentioned above, I don't think it's a good idea to assume that some two objects don't die in the same cycle (in other words, looking up the value of a WeakMember in a destructor is wrong). If you don't want to add a pre-finalizer to ColorChooserPopupUIController, how about adding a pre-finalizer to ColorInputType and call m_chooser->closePopup() in the pre-finalizer?
added a prefinalizer.
LGTM
Description was changed from ========== Oilpan: trace ColorChooserPopupUIController::m_chromeClient R= BUG=553613 ========== to ========== Oilpan: trace ColorChooserPopupUIController::m_chromeClient R=haraken BUG=553613 ==========
The CQ bit was checked by sigbjornf@opera.com
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/1442543003/#ps60001 (title: "only register with Oilpan")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442543003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442543003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sigbjornf@opera.com
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/1442543003/#ps80001 (title: "leave out unrelated code tidying")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442543003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442543003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c29f20c8bd73d5c6e3c0bd45e11c1bcf738ceb37 Cr-Commit-Position: refs/heads/master@{#361043} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
