Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
891263003:
Keep SVGImage alive across its animation timer callback. (Closed)
|
Created:
5 years, 10 months ago by sof Modified:
5 years, 10 months ago CC:
blink-reviews, krit, kouhei+svg_chromium.org, ed+blinkwatch_opera.com, f(malita), gyuyoung.kim_webkit.org, Stephen Chennney, pdr+svgwatchlist_chromium.org, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionKeep SVGImage alive across its animation timer callback.
An SVGImage runs the risk of being finalized while its animation
callback is running. Add the required protection.
The premature finalization is only a possibility with Oilpan enabled;
see code comment.
R=haraken,fs@opera.com
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189338
Patch Set 1 #
Total comments: 4
Patch Set 2 : a stack protection is required here #Messages
Total messages: 21 (6 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
Please take a look. Blind fix for https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... File Source/core/svg/graphics/SVGImageChromeClient.cpp (right): https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... Source/core/svg/graphics/SVGImageChromeClient.cpp:89: m_image->frameView()->page()->animator().serviceScriptedAnimations(monotonicallyIncreasingTime()); m_image is on stack and thus it's protected, isn't it?
https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... File Source/core/svg/graphics/SVGImageChromeClient.cpp (right): https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... Source/core/svg/graphics/SVGImageChromeClient.cpp:89: m_image->frameView()->page()->animator().serviceScriptedAnimations(monotonicallyIncreasingTime()); On 2015/02/02 10:49:01, haraken wrote: > > m_image is on stack and thus it's protected, isn't it? Not necessarily. "this" would be, but it is not a GCed object.
LGTM https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... File Source/core/svg/graphics/SVGImageChromeClient.cpp (right): https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... Source/core/svg/graphics/SVGImageChromeClient.cpp:89: m_image->frameView()->page()->animator().serviceScriptedAnimations(monotonicallyIncreasingTime()); On 2015/02/02 10:51:40, sof wrote: > On 2015/02/02 10:49:01, haraken wrote: > > > > m_image is on stack and thus it's protected, isn't it? > > Not necessarily. "this" would be, but it is not a GCed object. ah, understood... This looks nasty and we might want to move ChromeClient to the heap, ideally.
On 2015/02/02 10:55:42, haraken wrote: > LGTM > > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > File Source/core/svg/graphics/SVGImageChromeClient.cpp (right): > > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > Source/core/svg/graphics/SVGImageChromeClient.cpp:89: > m_image->frameView()->page()->animator().serviceScriptedAnimations(monotonicallyIncreasingTime()); > On 2015/02/02 10:51:40, sof wrote: > > On 2015/02/02 10:49:01, haraken wrote: > > > > > > m_image is on stack and thus it's protected, isn't it? > > > > Not necessarily. "this" would be, but it is not a GCed object. > > ah, understood... This looks nasty and we might want to move ChromeClient to the > heap, ideally. Agreed, moving the ChromeClients would be a good (future) goal.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/891263003/1
fs@opera.com changed reviewers: + fs@opera.com
https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... File Source/core/svg/graphics/SVGImageChromeClient.cpp (right): https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... Source/core/svg/graphics/SVGImageChromeClient.cpp:89: m_image->frameView()->page()->animator().serviceScriptedAnimations(monotonicallyIncreasingTime()); On 2015/02/02 10:55:42, haraken wrote: > On 2015/02/02 10:51:40, sof wrote: > > On 2015/02/02 10:49:01, haraken wrote: > > > > > > m_image is on stack and thus it's protected, isn't it? > > > > Not necessarily. "this" would be, but it is not a GCed object. > > ah, understood... This looks nasty and we might want to move ChromeClient to the > heap, ideally. Isn't 'this' owned by 'm_image' here though?
On 2015/02/02 11:53:47, fs wrote: > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > File Source/core/svg/graphics/SVGImageChromeClient.cpp (right): > > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > Source/core/svg/graphics/SVGImageChromeClient.cpp:89: > m_image->frameView()->page()->animator().serviceScriptedAnimations(monotonicallyIncreasingTime()); > On 2015/02/02 10:55:42, haraken wrote: > > On 2015/02/02 10:51:40, sof wrote: > > > On 2015/02/02 10:49:01, haraken wrote: > > > > > > > > m_image is on stack and thus it's protected, isn't it? > > > > > > Not necessarily. "this" would be, but it is not a GCed object. > > > > ah, understood... This looks nasty and we might want to move ChromeClient to > the > > heap, ideally. > > Isn't 'this' owned by 'm_image' here though? Via an OwnPtr, yes.
On 2015/02/02 11:56:39, sof wrote: > On 2015/02/02 11:53:47, fs wrote: > > > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > > File Source/core/svg/graphics/SVGImageChromeClient.cpp (right): > > > > > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > > Source/core/svg/graphics/SVGImageChromeClient.cpp:89: > > > m_image->frameView()->page()->animator().serviceScriptedAnimations(monotonicallyIncreasingTime()); > > On 2015/02/02 10:55:42, haraken wrote: > > > On 2015/02/02 10:51:40, sof wrote: > > > > On 2015/02/02 10:49:01, haraken wrote: > > > > > > > > > > m_image is on stack and thus it's protected, isn't it? > > > > > > > > Not necessarily. "this" would be, but it is not a GCed object. > > > > > > ah, understood... This looks nasty and we might want to move ChromeClient to > > the > > > heap, ideally. > > > > Isn't 'this' owned by 'm_image' here though? > > Via an OwnPtr, yes. Maybe I'm confused, but I read "...the SVG image isn't otherwise referred to, it will be finalized and m_image is cleared.", and inferred that "finalized" -> "~SVGImage called" -> SVGImageChromeClient also destroyed -> |this| pointing to freed memory.
The CQ bit was unchecked by sigbjornf@opera.com
On 2015/02/02 12:15:32, fs wrote: > On 2015/02/02 11:56:39, sof wrote: > > On 2015/02/02 11:53:47, fs wrote: > > > > > > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > > > File Source/core/svg/graphics/SVGImageChromeClient.cpp (right): > > > > > > > > > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > > > Source/core/svg/graphics/SVGImageChromeClient.cpp:89: > > > > > > m_image->frameView()->page()->animator().serviceScriptedAnimations(monotonicallyIncreasingTime()); > > > On 2015/02/02 10:55:42, haraken wrote: > > > > On 2015/02/02 10:51:40, sof wrote: > > > > > On 2015/02/02 10:49:01, haraken wrote: > > > > > > > > > > > > m_image is on stack and thus it's protected, isn't it? > > > > > > > > > > Not necessarily. "this" would be, but it is not a GCed object. > > > > > > > > ah, understood... This looks nasty and we might want to move ChromeClient > to > > > the > > > > heap, ideally. > > > > > > Isn't 'this' owned by 'm_image' here though? > > > > Via an OwnPtr, yes. > > Maybe I'm confused, but I read "...the SVG image isn't otherwise referred to, it > will be finalized and m_image is cleared.", and inferred that "finalized" -> > "~SVGImage called" -> SVGImageChromeClient also destroyed -> |this| pointing to > freed memory. You're right, it is vulnerable to that. Addressing.
On 2015/02/02 12:23:44, sof wrote: > On 2015/02/02 12:15:32, fs wrote: > > On 2015/02/02 11:56:39, sof wrote: > > > On 2015/02/02 11:53:47, fs wrote: > > > > > > > > > > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > > > > File Source/core/svg/graphics/SVGImageChromeClient.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > > > > Source/core/svg/graphics/SVGImageChromeClient.cpp:89: > > > > > > > > > > m_image->frameView()->page()->animator().serviceScriptedAnimations(monotonicallyIncreasingTime()); > > > > On 2015/02/02 10:55:42, haraken wrote: > > > > > On 2015/02/02 10:51:40, sof wrote: > > > > > > On 2015/02/02 10:49:01, haraken wrote: > > > > > > > > > > > > > > m_image is on stack and thus it's protected, isn't it? > > > > > > > > > > > > Not necessarily. "this" would be, but it is not a GCed object. > > > > > > > > > > ah, understood... This looks nasty and we might want to move > ChromeClient > > to > > > > the > > > > > heap, ideally. > > > > > > > > Isn't 'this' owned by 'm_image' here though? > > > > > > Via an OwnPtr, yes. > > > > Maybe I'm confused, but I read "...the SVG image isn't otherwise referred to, > it > > will be finalized and m_image is cleared.", and inferred that "finalized" -> > > "~SVGImage called" -> SVGImageChromeClient also destroyed -> |this| pointing > to > > freed memory. > > You're right, it is vulnerable to that. Addressing. Done; i.e., protect the SVGImage (and by implication, "ourselves") for the duration of this timer callback.
On 2015/02/02 12:46:05, sof wrote: > On 2015/02/02 12:23:44, sof wrote: > > On 2015/02/02 12:15:32, fs wrote: > > > On 2015/02/02 11:56:39, sof wrote: > > > > On 2015/02/02 11:53:47, fs wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > > > > > File Source/core/svg/graphics/SVGImageChromeClient.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > > > > > Source/core/svg/graphics/SVGImageChromeClient.cpp:89: > > > > > > > > > > > > > > > m_image->frameView()->page()->animator().serviceScriptedAnimations(monotonicallyIncreasingTime()); > > > > > On 2015/02/02 10:55:42, haraken wrote: > > > > > > On 2015/02/02 10:51:40, sof wrote: > > > > > > > On 2015/02/02 10:49:01, haraken wrote: > > > > > > > > > > > > > > > > m_image is on stack and thus it's protected, isn't it? > > > > > > > > > > > > > > Not necessarily. "this" would be, but it is not a GCed object. > > > > > > > > > > > > ah, understood... This looks nasty and we might want to move > > ChromeClient > > > to > > > > > the > > > > > > heap, ideally. > > > > > > > > > > Isn't 'this' owned by 'm_image' here though? > > > > > > > > Via an OwnPtr, yes. > > > > > > Maybe I'm confused, but I read "...the SVG image isn't otherwise referred > to, > > it > > > will be finalized and m_image is cleared.", and inferred that "finalized" -> > > > "~SVGImage called" -> SVGImageChromeClient also destroyed -> |this| pointing > > to > > > freed memory. > > > > You're right, it is vulnerable to that. Addressing. > > Done; i.e., protect the SVGImage (and by implication, "ourselves") for the > duration of this timer callback. LGTM, thanks.
On 2015/02/02 12:47:27, fs wrote: > On 2015/02/02 12:46:05, sof wrote: > > On 2015/02/02 12:23:44, sof wrote: > > > On 2015/02/02 12:15:32, fs wrote: > > > > On 2015/02/02 11:56:39, sof wrote: > > > > > On 2015/02/02 11:53:47, fs wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > > > > > > File Source/core/svg/graphics/SVGImageChromeClient.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/891263003/diff/1/Source/core/svg/graphics/SVG... > > > > > > Source/core/svg/graphics/SVGImageChromeClient.cpp:89: > > > > > > > > > > > > > > > > > > > > > m_image->frameView()->page()->animator().serviceScriptedAnimations(monotonicallyIncreasingTime()); > > > > > > On 2015/02/02 10:55:42, haraken wrote: > > > > > > > On 2015/02/02 10:51:40, sof wrote: > > > > > > > > On 2015/02/02 10:49:01, haraken wrote: > > > > > > > > > > > > > > > > > > m_image is on stack and thus it's protected, isn't it? > > > > > > > > > > > > > > > > Not necessarily. "this" would be, but it is not a GCed object. > > > > > > > > > > > > > > ah, understood... This looks nasty and we might want to move > > > ChromeClient > > > > to > > > > > > the > > > > > > > heap, ideally. > > > > > > > > > > > > Isn't 'this' owned by 'm_image' here though? > > > > > > > > > > Via an OwnPtr, yes. > > > > > > > > Maybe I'm confused, but I read "...the SVG image isn't otherwise referred > > to, > > > it > > > > will be finalized and m_image is cleared.", and inferred that "finalized" > -> > > > > "~SVGImage called" -> SVGImageChromeClient also destroyed -> |this| > pointing > > > to > > > > freed memory. > > > > > > You're right, it is vulnerable to that. Addressing. > > > > Done; i.e., protect the SVGImage (and by implication, "ourselves") for the > > duration of this timer callback. > > LGTM, thanks. ditto :)
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/891263003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=189338 |
