Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2247783003:
Avoid setting timers from SVGImage::resetAnimation() (Closed)
|
Created:
4 years, 4 months ago by fs Modified:
4 years, 4 months ago Reviewers:
pdr. CC:
blink-reviews, chromium-reviews, krit, f(malita), gyuyoung2, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid setting timers from SVGImage::resetAnimation()
When resetting the timeline to t=0, we may up generating syncbase
notification, which sets up a timer (to update any possibly dependent
intervals.) Since resetAnimation() is what's called when the (SVG)Image
no longer has any clients, we should try to make sure it is indeed
idle after that happens. This avoids trying to update animation state
while the image is otherwise dead, leaving "reactivation" to the time
it is next painted.
BUG=627418
Committed: https://crrev.com/12c039d9866df4bae5c2b1a398b04816701233b3
Cr-Commit-Position: refs/heads/master@{#412798}
Patch Set 1 #
Messages
Total messages: 21 (9 generated)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fs@opera.com changed reviewers: + pdr@chromium.org
No test here because it involve racing against the GC-heuristics (takes quite a while to repro for me =/.) I think we should have tests for the reset semantics already so that angle is hopefully covered.
This isn't crashing at all for me locally. Did you use ASAN/MSAN/PARMESAN or does it crash for you in debug? Just looking at the stacktraces and code... there seem to be a few timers: one attached to SVGImage (through SVGImageChromeClient) and one (kinda two) per SVGSVGElement/SVGSMILElement (through SMILTimeContainer). Do we need to go crawl around the tree and stop all of the SMILTimeContainer timers, not just the root one? A more meta-question, why isn't this timer getting cancelled on its own when the SVGSVGElement destructs which destructs the SMILTimeContainer?
On 2016/08/17 at 03:20:30, pdr wrote: > This isn't crashing at all for me locally. Did you use ASAN/MSAN/PARMESAN or does it crash for you in debug? It (timloh's reduction) crashes for me in both Debug and Release - no *SAN required (not even Grana PadanoSAN.) As mentioned though, it can take quite a while (minutes.) > Just looking at the stacktraces and code... there seem to be a few timers: one attached to SVGImage (through SVGImageChromeClient) and one (kinda two) per SVGSVGElement/SVGSMILElement (through SMILTimeContainer). Do we need to go crawl around the tree and stop all of the SMILTimeContainer timers, not just the root one? That's a relevant question... we probably should have SVGImage go through the (top-level) SVGDocumentExtensions rather than just the document root for all the SMIL manipulation. (Arguably we could just let only the outermost root have a active time container - IIRC that's what we did in Presto. Sub-SVG level timeline control is pretty crazy.) I'll see about modifying the testcase so that it has more than one timeline and probably do a precursor CL to fix that first. > A more meta-question, why isn't this timer getting cancelled on its own when the SVGSVGElement destructs which destructs the SMILTimeContainer? The latter is referenced (since it's on the stack) and in turn has a reference to the former, meaning they'll both be alive after the GC sweep has taken place.
On 2016/08/17 at 09:15:05, fs wrote: > On 2016/08/17 at 03:20:30, pdr wrote: > > Just looking at the stacktraces and code... there seem to be a few timers: one attached to SVGImage (through SVGImageChromeClient) and one (kinda two) per SVGSVGElement/SVGSMILElement (through SMILTimeContainer). Do we need to go crawl around the tree and stop all of the SMILTimeContainer timers, not just the root one? > > That's a relevant question... we probably should have SVGImage go through the (top-level) SVGDocumentExtensions rather than just the document root for all the SMIL manipulation. (Arguably we could just let only the outermost root have a active time container - IIRC that's what we did in Presto. Sub-SVG level timeline control is pretty crazy.) I'll see about modifying the testcase so that it has more than one timeline and probably do a precursor CL to fix that first. I've been trying to provoke problems via nested time containers, but I've yet to succeed. I filed crbug.com/638568 for pondering the time container situation - need to look at various specs too I guess (one per document is alluring...)
On 2016/08/17 at 12:57:04, fs wrote: > On 2016/08/17 at 09:15:05, fs wrote: > > On 2016/08/17 at 03:20:30, pdr wrote: > > > Just looking at the stacktraces and code... there seem to be a few timers: one attached to SVGImage (through SVGImageChromeClient) and one (kinda two) per SVGSVGElement/SVGSMILElement (through SMILTimeContainer). Do we need to go crawl around the tree and stop all of the SMILTimeContainer timers, not just the root one? > > > > That's a relevant question... we probably should have SVGImage go through the (top-level) SVGDocumentExtensions rather than just the document root for all the SMIL manipulation. (Arguably we could just let only the outermost root have a active time container - IIRC that's what we did in Presto. Sub-SVG level timeline control is pretty crazy.) I'll see about modifying the testcase so that it has more than one timeline and probably do a precursor CL to fix that first. > > I've been trying to provoke problems via nested time containers, but I've yet to succeed. I filed crbug.com/638568 for pondering the time container situation - need to look at various specs too I guess (one per document is alluring...) Thanks for investigating. One per document does sound really nice. It might be a simple change, or not... do you think we should proceed with a change like this, or investigate the one-per-doc first?
On 2016/08/17 at 18:01:28, pdr wrote: > On 2016/08/17 at 12:57:04, fs wrote: > > On 2016/08/17 at 09:15:05, fs wrote: > > > On 2016/08/17 at 03:20:30, pdr wrote: > > > > Just looking at the stacktraces and code... there seem to be a few timers: one attached to SVGImage (through SVGImageChromeClient) and one (kinda two) per SVGSVGElement/SVGSMILElement (through SMILTimeContainer). Do we need to go crawl around the tree and stop all of the SMILTimeContainer timers, not just the root one? > > > > > > That's a relevant question... we probably should have SVGImage go through the (top-level) SVGDocumentExtensions rather than just the document root for all the SMIL manipulation. (Arguably we could just let only the outermost root have a active time container - IIRC that's what we did in Presto. Sub-SVG level timeline control is pretty crazy.) I'll see about modifying the testcase so that it has more than one timeline and probably do a precursor CL to fix that first. > > > > I've been trying to provoke problems via nested time containers, but I've yet to succeed. I filed crbug.com/638568 for pondering the time container situation - need to look at various specs too I guess (one per document is alluring...) > > Thanks for investigating. One per document does sound really nice. It might be a simple change, or not... do you think we should proceed with a change like this, or investigate the one-per-doc first? Since we might want to backport this, I'd like to go with the "plug-the-hole" fix first. Moving/merging time container will be an observable (and spec-breaking) change too, so may need use-counting etc. first. (Implementation-wise I don't think it'll be too difficult.)
On 2016/08/17 at 18:14:23, fs wrote: > On 2016/08/17 at 18:01:28, pdr wrote: > > On 2016/08/17 at 12:57:04, fs wrote: > > > On 2016/08/17 at 09:15:05, fs wrote: > > > > On 2016/08/17 at 03:20:30, pdr wrote: > > > > > Just looking at the stacktraces and code... there seem to be a few timers: one attached to SVGImage (through SVGImageChromeClient) and one (kinda two) per SVGSVGElement/SVGSMILElement (through SMILTimeContainer). Do we need to go crawl around the tree and stop all of the SMILTimeContainer timers, not just the root one? > > > > > > > > That's a relevant question... we probably should have SVGImage go through the (top-level) SVGDocumentExtensions rather than just the document root for all the SMIL manipulation. (Arguably we could just let only the outermost root have a active time container - IIRC that's what we did in Presto. Sub-SVG level timeline control is pretty crazy.) I'll see about modifying the testcase so that it has more than one timeline and probably do a precursor CL to fix that first. > > > > > > I've been trying to provoke problems via nested time containers, but I've yet to succeed. I filed crbug.com/638568 for pondering the time container situation - need to look at various specs too I guess (one per document is alluring...) > > > > Thanks for investigating. One per document does sound really nice. It might be a simple change, or not... do you think we should proceed with a change like this, or investigate the one-per-doc first? > > Since we might want to backport this, I'd like to go with the "plug-the-hole" fix first. Moving/merging time container will be an observable (and spec-breaking) change too, so may need use-counting etc. first. (Implementation-wise I don't think it'll be too difficult.) SGTM LGTM
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Avoid setting timers from SVGImage::resetAnimation() When resetting the timeline to t=0, we may up generating syncbase notification, which sets up a timer (to update any possibly dependent intervals.) Since resetAnimation() is what's called when the (SVG)Image no longer has any clients, we should try to make sure it is indeed idle after that happens. This avoids trying to update animation state while the image is otherwise dead, leaving "reactivation" to the time it is next painted. BUG=627418 ========== to ========== Avoid setting timers from SVGImage::resetAnimation() When resetting the timeline to t=0, we may up generating syncbase notification, which sets up a timer (to update any possibly dependent intervals.) Since resetAnimation() is what's called when the (SVG)Image no longer has any clients, we should try to make sure it is indeed idle after that happens. This avoids trying to update animation state while the image is otherwise dead, leaving "reactivation" to the time it is next painted. BUG=627418 Committed: https://crrev.com/12c039d9866df4bae5c2b1a398b04816701233b3 Cr-Commit-Position: refs/heads/master@{#412798} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/12c039d9866df4bae5c2b1a398b04816701233b3 Cr-Commit-Position: refs/heads/master@{#412798} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
