Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2004263003:
Delay resetting image animation, if possible. (Closed)
|
Created:
4 years, 7 months ago by sof Modified:
4 years, 6 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, Yoav Weiss, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, Nate Chapin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDelay resetting image animation, if possible.
When the last client of an ImageResource removes itself, the animations
of the image is explicitly reset. That resetting can happen either while
finalizing objects after a GC or as part of other explicit removals of
ImageObserver clients.
Having that reset happen as part of a garbage collection is interacting badly
with code in the middle of updating animations (which happen to trigger a
conservative GC.) So, to avoid introducing such abrupt & harmful resets, delay
the reset'ing until back at the event loop (and the animations update step
having completed.)
R=
BUG=613709, 581546
Committed: https://crrev.com/a9a17309a49caff18a2cffd43867d99aa20cf42e
Cr-Commit-Position: refs/heads/master@{#400934}
Patch Set 1 #Patch Set 2 : expand the comment #
Total comments: 2
Patch Set 3 : firm up willObjectBeLazilySwept() #Patch Set 4 : comment typo #Patch Set 5 : rebased #Patch Set 6 : rebased #
Messages
Total messages: 43 (8 generated)
Description was changed from ========== Delay resetting image animation, if possible. R= BUG= ========== to ========== Delay resetting image animation, if possible. When the last client of an ImageResource removes itself, the animations of the image is explicitly reset. That resetting can happen either while finalizing objects after a GC or as part of other explicit removals of ImageObserver clients. Having that reset happen as part of a garbage collection is interacting badly with code in the middle of updating animations (which happen to trigger a GC), so to avoid introducing such abrupt resets, delay the operation until back at the event loop (and the animations update step having completed.) R= BUG=613709, 581546 ==========
sigbjornf@opera.com changed reviewers: + davve@opera.com, fs@opera.com, oilpan-reviews@chromium.org
please take a look. See https://codereview.chromium.org/2005693002/ for an alternate approach considered.
On 2016/05/24 at 14:07:31, sigbjornf wrote: > please take a look. > > See https://codereview.chromium.org/2005693002/ for an alternate approach considered. Seems reasonable to me (albeit very non-obvious if you don't know what it's fixing.) LGTM
On 2016/05/24 14:16:00, fs wrote: > On 2016/05/24 at 14:07:31, sigbjornf wrote: > > please take a look. > > > > See https://codereview.chromium.org/2005693002/ for an alternate approach > considered. > > Seems reasonable to me (albeit very non-obvious if you don't know what it's > fixing.) > > LGTM The comment didn't offer very many clues either; expanded it some.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResource.cpp:221: if (!ThreadHeap::willObjectBeLazilySwept(this)) Hmm, I'm not really happy about using ThreadHeap::willObjectBeLazilySwept since it can return a wrong result (as we discussed previously). Would it be possible to introduce a flag to ImageResource and set it in a pre-finalizer? Then we can check the flag here to see if the ImageResource is going to be collected or not.
https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResource.cpp:221: if (!ThreadHeap::willObjectBeLazilySwept(this)) On 2016/05/24 15:40:54, haraken wrote: > > Hmm, I'm not really happy about using ThreadHeap::willObjectBeLazilySwept since > it can return a wrong result (as we discussed previously). > > Would it be possible to introduce a flag to ImageResource and set it in a > pre-finalizer? Then we can check the flag here to see if the ImageResource is > going to be collected or not. As this method will be called by another object's prefinalizer, I don't think that scheme will work. As long as willBeLazilySwept() returns true while that other object's prefinalizer is running, that's sufficient. We could add that as a flag to ThreadState (== is running prefinalizers), I suppose.
On 2016/05/24 15:47:20, sof wrote: > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/ImageResource.cpp:221: if > (!ThreadHeap::willObjectBeLazilySwept(this)) > On 2016/05/24 15:40:54, haraken wrote: > > > > Hmm, I'm not really happy about using ThreadHeap::willObjectBeLazilySwept > since > > it can return a wrong result (as we discussed previously). > > > > Would it be possible to introduce a flag to ImageResource and set it in a > > pre-finalizer? Then we can check the flag here to see if the ImageResource is > > going to be collected or not. > > As this method will be called by another object's prefinalizer, I don't think > that scheme will work. What's the another object? Would it be an option to set a flag on ImageResource at the very beginning of the another object's pre-finalizer?
On 2016/05/24 15:53:57, haraken wrote: > On 2016/05/24 15:47:20, sof wrote: > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): > > > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/fetch/ImageResource.cpp:221: if > > (!ThreadHeap::willObjectBeLazilySwept(this)) > > On 2016/05/24 15:40:54, haraken wrote: > > > > > > Hmm, I'm not really happy about using ThreadHeap::willObjectBeLazilySwept > > since > > > it can return a wrong result (as we discussed previously). > > > > > > Would it be possible to introduce a flag to ImageResource and set it in a > > > pre-finalizer? Then we can check the flag here to see if the ImageResource > is > > > going to be collected or not. > > > > As this method will be called by another object's prefinalizer, I don't think > > that scheme will work. > > What's the another object? Would it be an option to set a flag on ImageResource > at the very beginning of the another object's pre-finalizer? It would be a hack, I think. CSSFetchImage and whoever else is a client of this ImageResource. What's being done here is safe, i.e., ensuring that we don't revive a dying object during finalization. More generally, it would be good if you could write out a clear argument why&how willObjectBeLazilySwept() is unsafe in our implementation -- I don't think I've seen that. We rely on it quite a bit, timers wouldn't be safe without it. And I believe they are :)
On 2016/05/24 18:00:20, sof wrote: > On 2016/05/24 15:53:57, haraken wrote: > > On 2016/05/24 15:47:20, sof wrote: > > > > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): > > > > > > > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/fetch/ImageResource.cpp:221: if > > > (!ThreadHeap::willObjectBeLazilySwept(this)) > > > On 2016/05/24 15:40:54, haraken wrote: > > > > > > > > Hmm, I'm not really happy about using ThreadHeap::willObjectBeLazilySwept > > > since > > > > it can return a wrong result (as we discussed previously). > > > > > > > > Would it be possible to introduce a flag to ImageResource and set it in a > > > > pre-finalizer? Then we can check the flag here to see if the ImageResource > > is > > > > going to be collected or not. > > > > > > As this method will be called by another object's prefinalizer, I don't > think > > > that scheme will work. > > > > What's the another object? Would it be an option to set a flag on > ImageResource > > at the very beginning of the another object's pre-finalizer? > > It would be a hack, I think. CSSFetchImage and whoever else is a client of this > ImageResource. > > What's being done here is safe, i.e., ensuring that we don't revive a dying > object during finalization. > > More generally, it would be good if you could write out a clear argument why&how > willObjectBeLazilySwept() is unsafe in our implementation -- I don't think I've > seen that. We rely on it quite a bit, timers wouldn't be safe without it. And I > believe they are :) s/CSSFetchImage/StyleFetchedImage/
On 2016/05/24 18:00:20, sof wrote: > On 2016/05/24 15:53:57, haraken wrote: > > On 2016/05/24 15:47:20, sof wrote: > > > > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): > > > > > > > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/fetch/ImageResource.cpp:221: if > > > (!ThreadHeap::willObjectBeLazilySwept(this)) > > > On 2016/05/24 15:40:54, haraken wrote: > > > > > > > > Hmm, I'm not really happy about using ThreadHeap::willObjectBeLazilySwept > > > since > > > > it can return a wrong result (as we discussed previously). > > > > > > > > Would it be possible to introduce a flag to ImageResource and set it in a > > > > pre-finalizer? Then we can check the flag here to see if the ImageResource > > is > > > > going to be collected or not. > > > > > > As this method will be called by another object's prefinalizer, I don't > think > > > that scheme will work. > > > > What's the another object? Would it be an option to set a flag on > ImageResource > > at the very beginning of the another object's pre-finalizer? > > It would be a hack, I think. CSSFetchImage and whoever else is a client of this > ImageResource. > > What's being done here is safe, i.e., ensuring that we don't revive a dying > object during finalization. > > More generally, it would be good if you could write out a clear argument why&how > willObjectBeLazilySwept() is unsafe in our implementation -- I don't think I've > seen that. We rely on it quite a bit, timers wouldn't be safe without it. And I > believe they are :) See https://codereview.chromium.org/1151163002/#msg13. willObjectBeLazilySwept() is safe only if it's guaranteed that it's not called when we have a page s.t. a part of the page has already been swept but the remaining part of the page has not yet been swept. At the end of an event loop meets the condition, so willObjectBeLazilySwept() in timer is safe. But due to the subtlety, I want to avoid using willObjectBeLazilySwept() as much as possible.
On 2016/05/24 19:57:47, haraken wrote: > On 2016/05/24 18:00:20, sof wrote: > > On 2016/05/24 15:53:57, haraken wrote: > > > On 2016/05/24 15:47:20, sof wrote: > > > > > > > > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/fetch/ImageResource.cpp:221: if > > > > (!ThreadHeap::willObjectBeLazilySwept(this)) > > > > On 2016/05/24 15:40:54, haraken wrote: > > > > > > > > > > Hmm, I'm not really happy about using > ThreadHeap::willObjectBeLazilySwept > > > > since > > > > > it can return a wrong result (as we discussed previously). > > > > > > > > > > Would it be possible to introduce a flag to ImageResource and set it in > a > > > > > pre-finalizer? Then we can check the flag here to see if the > ImageResource > > > is > > > > > going to be collected or not. > > > > > > > > As this method will be called by another object's prefinalizer, I don't > > think > > > > that scheme will work. > > > > > > What's the another object? Would it be an option to set a flag on > > ImageResource > > > at the very beginning of the another object's pre-finalizer? > > > > It would be a hack, I think. CSSFetchImage and whoever else is a client of > this > > ImageResource. > > > > What's being done here is safe, i.e., ensuring that we don't revive a dying > > object during finalization. > > > > More generally, it would be good if you could write out a clear argument > why&how > > willObjectBeLazilySwept() is unsafe in our implementation -- I don't think > I've > > seen that. We rely on it quite a bit, timers wouldn't be safe without it. And > I > > believe they are :) > > See https://codereview.chromium.org/1151163002/#msg13. > > willObjectBeLazilySwept() is safe only if it's guaranteed that it's not called > when we have a page s.t. a part of the page has already been swept but the > remaining part of the page has not yet been swept. At the end of an event loop > meets the condition, so willObjectBeLazilySwept() in timer is safe. But due to > the subtlety, I want to avoid using willObjectBeLazilySwept() as much as > possible. Thanks for the refresher; call to willObjectBeLazilySwept() in code executed by non-pre/normal finalizers is what to look out for. Its imprecision wouldn't matter here, but I accept that you want to avoid its use. What we're really after here is a way to check if this is executed as part of the (pre)finalization steps that a conservative GC will perform before returning from the allocate() call that triggered the conservative GC..but we also need to determine liveness of the ImageResource. Let me think it over if there's a way to sidestep willObjectBeLazilySwept() (but not introduce more prefinalization code, preferably.)
On 2016/05/24 20:57:59, sof wrote: > On 2016/05/24 19:57:47, haraken wrote: > > On 2016/05/24 18:00:20, sof wrote: > > > On 2016/05/24 15:53:57, haraken wrote: > > > > On 2016/05/24 15:47:20, sof wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/core/fetch/ImageResource.cpp:221: if > > > > > (!ThreadHeap::willObjectBeLazilySwept(this)) > > > > > On 2016/05/24 15:40:54, haraken wrote: > > > > > > > > > > > > Hmm, I'm not really happy about using > > ThreadHeap::willObjectBeLazilySwept > > > > > since > > > > > > it can return a wrong result (as we discussed previously). > > > > > > > > > > > > Would it be possible to introduce a flag to ImageResource and set it > in > > a > > > > > > pre-finalizer? Then we can check the flag here to see if the > > ImageResource > > > > is > > > > > > going to be collected or not. > > > > > > > > > > As this method will be called by another object's prefinalizer, I don't > > > think > > > > > that scheme will work. > > > > > > > > What's the another object? Would it be an option to set a flag on > > > ImageResource > > > > at the very beginning of the another object's pre-finalizer? > > > > > > It would be a hack, I think. CSSFetchImage and whoever else is a client of > > this > > > ImageResource. > > > > > > What's being done here is safe, i.e., ensuring that we don't revive a dying > > > object during finalization. > > > > > > More generally, it would be good if you could write out a clear argument > > why&how > > > willObjectBeLazilySwept() is unsafe in our implementation -- I don't think > > I've > > > seen that. We rely on it quite a bit, timers wouldn't be safe without it. > And > > I > > > believe they are :) > > > > See https://codereview.chromium.org/1151163002/#msg13. > > > > willObjectBeLazilySwept() is safe only if it's guaranteed that it's not called > > when we have a page s.t. a part of the page has already been swept but the > > remaining part of the page has not yet been swept. At the end of an event loop > > meets the condition, so willObjectBeLazilySwept() in timer is safe. But due to > > the subtlety, I want to avoid using willObjectBeLazilySwept() as much as > > possible. > > Thanks for the refresher; call to willObjectBeLazilySwept() in code executed by > non-pre/normal finalizers is what to look out for. Its imprecision wouldn't > matter here, but I accept that you want to avoid its use. > > What we're really after here is a way to check if this is executed as part of > the (pre)finalization steps that a conservative GC will perform before returning > from the allocate() call that triggered the conservative GC..but we also need to > determine liveness of the ImageResource. Let me think it over if there's a way > to sidestep willObjectBeLazilySwept() (but not introduce more prefinalization > code, preferably.) Thinking that over a bit made me realize that willObjectBeLazilySwept(this) will always return a correct result.
On 2016/05/24 21:34:42, sof wrote: > On 2016/05/24 20:57:59, sof wrote: > > On 2016/05/24 19:57:47, haraken wrote: > > > On 2016/05/24 18:00:20, sof wrote: > > > > On 2016/05/24 15:53:57, haraken wrote: > > > > > On 2016/05/24 15:47:20, sof wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > > > > > File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/core/fetch/ImageResource.cpp:221: if > > > > > > (!ThreadHeap::willObjectBeLazilySwept(this)) > > > > > > On 2016/05/24 15:40:54, haraken wrote: > > > > > > > > > > > > > > Hmm, I'm not really happy about using > > > ThreadHeap::willObjectBeLazilySwept > > > > > > since > > > > > > > it can return a wrong result (as we discussed previously). > > > > > > > > > > > > > > Would it be possible to introduce a flag to ImageResource and set it > > in > > > a > > > > > > > pre-finalizer? Then we can check the flag here to see if the > > > ImageResource > > > > > is > > > > > > > going to be collected or not. > > > > > > > > > > > > As this method will be called by another object's prefinalizer, I > don't > > > > think > > > > > > that scheme will work. > > > > > > > > > > What's the another object? Would it be an option to set a flag on > > > > ImageResource > > > > > at the very beginning of the another object's pre-finalizer? > > > > > > > > It would be a hack, I think. CSSFetchImage and whoever else is a client of > > > this > > > > ImageResource. > > > > > > > > What's being done here is safe, i.e., ensuring that we don't revive a > dying > > > > object during finalization. > > > > > > > > More generally, it would be good if you could write out a clear argument > > > why&how > > > > willObjectBeLazilySwept() is unsafe in our implementation -- I don't think > > > I've > > > > seen that. We rely on it quite a bit, timers wouldn't be safe without it. > > And > > > I > > > > believe they are :) > > > > > > See https://codereview.chromium.org/1151163002/#msg13. > > > > > > willObjectBeLazilySwept() is safe only if it's guaranteed that it's not > called > > > when we have a page s.t. a part of the page has already been swept but the > > > remaining part of the page has not yet been swept. At the end of an event > loop > > > meets the condition, so willObjectBeLazilySwept() in timer is safe. But due > to > > > the subtlety, I want to avoid using willObjectBeLazilySwept() as much as > > > possible. > > > > Thanks for the refresher; call to willObjectBeLazilySwept() in code executed > by > > non-pre/normal finalizers is what to look out for. Its imprecision wouldn't > > matter here, but I accept that you want to avoid its use. > > > > What we're really after here is a way to check if this is executed as part of > > the (pre)finalization steps that a conservative GC will perform before > returning > > from the allocate() call that triggered the conservative GC..but we also need > to > > determine liveness of the ImageResource. Let me think it over if there's a way > > to sidestep willObjectBeLazilySwept() (but not introduce more prefinalization > > code, preferably.) > > Thinking that over a bit made me realize that willObjectBeLazilySwept(this) will > always return a correct result. What happens if ImageResource::allClientsAndObserversRemoved is called by code other than the pre-finalizer? Does willObjectBeLazilySwept(this) return a correct result even in the case? But either way, I think willObjectBeLazilySwept(this) is too complicated to reason about. If we can set a flag at the beginning of the pre-finalizer and prevent the animation update, that seems safer to me.
On 2016/05/24 22:37:50, haraken wrote: > On 2016/05/24 21:34:42, sof wrote: > > On 2016/05/24 20:57:59, sof wrote: > > > On 2016/05/24 19:57:47, haraken wrote: > > > > On 2016/05/24 18:00:20, sof wrote: > > > > > On 2016/05/24 15:53:57, haraken wrote: > > > > > > On 2016/05/24 15:47:20, sof wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > > > > > > File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Sour... > > > > > > > third_party/WebKit/Source/core/fetch/ImageResource.cpp:221: if > > > > > > > (!ThreadHeap::willObjectBeLazilySwept(this)) > > > > > > > On 2016/05/24 15:40:54, haraken wrote: > > > > > > > > > > > > > > > > Hmm, I'm not really happy about using > > > > ThreadHeap::willObjectBeLazilySwept > > > > > > > since > > > > > > > > it can return a wrong result (as we discussed previously). > > > > > > > > > > > > > > > > Would it be possible to introduce a flag to ImageResource and set > it > > > in > > > > a > > > > > > > > pre-finalizer? Then we can check the flag here to see if the > > > > ImageResource > > > > > > is > > > > > > > > going to be collected or not. > > > > > > > > > > > > > > As this method will be called by another object's prefinalizer, I > > don't > > > > > think > > > > > > > that scheme will work. > > > > > > > > > > > > What's the another object? Would it be an option to set a flag on > > > > > ImageResource > > > > > > at the very beginning of the another object's pre-finalizer? > > > > > > > > > > It would be a hack, I think. CSSFetchImage and whoever else is a client > of > > > > this > > > > > ImageResource. > > > > > > > > > > What's being done here is safe, i.e., ensuring that we don't revive a > > dying > > > > > object during finalization. > > > > > > > > > > More generally, it would be good if you could write out a clear argument > > > > why&how > > > > > willObjectBeLazilySwept() is unsafe in our implementation -- I don't > think > > > > I've > > > > > seen that. We rely on it quite a bit, timers wouldn't be safe without > it. > > > And > > > > I > > > > > believe they are :) > > > > > > > > See https://codereview.chromium.org/1151163002/#msg13. > > > > > > > > willObjectBeLazilySwept() is safe only if it's guaranteed that it's not > > called > > > > when we have a page s.t. a part of the page has already been swept but the > > > > remaining part of the page has not yet been swept. At the end of an event > > loop > > > > meets the condition, so willObjectBeLazilySwept() in timer is safe. But > due > > to > > > > the subtlety, I want to avoid using willObjectBeLazilySwept() as much as > > > > possible. > > > > > > Thanks for the refresher; call to willObjectBeLazilySwept() in code executed > > by > > > non-pre/normal finalizers is what to look out for. Its imprecision wouldn't > > > matter here, but I accept that you want to avoid its use. > > > > > > What we're really after here is a way to check if this is executed as part > of > > > the (pre)finalization steps that a conservative GC will perform before > > returning > > > from the allocate() call that triggered the conservative GC..but we also > need > > to > > > determine liveness of the ImageResource. Let me think it over if there's a > way > > > to sidestep willObjectBeLazilySwept() (but not introduce more > prefinalization > > > code, preferably.) > > > > Thinking that over a bit made me realize that willObjectBeLazilySwept(this) > will > > always return a correct result. > > What happens if ImageResource::allClientsAndObserversRemoved is called by code > other than the pre-finalizer? Does willObjectBeLazilySwept(this) return a > correct result even in the case? > > But either way, I think willObjectBeLazilySwept(this) is too complicated to > reason about. If we can set a flag at the beginning of the pre-finalizer and > prevent the animation update, that seems safer to me. We need to fix willObjectBeLazilySwept() to also handle this rare lazy sweeping case; ps#3 addresses.
> We need to fix willObjectBeLazilySwept() to also handle this rare lazy sweeping > case; ps#3 addresses. Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd prefer decreasing the call site rather than making it more correct... Stepping back, let me ask a couple of questions: - Do we really need to call resetAnimation when all the clients and the ImageResource die at the same time? - You don't delay calling resetAnimation when all the clients are removed but the ImageResource is still live. This means that style recalc can run in CSSStyleImage's pre-finalizer, but isn't it dangerous? A pre-finalizer is not allowed to change object graphs (accurately speaking, it is not allowed to resurrect any reference), so it looks unsafe to run a complicated thing like style recalc during a pre-finalizer.
On 2016/05/26 04:20:27, haraken wrote: > > We need to fix willObjectBeLazilySwept() to also handle this rare lazy > sweeping > > case; ps#3 addresses. > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd prefer > decreasing the call site rather than making it more correct... > Don't agree, this is a good solution that I want to go with. > Stepping back, let me ask a couple of questions: > > - Do we really need to call resetAnimation when all the clients and the > ImageResource die at the same time? > That's what we've been doing all along & currently. > - You don't delay calling resetAnimation when all the clients are removed but > the ImageResource is still live. This means that style recalc can run in > CSSStyleImage's pre-finalizer, but isn't it dangerous? A pre-finalizer is not > allowed to change object graphs (accurately speaking, it is not allowed to > resurrect any reference), so it looks unsafe to run a complicated thing like > style recalc during a pre-finalizer. It's the other way around, when it is dead we cannot delay the call. Yes, we all would want there not to be that amount of code executed when finalizing, but that's an old topic.
On 2016/05/26 05:19:23, sof wrote: > On 2016/05/26 04:20:27, haraken wrote: > > > We need to fix willObjectBeLazilySwept() to also handle this rare lazy > > sweeping > > > case; ps#3 addresses. > > > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd prefer > > decreasing the call site rather than making it more correct... > > > > Don't agree, this is a good solution that I want to go with. > > > Stepping back, let me ask a couple of questions: > > > > - Do we really need to call resetAnimation when all the clients and the > > ImageResource die at the same time? > > > > That's what we've been doing all along & currently. > > > - You don't delay calling resetAnimation when all the clients are removed but > > the ImageResource is still live. This means that style recalc can run in > > CSSStyleImage's pre-finalizer, but isn't it dangerous? A pre-finalizer is not > > allowed to change object graphs (accurately speaking, it is not allowed to > > resurrect any reference), so it looks unsafe to run a complicated thing like > > style recalc during a pre-finalizer. > > It's the other way around, when it is dead we cannot delay the call. Yes, we all > would want there not to be that amount of code executed when finalizing, but > that's an old topic. I don't see any reason we need to run resetAnimations when both the clients and the ImageResource at the same time. If we just need to run resetAnimations only when the clients are dead but the ImageResource is live, can we just use weak processing to post a task for resetAnimations? ImageResource can post a task when the weakly observing clients are gone.
On 2016/05/26 08:16:18, haraken wrote: > On 2016/05/26 05:19:23, sof wrote: > > On 2016/05/26 04:20:27, haraken wrote: > > > > We need to fix willObjectBeLazilySwept() to also handle this rare lazy > > > sweeping > > > > case; ps#3 addresses. > > > > > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd prefer > > > decreasing the call site rather than making it more correct... > > > > > > > Don't agree, this is a good solution that I want to go with. > > > > > Stepping back, let me ask a couple of questions: > > > > > > - Do we really need to call resetAnimation when all the clients and the > > > ImageResource die at the same time? > > > > > > > That's what we've been doing all along & currently. > > > > > - You don't delay calling resetAnimation when all the clients are removed > but > > > the ImageResource is still live. This means that style recalc can run in > > > CSSStyleImage's pre-finalizer, but isn't it dangerous? A pre-finalizer is > not > > > allowed to change object graphs (accurately speaking, it is not allowed to > > > resurrect any reference), so it looks unsafe to run a complicated thing like > > > style recalc during a pre-finalizer. > > > > It's the other way around, when it is dead we cannot delay the call. Yes, we > all > > would want there not to be that amount of code executed when finalizing, but > > that's an old topic. > > I don't see any reason we need to run resetAnimations when both the clients and > the ImageResource at the same time. > > If we just need to run resetAnimations only when the clients are dead but the > ImageResource is live, can we just use weak processing to post a task for > resetAnimations? ImageResource can post a task when the weakly observing clients > are gone. resetAnimation() does a lot of work, how would you know that it isn't needed? This is what ToT does and have been doing for a long time. iow, I'm looking to address an instability here that has a beta-block label on it, first & foremost.
On 2016/05/26 08:20:57, sof wrote: > On 2016/05/26 08:16:18, haraken wrote: > > On 2016/05/26 05:19:23, sof wrote: > > > On 2016/05/26 04:20:27, haraken wrote: > > > > > We need to fix willObjectBeLazilySwept() to also handle this rare lazy > > > > sweeping > > > > > case; ps#3 addresses. > > > > > > > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd prefer > > > > decreasing the call site rather than making it more correct... > > > > > > > > > > Don't agree, this is a good solution that I want to go with. > > > > > > > Stepping back, let me ask a couple of questions: > > > > > > > > - Do we really need to call resetAnimation when all the clients and the > > > > ImageResource die at the same time? > > > > > > > > > > That's what we've been doing all along & currently. > > > > > > > - You don't delay calling resetAnimation when all the clients are removed > > but > > > > the ImageResource is still live. This means that style recalc can run in > > > > CSSStyleImage's pre-finalizer, but isn't it dangerous? A pre-finalizer is > > not > > > > allowed to change object graphs (accurately speaking, it is not allowed to > > > > resurrect any reference), so it looks unsafe to run a complicated thing > like > > > > style recalc during a pre-finalizer. > > > > > > It's the other way around, when it is dead we cannot delay the call. Yes, we > > all > > > would want there not to be that amount of code executed when finalizing, but > > > that's an old topic. > > > > I don't see any reason we need to run resetAnimations when both the clients > and > > the ImageResource at the same time. > > > > If we just need to run resetAnimations only when the clients are dead but the > > ImageResource is live, can we just use weak processing to post a task for > > resetAnimations? ImageResource can post a task when the weakly observing > clients > > are gone. > > resetAnimation() does a lot of work, how would you know that it isn't needed? > This is what ToT does and have been doing for a long time. > > iow, I'm looking to address an instability here that has a beta-block label on > it, first & foremost. I'm not convinced that this is a right fix... The problem is that we're calling resetAnimations during a pre-finalizer. This CL still runs resetAnimations during a pre-finalizer when the clients and the ImageResource die at the same time. This CL may reduce the crash rate, but the issue is still there, isn't it?
On 2016/05/26 08:43:33, haraken wrote: > On 2016/05/26 08:20:57, sof wrote: > > On 2016/05/26 08:16:18, haraken wrote: > > > On 2016/05/26 05:19:23, sof wrote: > > > > On 2016/05/26 04:20:27, haraken wrote: > > > > > > We need to fix willObjectBeLazilySwept() to also handle this rare lazy > > > > > sweeping > > > > > > case; ps#3 addresses. > > > > > > > > > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd prefer > > > > > decreasing the call site rather than making it more correct... > > > > > > > > > > > > > Don't agree, this is a good solution that I want to go with. > > > > > > > > > Stepping back, let me ask a couple of questions: > > > > > > > > > > - Do we really need to call resetAnimation when all the clients and the > > > > > ImageResource die at the same time? > > > > > > > > > > > > > That's what we've been doing all along & currently. > > > > > > > > > - You don't delay calling resetAnimation when all the clients are > removed > > > but > > > > > the ImageResource is still live. This means that style recalc can run in > > > > > CSSStyleImage's pre-finalizer, but isn't it dangerous? A pre-finalizer > is > > > not > > > > > allowed to change object graphs (accurately speaking, it is not allowed > to > > > > > resurrect any reference), so it looks unsafe to run a complicated thing > > like > > > > > style recalc during a pre-finalizer. > > > > > > > > It's the other way around, when it is dead we cannot delay the call. Yes, > we > > > all > > > > would want there not to be that amount of code executed when finalizing, > but > > > > that's an old topic. > > > > > > I don't see any reason we need to run resetAnimations when both the clients > > and > > > the ImageResource at the same time. > > > > > > If we just need to run resetAnimations only when the clients are dead but > the > > > ImageResource is live, can we just use weak processing to post a task for > > > resetAnimations? ImageResource can post a task when the weakly observing > > clients > > > are gone. > > > > resetAnimation() does a lot of work, how would you know that it isn't needed? > > This is what ToT does and have been doing for a long time. > > > > iow, I'm looking to address an instability here that has a beta-block label on > > it, first & foremost. > > I'm not convinced that this is a right fix... > > The problem is that we're calling resetAnimations during a pre-finalizer. This > CL still runs resetAnimations during a pre-finalizer when the clients and the > ImageResource die at the same time. This CL may reduce the crash rate, but the > issue is still there, isn't it? I don't think it is; SVGImage is where this is manifesting itself, where we explicitly hold on to the ImageResource while executing animations updates.
On 2016/05/26 08:50:45, sof wrote: > On 2016/05/26 08:43:33, haraken wrote: > > On 2016/05/26 08:20:57, sof wrote: > > > On 2016/05/26 08:16:18, haraken wrote: > > > > On 2016/05/26 05:19:23, sof wrote: > > > > > On 2016/05/26 04:20:27, haraken wrote: > > > > > > > We need to fix willObjectBeLazilySwept() to also handle this rare > lazy > > > > > > sweeping > > > > > > > case; ps#3 addresses. > > > > > > > > > > > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd > prefer > > > > > > decreasing the call site rather than making it more correct... > > > > > > > > > > > > > > > > Don't agree, this is a good solution that I want to go with. > > > > > > > > > > > Stepping back, let me ask a couple of questions: > > > > > > > > > > > > - Do we really need to call resetAnimation when all the clients and > the > > > > > > ImageResource die at the same time? > > > > > > > > > > > > > > > > That's what we've been doing all along & currently. > > > > > > > > > > > - You don't delay calling resetAnimation when all the clients are > > removed > > > > but > > > > > > the ImageResource is still live. This means that style recalc can run > in > > > > > > CSSStyleImage's pre-finalizer, but isn't it dangerous? A pre-finalizer > > is > > > > not > > > > > > allowed to change object graphs (accurately speaking, it is not > allowed > > to > > > > > > resurrect any reference), so it looks unsafe to run a complicated > thing > > > like > > > > > > style recalc during a pre-finalizer. > > > > > > > > > > It's the other way around, when it is dead we cannot delay the call. > Yes, > > we > > > > all > > > > > would want there not to be that amount of code executed when finalizing, > > but > > > > > that's an old topic. > > > > > > > > I don't see any reason we need to run resetAnimations when both the > clients > > > and > > > > the ImageResource at the same time. > > > > > > > > If we just need to run resetAnimations only when the clients are dead but > > the > > > > ImageResource is live, can we just use weak processing to post a task for > > > > resetAnimations? ImageResource can post a task when the weakly observing > > > clients > > > > are gone. > > > > > > resetAnimation() does a lot of work, how would you know that it isn't > needed? > > > This is what ToT does and have been doing for a long time. > > > > > > iow, I'm looking to address an instability here that has a beta-block label > on > > > it, first & foremost. > > > > I'm not convinced that this is a right fix... > > > > The problem is that we're calling resetAnimations during a pre-finalizer. This > > CL still runs resetAnimations during a pre-finalizer when the clients and the > > ImageResource die at the same time. This CL may reduce the crash rate, but the > > issue is still there, isn't it? > > I don't think it is; SVGImage is where this is manifesting itself, where we > explicitly hold on to the ImageResource while executing animations updates. Do you mean that it's not possible that allClientsRemoved() is called when the clients and the ImageResource die at the same time?
On 2016/05/26 08:50:45, sof wrote: > On 2016/05/26 08:43:33, haraken wrote: > > On 2016/05/26 08:20:57, sof wrote: > > > On 2016/05/26 08:16:18, haraken wrote: > > > > On 2016/05/26 05:19:23, sof wrote: > > > > > On 2016/05/26 04:20:27, haraken wrote: > > > > > > > We need to fix willObjectBeLazilySwept() to also handle this rare > lazy > > > > > > sweeping > > > > > > > case; ps#3 addresses. > > > > > > > > > > > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd > prefer > > > > > > decreasing the call site rather than making it more correct... > > > > > > > > > > > > > > > > Don't agree, this is a good solution that I want to go with. > > > > > > > > > > > Stepping back, let me ask a couple of questions: > > > > > > > > > > > > - Do we really need to call resetAnimation when all the clients and > the > > > > > > ImageResource die at the same time? > > > > > > > > > > > > > > > > That's what we've been doing all along & currently. > > > > > > > > > > > - You don't delay calling resetAnimation when all the clients are > > removed > > > > but > > > > > > the ImageResource is still live. This means that style recalc can run > in > > > > > > CSSStyleImage's pre-finalizer, but isn't it dangerous? A pre-finalizer > > is > > > > not > > > > > > allowed to change object graphs (accurately speaking, it is not > allowed > > to > > > > > > resurrect any reference), so it looks unsafe to run a complicated > thing > > > like > > > > > > style recalc during a pre-finalizer. > > > > > > > > > > It's the other way around, when it is dead we cannot delay the call. > Yes, > > we > > > > all > > > > > would want there not to be that amount of code executed when finalizing, > > but > > > > > that's an old topic. > > > > > > > > I don't see any reason we need to run resetAnimations when both the > clients > > > and > > > > the ImageResource at the same time. > > > > > > > > If we just need to run resetAnimations only when the clients are dead but > > the > > > > ImageResource is live, can we just use weak processing to post a task for > > > > resetAnimations? ImageResource can post a task when the weakly observing > > > clients > > > > are gone. > > > > > > resetAnimation() does a lot of work, how would you know that it isn't > needed? > > > This is what ToT does and have been doing for a long time. > > > > > > iow, I'm looking to address an instability here that has a beta-block label > on > > > it, first & foremost. > > > > I'm not convinced that this is a right fix... > > > > The problem is that we're calling resetAnimations during a pre-finalizer. This > > CL still runs resetAnimations during a pre-finalizer when the clients and the > > ImageResource die at the same time. This CL may reduce the crash rate, but the > > issue is still there, isn't it? > > I don't think it is; SVGImage is where this is manifesting itself, where we > explicitly hold on to the ImageResource while executing animations updates. And just to be clear, the resetAnimation() call isn't unsafe in that accesses objects that it shouldn't and similar while running finalization code, but that it upsets an ongoing animations update to have the animation be reset while it runs (and happens to trigger a conservative GC.) Hence delaying the reset'ing.
On 2016/05/26 08:52:56, haraken wrote: > On 2016/05/26 08:50:45, sof wrote: ... > > > > I don't think it is; SVGImage is where this is manifesting itself, where we > > explicitly hold on to the ImageResource while executing animations updates. > > Do you mean that it's not possible that allClientsRemoved() is called when the > clients and the ImageResource die at the same time? It is possible, and it is not a problem for that (lengthy) call to go ahead. Except for reasons in the context of SVGImage, as detailed in #24.
On 2016/05/26 08:58:02, sof wrote: > On 2016/05/26 08:50:45, sof wrote: > > On 2016/05/26 08:43:33, haraken wrote: > > > On 2016/05/26 08:20:57, sof wrote: > > > > On 2016/05/26 08:16:18, haraken wrote: > > > > > On 2016/05/26 05:19:23, sof wrote: > > > > > > On 2016/05/26 04:20:27, haraken wrote: > > > > > > > > We need to fix willObjectBeLazilySwept() to also handle this rare > > lazy > > > > > > > sweeping > > > > > > > > case; ps#3 addresses. > > > > > > > > > > > > > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd > > prefer > > > > > > > decreasing the call site rather than making it more correct... > > > > > > > > > > > > > > > > > > > Don't agree, this is a good solution that I want to go with. > > > > > > > > > > > > > Stepping back, let me ask a couple of questions: > > > > > > > > > > > > > > - Do we really need to call resetAnimation when all the clients and > > the > > > > > > > ImageResource die at the same time? > > > > > > > > > > > > > > > > > > > That's what we've been doing all along & currently. > > > > > > > > > > > > > - You don't delay calling resetAnimation when all the clients are > > > removed > > > > > but > > > > > > > the ImageResource is still live. This means that style recalc can > run > > in > > > > > > > CSSStyleImage's pre-finalizer, but isn't it dangerous? A > pre-finalizer > > > is > > > > > not > > > > > > > allowed to change object graphs (accurately speaking, it is not > > allowed > > > to > > > > > > > resurrect any reference), so it looks unsafe to run a complicated > > thing > > > > like > > > > > > > style recalc during a pre-finalizer. > > > > > > > > > > > > It's the other way around, when it is dead we cannot delay the call. > > Yes, > > > we > > > > > all > > > > > > would want there not to be that amount of code executed when > finalizing, > > > but > > > > > > that's an old topic. > > > > > > > > > > I don't see any reason we need to run resetAnimations when both the > > clients > > > > and > > > > > the ImageResource at the same time. > > > > > > > > > > If we just need to run resetAnimations only when the clients are dead > but > > > the > > > > > ImageResource is live, can we just use weak processing to post a task > for > > > > > resetAnimations? ImageResource can post a task when the weakly observing > > > > clients > > > > > are gone. > > > > > > > > resetAnimation() does a lot of work, how would you know that it isn't > > needed? > > > > This is what ToT does and have been doing for a long time. > > > > > > > > iow, I'm looking to address an instability here that has a beta-block > label > > on > > > > it, first & foremost. > > > > > > I'm not convinced that this is a right fix... > > > > > > The problem is that we're calling resetAnimations during a pre-finalizer. > This > > > CL still runs resetAnimations during a pre-finalizer when the clients and > the > > > ImageResource die at the same time. This CL may reduce the crash rate, but > the > > > issue is still there, isn't it? > > > > I don't think it is; SVGImage is where this is manifesting itself, where we > > explicitly hold on to the ImageResource while executing animations updates. > > And just to be clear, the resetAnimation() call isn't unsafe in that accesses > objects that it shouldn't and similar while running finalization code, but that > it upsets an ongoing animations update to have the animation be reset while it > runs (and happens to trigger a conservative GC.) Hence delaying the reset'ing. Maybe I'm not fully understanding the real problem here. > but that > it upsets an ongoing animations update to have the animation be reset while it > runs (and happens to trigger a conservative GC.) ^^^ Would you elaborate on this? Why can a conservative GC be triggered during resetAnimations? GCs should be forbidden during pre-finalizers.
On 2016/05/26 09:03:11, haraken wrote: > On 2016/05/26 08:58:02, sof wrote: > > On 2016/05/26 08:50:45, sof wrote: > > > On 2016/05/26 08:43:33, haraken wrote: > > > > On 2016/05/26 08:20:57, sof wrote: > > > > > On 2016/05/26 08:16:18, haraken wrote: > > > > > > On 2016/05/26 05:19:23, sof wrote: > > > > > > > On 2016/05/26 04:20:27, haraken wrote: > > > > > > > > > We need to fix willObjectBeLazilySwept() to also handle this > rare > > > lazy > > > > > > > > sweeping > > > > > > > > > case; ps#3 addresses. > > > > > > > > > > > > > > > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd > > > prefer > > > > > > > > decreasing the call site rather than making it more correct... > > > > > > > > > > > > > > > > > > > > > > Don't agree, this is a good solution that I want to go with. > > > > > > > > > > > > > > > Stepping back, let me ask a couple of questions: > > > > > > > > > > > > > > > > - Do we really need to call resetAnimation when all the clients > and > > > the > > > > > > > > ImageResource die at the same time? > > > > > > > > > > > > > > > > > > > > > > That's what we've been doing all along & currently. > > > > > > > > > > > > > > > - You don't delay calling resetAnimation when all the clients are > > > > removed > > > > > > but > > > > > > > > the ImageResource is still live. This means that style recalc can > > run > > > in > > > > > > > > CSSStyleImage's pre-finalizer, but isn't it dangerous? A > > pre-finalizer > > > > is > > > > > > not > > > > > > > > allowed to change object graphs (accurately speaking, it is not > > > allowed > > > > to > > > > > > > > resurrect any reference), so it looks unsafe to run a complicated > > > thing > > > > > like > > > > > > > > style recalc during a pre-finalizer. > > > > > > > > > > > > > > It's the other way around, when it is dead we cannot delay the call. > > > Yes, > > > > we > > > > > > all > > > > > > > would want there not to be that amount of code executed when > > finalizing, > > > > but > > > > > > > that's an old topic. > > > > > > > > > > > > I don't see any reason we need to run resetAnimations when both the > > > clients > > > > > and > > > > > > the ImageResource at the same time. > > > > > > > > > > > > If we just need to run resetAnimations only when the clients are dead > > but > > > > the > > > > > > ImageResource is live, can we just use weak processing to post a task > > for > > > > > > resetAnimations? ImageResource can post a task when the weakly > observing > > > > > clients > > > > > > are gone. > > > > > > > > > > resetAnimation() does a lot of work, how would you know that it isn't > > > needed? > > > > > This is what ToT does and have been doing for a long time. > > > > > > > > > > iow, I'm looking to address an instability here that has a beta-block > > label > > > on > > > > > it, first & foremost. > > > > > > > > I'm not convinced that this is a right fix... > > > > > > > > The problem is that we're calling resetAnimations during a pre-finalizer. > > This > > > > CL still runs resetAnimations during a pre-finalizer when the clients and > > the > > > > ImageResource die at the same time. This CL may reduce the crash rate, but > > the > > > > issue is still there, isn't it? > > > > > > I don't think it is; SVGImage is where this is manifesting itself, where we > > > explicitly hold on to the ImageResource while executing animations updates. > > > > And just to be clear, the resetAnimation() call isn't unsafe in that accesses > > objects that it shouldn't and similar while running finalization code, but > that > > it upsets an ongoing animations update to have the animation be reset while it > > runs (and happens to trigger a conservative GC.) Hence delaying the reset'ing. > > Maybe I'm not fully understanding the real problem here. > > > but that > > it upsets an ongoing animations update to have the animation be reset while it > > runs (and happens to trigger a conservative GC.) > > ^^^ Would you elaborate on this? > > Why can a conservative GC be triggered during resetAnimations? GCs should be > forbidden during pre-finalizers. They don't happen there but while executing the animations update. Please see https://bugs.chromium.org/p/chromium/issues/detail?id=581546#c3 , https://bugs.chromium.org/p/chromium/issues/detail?id=613709#c8 and https://codereview.chromium.org/2005693002/#msg9
On 2016/05/26 09:09:00, sof wrote: > On 2016/05/26 09:03:11, haraken wrote: > > On 2016/05/26 08:58:02, sof wrote: > > > On 2016/05/26 08:50:45, sof wrote: > > > > On 2016/05/26 08:43:33, haraken wrote: > > > > > On 2016/05/26 08:20:57, sof wrote: > > > > > > On 2016/05/26 08:16:18, haraken wrote: > > > > > > > On 2016/05/26 05:19:23, sof wrote: > > > > > > > > On 2016/05/26 04:20:27, haraken wrote: > > > > > > > > > > We need to fix willObjectBeLazilySwept() to also handle this > > rare > > > > lazy > > > > > > > > > sweeping > > > > > > > > > > case; ps#3 addresses. > > > > > > > > > > > > > > > > > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd > > > > prefer > > > > > > > > > decreasing the call site rather than making it more correct... > > > > > > > > > > > > > > > > > > > > > > > > > Don't agree, this is a good solution that I want to go with. > > > > > > > > > > > > > > > > > Stepping back, let me ask a couple of questions: > > > > > > > > > > > > > > > > > > - Do we really need to call resetAnimation when all the clients > > and > > > > the > > > > > > > > > ImageResource die at the same time? > > > > > > > > > > > > > > > > > > > > > > > > > That's what we've been doing all along & currently. > > > > > > > > > > > > > > > > > - You don't delay calling resetAnimation when all the clients > are > > > > > removed > > > > > > > but > > > > > > > > > the ImageResource is still live. This means that style recalc > can > > > run > > > > in > > > > > > > > > CSSStyleImage's pre-finalizer, but isn't it dangerous? A > > > pre-finalizer > > > > > is > > > > > > > not > > > > > > > > > allowed to change object graphs (accurately speaking, it is not > > > > allowed > > > > > to > > > > > > > > > resurrect any reference), so it looks unsafe to run a > complicated > > > > thing > > > > > > like > > > > > > > > > style recalc during a pre-finalizer. > > > > > > > > > > > > > > > > It's the other way around, when it is dead we cannot delay the > call. > > > > Yes, > > > > > we > > > > > > > all > > > > > > > > would want there not to be that amount of code executed when > > > finalizing, > > > > > but > > > > > > > > that's an old topic. > > > > > > > > > > > > > > I don't see any reason we need to run resetAnimations when both the > > > > clients > > > > > > and > > > > > > > the ImageResource at the same time. > > > > > > > > > > > > > > If we just need to run resetAnimations only when the clients are > dead > > > but > > > > > the > > > > > > > ImageResource is live, can we just use weak processing to post a > task > > > for > > > > > > > resetAnimations? ImageResource can post a task when the weakly > > observing > > > > > > clients > > > > > > > are gone. > > > > > > > > > > > > resetAnimation() does a lot of work, how would you know that it isn't > > > > needed? > > > > > > This is what ToT does and have been doing for a long time. > > > > > > > > > > > > iow, I'm looking to address an instability here that has a beta-block > > > label > > > > on > > > > > > it, first & foremost. > > > > > > > > > > I'm not convinced that this is a right fix... > > > > > > > > > > The problem is that we're calling resetAnimations during a > pre-finalizer. > > > This > > > > > CL still runs resetAnimations during a pre-finalizer when the clients > and > > > the > > > > > ImageResource die at the same time. This CL may reduce the crash rate, > but > > > the > > > > > issue is still there, isn't it? > > > > > > > > I don't think it is; SVGImage is where this is manifesting itself, where > we > > > > explicitly hold on to the ImageResource while executing animations > updates. > > > > > > And just to be clear, the resetAnimation() call isn't unsafe in that > accesses > > > objects that it shouldn't and similar while running finalization code, but > > that > > > it upsets an ongoing animations update to have the animation be reset while > it > > > runs (and happens to trigger a conservative GC.) Hence delaying the > reset'ing. > > > > Maybe I'm not fully understanding the real problem here. > > > > > but that > > > it upsets an ongoing animations update to have the animation be reset while > it > > > runs (and happens to trigger a conservative GC.) > > > > ^^^ Would you elaborate on this? > > > > Why can a conservative GC be triggered during resetAnimations? GCs should be > > forbidden during pre-finalizers. > > They don't happen there but while executing the animations update. > > Please see https://bugs.chromium.org/p/chromium/issues/detail?id=581546#c3 , > https://bugs.chromium.org/p/chromium/issues/detail?id=613709#c8 and > https://codereview.chromium.org/2005693002/#msg9 Thanks, I now understand the problem well. Would it be an option to change resetAnimation() to resetAnimationWhenDone()? If resetAnimationWhenDone() is called during an animation, it just sets a flag. The animation calls resetAnimation() at the end of the animation if the flag is set.
On 2016/05/26 16:03:47, haraken wrote: > On 2016/05/26 09:09:00, sof wrote: > > On 2016/05/26 09:03:11, haraken wrote: > > > On 2016/05/26 08:58:02, sof wrote: > > > > On 2016/05/26 08:50:45, sof wrote: > > > > > On 2016/05/26 08:43:33, haraken wrote: > > > > > > On 2016/05/26 08:20:57, sof wrote: > > > > > > > On 2016/05/26 08:16:18, haraken wrote: > > > > > > > > On 2016/05/26 05:19:23, sof wrote: > > > > > > > > > On 2016/05/26 04:20:27, haraken wrote: > > > > > > > > > > > We need to fix willObjectBeLazilySwept() to also handle this > > > rare > > > > > lazy > > > > > > > > > > sweeping > > > > > > > > > > > case; ps#3 addresses. > > > > > > > > > > > > > > > > > > > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, but > I'd > > > > > prefer > > > > > > > > > > decreasing the call site rather than making it more correct... > > > > > > > > > > > > > > > > > > > > > > > > > > > > Don't agree, this is a good solution that I want to go with. > > > > > > > > > > > > > > > > > > > Stepping back, let me ask a couple of questions: > > > > > > > > > > > > > > > > > > > > - Do we really need to call resetAnimation when all the > clients > > > and > > > > > the > > > > > > > > > > ImageResource die at the same time? > > > > > > > > > > > > > > > > > > > > > > > > > > > > That's what we've been doing all along & currently. > > > > > > > > > > > > > > > > > > > - You don't delay calling resetAnimation when all the clients > > are > > > > > > removed > > > > > > > > but > > > > > > > > > > the ImageResource is still live. This means that style recalc > > can > > > > run > > > > > in > > > > > > > > > > CSSStyleImage's pre-finalizer, but isn't it dangerous? A > > > > pre-finalizer > > > > > > is > > > > > > > > not > > > > > > > > > > allowed to change object graphs (accurately speaking, it is > not > > > > > allowed > > > > > > to > > > > > > > > > > resurrect any reference), so it looks unsafe to run a > > complicated > > > > > thing > > > > > > > like > > > > > > > > > > style recalc during a pre-finalizer. > > > > > > > > > > > > > > > > > > It's the other way around, when it is dead we cannot delay the > > call. > > > > > Yes, > > > > > > we > > > > > > > > all > > > > > > > > > would want there not to be that amount of code executed when > > > > finalizing, > > > > > > but > > > > > > > > > that's an old topic. > > > > > > > > > > > > > > > > I don't see any reason we need to run resetAnimations when both > the > > > > > clients > > > > > > > and > > > > > > > > the ImageResource at the same time. > > > > > > > > > > > > > > > > If we just need to run resetAnimations only when the clients are > > dead > > > > but > > > > > > the > > > > > > > > ImageResource is live, can we just use weak processing to post a > > task > > > > for > > > > > > > > resetAnimations? ImageResource can post a task when the weakly > > > observing > > > > > > > clients > > > > > > > > are gone. > > > > > > > > > > > > > > resetAnimation() does a lot of work, how would you know that it > isn't > > > > > needed? > > > > > > > This is what ToT does and have been doing for a long time. > > > > > > > > > > > > > > iow, I'm looking to address an instability here that has a > beta-block > > > > label > > > > > on > > > > > > > it, first & foremost. > > > > > > > > > > > > I'm not convinced that this is a right fix... > > > > > > > > > > > > The problem is that we're calling resetAnimations during a > > pre-finalizer. > > > > This > > > > > > CL still runs resetAnimations during a pre-finalizer when the clients > > and > > > > the > > > > > > ImageResource die at the same time. This CL may reduce the crash rate, > > but > > > > the > > > > > > issue is still there, isn't it? > > > > > > > > > > I don't think it is; SVGImage is where this is manifesting itself, where > > we > > > > > explicitly hold on to the ImageResource while executing animations > > updates. > > > > > > > > And just to be clear, the resetAnimation() call isn't unsafe in that > > accesses > > > > objects that it shouldn't and similar while running finalization code, but > > > that > > > > it upsets an ongoing animations update to have the animation be reset > while > > it > > > > runs (and happens to trigger a conservative GC.) Hence delaying the > > reset'ing. > > > > > > Maybe I'm not fully understanding the real problem here. > > > > > > > but that > > > > it upsets an ongoing animations update to have the animation be reset > while > > it > > > > runs (and happens to trigger a conservative GC.) > > > > > > ^^^ Would you elaborate on this? > > > > > > Why can a conservative GC be triggered during resetAnimations? GCs should be > > > forbidden during pre-finalizers. > > > > They don't happen there but while executing the animations update. > > > > Please see https://bugs.chromium.org/p/chromium/issues/detail?id=581546#c3 , > > https://bugs.chromium.org/p/chromium/issues/detail?id=613709#c8 and > > https://codereview.chromium.org/2005693002/#msg9 > > Thanks, I now understand the problem well. > > Would it be an option to change resetAnimation() to resetAnimationWhenDone()? If > resetAnimationWhenDone() is called during an animation, it just sets a flag. The > animation calls resetAnimation() at the end of the animation if the flag is set. Isn't that what the CL I referred to does ( https://codereview.chromium.org/2005693002/ ) ? See #msg9 why I don't prefer it. This is taking some time.
On 2016/05/26 17:44:40, sof wrote: > On 2016/05/26 16:03:47, haraken wrote: > > On 2016/05/26 09:09:00, sof wrote: > > > On 2016/05/26 09:03:11, haraken wrote: > > > > On 2016/05/26 08:58:02, sof wrote: > > > > > On 2016/05/26 08:50:45, sof wrote: > > > > > > On 2016/05/26 08:43:33, haraken wrote: > > > > > > > On 2016/05/26 08:20:57, sof wrote: > > > > > > > > On 2016/05/26 08:16:18, haraken wrote: > > > > > > > > > On 2016/05/26 05:19:23, sof wrote: > > > > > > > > > > On 2016/05/26 04:20:27, haraken wrote: > > > > > > > > > > > > We need to fix willObjectBeLazilySwept() to also handle > this > > > > rare > > > > > > lazy > > > > > > > > > > > sweeping > > > > > > > > > > > > case; ps#3 addresses. > > > > > > > > > > > > > > > > > > > > > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, but > > I'd > > > > > > prefer > > > > > > > > > > > decreasing the call site rather than making it more > correct... > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Don't agree, this is a good solution that I want to go with. > > > > > > > > > > > > > > > > > > > > > Stepping back, let me ask a couple of questions: > > > > > > > > > > > > > > > > > > > > > > - Do we really need to call resetAnimation when all the > > clients > > > > and > > > > > > the > > > > > > > > > > > ImageResource die at the same time? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > That's what we've been doing all along & currently. > > > > > > > > > > > > > > > > > > > > > - You don't delay calling resetAnimation when all the > clients > > > are > > > > > > > removed > > > > > > > > > but > > > > > > > > > > > the ImageResource is still live. This means that style > recalc > > > can > > > > > run > > > > > > in > > > > > > > > > > > CSSStyleImage's pre-finalizer, but isn't it dangerous? A > > > > > pre-finalizer > > > > > > > is > > > > > > > > > not > > > > > > > > > > > allowed to change object graphs (accurately speaking, it is > > not > > > > > > allowed > > > > > > > to > > > > > > > > > > > resurrect any reference), so it looks unsafe to run a > > > complicated > > > > > > thing > > > > > > > > like > > > > > > > > > > > style recalc during a pre-finalizer. > > > > > > > > > > > > > > > > > > > > It's the other way around, when it is dead we cannot delay the > > > call. > > > > > > Yes, > > > > > > > we > > > > > > > > > all > > > > > > > > > > would want there not to be that amount of code executed when > > > > > finalizing, > > > > > > > but > > > > > > > > > > that's an old topic. > > > > > > > > > > > > > > > > > > I don't see any reason we need to run resetAnimations when both > > the > > > > > > clients > > > > > > > > and > > > > > > > > > the ImageResource at the same time. > > > > > > > > > > > > > > > > > > If we just need to run resetAnimations only when the clients are > > > dead > > > > > but > > > > > > > the > > > > > > > > > ImageResource is live, can we just use weak processing to post a > > > task > > > > > for > > > > > > > > > resetAnimations? ImageResource can post a task when the weakly > > > > observing > > > > > > > > clients > > > > > > > > > are gone. > > > > > > > > > > > > > > > > resetAnimation() does a lot of work, how would you know that it > > isn't > > > > > > needed? > > > > > > > > This is what ToT does and have been doing for a long time. > > > > > > > > > > > > > > > > iow, I'm looking to address an instability here that has a > > beta-block > > > > > label > > > > > > on > > > > > > > > it, first & foremost. > > > > > > > > > > > > > > I'm not convinced that this is a right fix... > > > > > > > > > > > > > > The problem is that we're calling resetAnimations during a > > > pre-finalizer. > > > > > This > > > > > > > CL still runs resetAnimations during a pre-finalizer when the > clients > > > and > > > > > the > > > > > > > ImageResource die at the same time. This CL may reduce the crash > rate, > > > but > > > > > the > > > > > > > issue is still there, isn't it? > > > > > > > > > > > > I don't think it is; SVGImage is where this is manifesting itself, > where > > > we > > > > > > explicitly hold on to the ImageResource while executing animations > > > updates. > > > > > > > > > > And just to be clear, the resetAnimation() call isn't unsafe in that > > > accesses > > > > > objects that it shouldn't and similar while running finalization code, > but > > > > that > > > > > it upsets an ongoing animations update to have the animation be reset > > while > > > it > > > > > runs (and happens to trigger a conservative GC.) Hence delaying the > > > reset'ing. > > > > > > > > Maybe I'm not fully understanding the real problem here. > > > > > > > > > but that > > > > > it upsets an ongoing animations update to have the animation be reset > > while > > > it > > > > > runs (and happens to trigger a conservative GC.) > > > > > > > > ^^^ Would you elaborate on this? > > > > > > > > Why can a conservative GC be triggered during resetAnimations? GCs should > be > > > > forbidden during pre-finalizers. > > > > > > They don't happen there but while executing the animations update. > > > > > > Please see https://bugs.chromium.org/p/chromium/issues/detail?id=581546#c3 , > > > https://bugs.chromium.org/p/chromium/issues/detail?id=613709#c8 and > > > https://codereview.chromium.org/2005693002/#msg9 > > > > Thanks, I now understand the problem well. > > > > Would it be an option to change resetAnimation() to resetAnimationWhenDone()? > If > > resetAnimationWhenDone() is called during an animation, it just sets a flag. > The > > animation calls resetAnimation() at the end of the animation if the flag is > set. > > Isn't that what the CL I referred to does ( > https://codereview.chromium.org/2005693002/ ) ? See #msg9 why I don't prefer it. > > This is taking some time. I'd prefer the approach in https://codereview.chromium.org/2005693002/. Or we should try to keep alive the clients while doing the animation (If this is doable, I prefer this the best). This CL is doing something really hacky: - When the clients are dead but the ImageResource is live, post a task for resetAnimation because the animation may be still running at that point. - When the clients and the ImageResource are dead at the same time, don't post a task because we cannot post a task (because the ImageResource is already dead). But this is fine (by accident) because "the clients and the ImageResource are dead at the same time" should not happen while running the animation. - I don't want to increase the usage of "mysterious" APIs like willObjectBeLazilySwept(), which is hard for developers to understand.
On 2016/05/26 22:58:08, haraken wrote: > On 2016/05/26 17:44:40, sof wrote: > > On 2016/05/26 16:03:47, haraken wrote: > > > On 2016/05/26 09:09:00, sof wrote: > > > > On 2016/05/26 09:03:11, haraken wrote: > > > > > On 2016/05/26 08:58:02, sof wrote: > > > > > > On 2016/05/26 08:50:45, sof wrote: > > > > > > > On 2016/05/26 08:43:33, haraken wrote: > > > > > > > > On 2016/05/26 08:20:57, sof wrote: > > > > > > > > > On 2016/05/26 08:16:18, haraken wrote: > > > > > > > > > > On 2016/05/26 05:19:23, sof wrote: > > > > > > > > > > > On 2016/05/26 04:20:27, haraken wrote: > > > > > > > > > > > > > We need to fix willObjectBeLazilySwept() to also handle > > this > > > > > rare > > > > > > > lazy > > > > > > > > > > > > sweeping > > > > > > > > > > > > > case; ps#3 addresses. > > > > > > > > > > > > > > > > > > > > > > > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, > but > > > I'd > > > > > > > prefer > > > > > > > > > > > > decreasing the call site rather than making it more > > correct... > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Don't agree, this is a good solution that I want to go with. > > > > > > > > > > > > > > > > > > > > > > > Stepping back, let me ask a couple of questions: > > > > > > > > > > > > > > > > > > > > > > > > - Do we really need to call resetAnimation when all the > > > clients > > > > > and > > > > > > > the > > > > > > > > > > > > ImageResource die at the same time? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > That's what we've been doing all along & currently. > > > > > > > > > > > > > > > > > > > > > > > - You don't delay calling resetAnimation when all the > > clients > > > > are > > > > > > > > removed > > > > > > > > > > but > > > > > > > > > > > > the ImageResource is still live. This means that style > > recalc > > > > can > > > > > > run > > > > > > > in > > > > > > > > > > > > CSSStyleImage's pre-finalizer, but isn't it dangerous? A > > > > > > pre-finalizer > > > > > > > > is > > > > > > > > > > not > > > > > > > > > > > > allowed to change object graphs (accurately speaking, it > is > > > not > > > > > > > allowed > > > > > > > > to > > > > > > > > > > > > resurrect any reference), so it looks unsafe to run a > > > > complicated > > > > > > > thing > > > > > > > > > like > > > > > > > > > > > > style recalc during a pre-finalizer. > > > > > > > > > > > > > > > > > > > > > > It's the other way around, when it is dead we cannot delay > the > > > > call. > > > > > > > Yes, > > > > > > > > we > > > > > > > > > > all > > > > > > > > > > > would want there not to be that amount of code executed when > > > > > > finalizing, > > > > > > > > but > > > > > > > > > > > that's an old topic. > > > > > > > > > > > > > > > > > > > > I don't see any reason we need to run resetAnimations when > both > > > the > > > > > > > clients > > > > > > > > > and > > > > > > > > > > the ImageResource at the same time. > > > > > > > > > > > > > > > > > > > > If we just need to run resetAnimations only when the clients > are > > > > dead > > > > > > but > > > > > > > > the > > > > > > > > > > ImageResource is live, can we just use weak processing to post > a > > > > task > > > > > > for > > > > > > > > > > resetAnimations? ImageResource can post a task when the weakly > > > > > observing > > > > > > > > > clients > > > > > > > > > > are gone. > > > > > > > > > > > > > > > > > > resetAnimation() does a lot of work, how would you know that it > > > isn't > > > > > > > needed? > > > > > > > > > This is what ToT does and have been doing for a long time. > > > > > > > > > > > > > > > > > > iow, I'm looking to address an instability here that has a > > > beta-block > > > > > > label > > > > > > > on > > > > > > > > > it, first & foremost. > > > > > > > > > > > > > > > > I'm not convinced that this is a right fix... > > > > > > > > > > > > > > > > The problem is that we're calling resetAnimations during a > > > > pre-finalizer. > > > > > > This > > > > > > > > CL still runs resetAnimations during a pre-finalizer when the > > clients > > > > and > > > > > > the > > > > > > > > ImageResource die at the same time. This CL may reduce the crash > > rate, > > > > but > > > > > > the > > > > > > > > issue is still there, isn't it? > > > > > > > > > > > > > > I don't think it is; SVGImage is where this is manifesting itself, > > where > > > > we > > > > > > > explicitly hold on to the ImageResource while executing animations > > > > updates. > > > > > > > > > > > > And just to be clear, the resetAnimation() call isn't unsafe in that > > > > accesses > > > > > > objects that it shouldn't and similar while running finalization code, > > but > > > > > that > > > > > > it upsets an ongoing animations update to have the animation be reset > > > while > > > > it > > > > > > runs (and happens to trigger a conservative GC.) Hence delaying the > > > > reset'ing. > > > > > > > > > > Maybe I'm not fully understanding the real problem here. > > > > > > > > > > > but that > > > > > > it upsets an ongoing animations update to have the animation be reset > > > while > > > > it > > > > > > runs (and happens to trigger a conservative GC.) > > > > > > > > > > ^^^ Would you elaborate on this? > > > > > > > > > > Why can a conservative GC be triggered during resetAnimations? GCs > should > > be > > > > > forbidden during pre-finalizers. > > > > > > > > They don't happen there but while executing the animations update. > > > > > > > > Please see https://bugs.chromium.org/p/chromium/issues/detail?id=581546#c3 > , > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=613709#c8 and > > > > https://codereview.chromium.org/2005693002/#msg9 > > > > > > Thanks, I now understand the problem well. > > > > > > Would it be an option to change resetAnimation() to > resetAnimationWhenDone()? > > If > > > resetAnimationWhenDone() is called during an animation, it just sets a flag. > > The > > > animation calls resetAnimation() at the end of the animation if the flag is > > set. > > > > Isn't that what the CL I referred to does ( > > https://codereview.chromium.org/2005693002/ ) ? See #msg9 why I don't prefer > it. > > > > This is taking some time. > > I'd prefer the approach in https://codereview.chromium.org/2005693002/. Or we > should try to keep alive the clients while doing the animation (If this is > doable, I prefer this the best). > > This CL is doing something really hacky: > > - When the clients are dead but the ImageResource is live, post a task for > resetAnimation because the animation may be still running at that point. > > - When the clients and the ImageResource are dead at the same time, don't post a > task because we cannot post a task (because the ImageResource is already dead). > But this is fine (by accident) because "the clients and the ImageResource are > dead at the same time" should not happen while running the animation. > > - I don't want to increase the usage of "mysterious" APIs like > willObjectBeLazilySwept(), which is hard for developers to understand. i see, don't agree with your assessment(s) here. I won't pursue that approach.
Rebased up past r396798.
On 2016/06/03 06:39:17, sof wrote: > Rebased up past r396798. So... Any chance for progress here? There is still action on https://bugs.chromium.org/p/chromium/issues/detail?id=581546
On 2016/06/20 06:42:09, David Vest wrote: > On 2016/06/03 06:39:17, sof wrote: > > Rebased up past r396798. > > So... Any chance for progress here? > > There is still action on > https://bugs.chromium.org/p/chromium/issues/detail?id=581546 I still consider this (ps#5) the appropriate fix.
LGTM. I don't think this is a right fix, but it's better to land this than not landing this.
Description was changed from ========== Delay resetting image animation, if possible. When the last client of an ImageResource removes itself, the animations of the image is explicitly reset. That resetting can happen either while finalizing objects after a GC or as part of other explicit removals of ImageObserver clients. Having that reset happen as part of a garbage collection is interacting badly with code in the middle of updating animations (which happen to trigger a GC), so to avoid introducing such abrupt resets, delay the operation until back at the event loop (and the animations update step having completed.) R= BUG=613709, 581546 ========== to ========== Delay resetting image animation, if possible. When the last client of an ImageResource removes itself, the animations of the image is explicitly reset. That resetting can happen either while finalizing objects after a GC or as part of other explicit removals of ImageObserver clients. Having that reset happen as part of a garbage collection is interacting badly with code in the middle of updating animations (which happen to trigger a conservative GC.) So, to avoid introducing such abrupt & harmful resets, delay the reset'ing until back at the event loop (and the animations update step having completed.) R= BUG=613709, 581546 ==========
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2004263003/#ps100001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004263003/100001
Message was sent while issue was closed.
Description was changed from ========== Delay resetting image animation, if possible. When the last client of an ImageResource removes itself, the animations of the image is explicitly reset. That resetting can happen either while finalizing objects after a GC or as part of other explicit removals of ImageObserver clients. Having that reset happen as part of a garbage collection is interacting badly with code in the middle of updating animations (which happen to trigger a conservative GC.) So, to avoid introducing such abrupt & harmful resets, delay the reset'ing until back at the event loop (and the animations update step having completed.) R= BUG=613709, 581546 ========== to ========== Delay resetting image animation, if possible. When the last client of an ImageResource removes itself, the animations of the image is explicitly reset. That resetting can happen either while finalizing objects after a GC or as part of other explicit removals of ImageObserver clients. Having that reset happen as part of a garbage collection is interacting badly with code in the middle of updating animations (which happen to trigger a conservative GC.) So, to avoid introducing such abrupt & harmful resets, delay the reset'ing until back at the event loop (and the animations update step having completed.) R= BUG=613709, 581546 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Delay resetting image animation, if possible. When the last client of an ImageResource removes itself, the animations of the image is explicitly reset. That resetting can happen either while finalizing objects after a GC or as part of other explicit removals of ImageObserver clients. Having that reset happen as part of a garbage collection is interacting badly with code in the middle of updating animations (which happen to trigger a conservative GC.) So, to avoid introducing such abrupt & harmful resets, delay the reset'ing until back at the event loop (and the animations update step having completed.) R= BUG=613709, 581546 ========== to ========== Delay resetting image animation, if possible. When the last client of an ImageResource removes itself, the animations of the image is explicitly reset. That resetting can happen either while finalizing objects after a GC or as part of other explicit removals of ImageObserver clients. Having that reset happen as part of a garbage collection is interacting badly with code in the middle of updating animations (which happen to trigger a conservative GC.) So, to avoid introducing such abrupt & harmful resets, delay the reset'ing until back at the event loop (and the animations update step having completed.) R= BUG=613709, 581546 Committed: https://crrev.com/a9a17309a49caff18a2cffd43867d99aa20cf42e Cr-Commit-Position: refs/heads/master@{#400934} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a9a17309a49caff18a2cffd43867d99aa20cf42e Cr-Commit-Position: refs/heads/master@{#400934} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
