Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1610883002:
Oilpan: ImageObserver needs to be a GC mixin. (Closed)
|
Created:
4 years, 11 months ago by sof Modified:
4 years, 10 months ago CC:
chromium-reviews, blink-reviews-platform-graphics_chromium.org, dshwang, fs, kinuko+watch, kouhei+svg_chromium.org, rwlbuis, Yoav Weiss, krit, drott+blinkwatch_chromium.org, Justin Novosad, Rik, gavinp+loader_chromium.org, blink-reviews, gyuyoung2, pdr+svgwatchlist_chromium.org, vmpstr+blinkwatch_chromium.org, jbroman, pdr+graphicswatchlist_chromium.org, loading-reviews+fetch_chromium.org, Nate Chapin, Stephen Chennney, tyoshino+watch_chromium.org, f(malita), danakj+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOilpan: ImageObserver needs to be a GC mixin.
R=haraken
BUG=
Committed: https://crrev.com/bd807b3a29d4021b98b8b3cafc06eeeae8789ded
Cr-Commit-Position: refs/heads/master@{#374896}
Patch Set 1 #Patch Set 2 : unit test compile fix #
Total comments: 2
Patch Set 3 : remove clearImage() use in dtor #Patch Set 4 : keep an UntracedMember<> instead #Patch Set 5 : move up null check #Patch Set 6 : git cl try #
Total comments: 3
Messages
Total messages: 37 (14 generated)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1610883002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1610883002/1
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look. Going after this ASan crasher https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil... Assuming I'm not too tired and missing something obvious right now, but the bare ImageObserver* pointer on Image is not safe, as its derived class (ImageResource) is on the heap. Something the GC plugin is unable to catch.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1610883002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1610883002/20001
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1610883002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/1610883002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResource.cpp:97: clearImage(); Can we remove this in oilpan?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1610883002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/1610883002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResource.cpp:97: clearImage(); On 2016/01/20 23:24:27, haraken wrote: > > Can we remove this in oilpan? > Certainly can; well spotted. Removed, but leaving behind a comment.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1610883002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1610883002/40001
This is very much related to https://codereview.chromium.org/891263003 There might be a way to address it without introducing a WeakPersistent<>; looking into it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM On 2016/01/21 07:25:36, sof wrote: > This is very much related to https://codereview.chromium.org/891263003 > > There might be a way to address it without introducing a WeakPersistent<>; > looking into it. Or would it be an option to move the Image to the heap (and use a WeakMember)?
On 2016/01/21 09:56:42, haraken wrote: > LGTM > > On 2016/01/21 07:25:36, sof wrote: > > This is very much related to https://codereview.chromium.org/891263003 > > > > There might be a way to address it without introducing a WeakPersistent<>; > > looking into it. > > Or would it be an option to move the Image to the heap (and use a WeakMember)? that's the right direction overall - I invite the Google Oilpan team to take that on :)
Description was changed from ========== Oilpan: ImageObserver needs to be a GC mixin. R= BUG= ========== to ========== Oilpan: ImageObserver needs to be a GC mixin. R=haraken BUG= ==========
Time to return to this one & get it ready as some flaky crashers can be seen w/ ASan over SVGImages -- https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_ASA... I don't think we should use WeakPersistent<ImageObserver> back pointers over Image -- too many of them for my liking and the weakness isn't really needed. Instead use UntracedMember<> + set up the required Persistent<> protection while handling the SVGImageChromeClient timer callback.
haraken@: good to go?
https://codereview.chromium.org/1610883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp (right): https://codereview.chromium.org/1610883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:98: if (Heap::willObjectBeLazilySwept(m_image->imageObserver())) Can we avoid using Heap::willObjectBeLazilySwept by using a pre-finalizer?
https://codereview.chromium.org/1610883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp (right): https://codereview.chromium.org/1610883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:98: if (Heap::willObjectBeLazilySwept(m_image->imageObserver())) On 2016/02/11 13:25:19, haraken wrote: > > Can we avoid using Heap::willObjectBeLazilySwept by using a pre-finalizer? ImageLoader already is, so it gets complex if ImageResource should be also. And there'll be quite a few of them.
https://codereview.chromium.org/1610883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp (right): https://codereview.chromium.org/1610883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:98: if (Heap::willObjectBeLazilySwept(m_image->imageObserver())) On 2016/02/11 13:27:41, sof wrote: > On 2016/02/11 13:25:19, haraken wrote: > > > > Can we avoid using Heap::willObjectBeLazilySwept by using a pre-finalizer? > > ImageLoader already is, so it gets complex if ImageResource should be also. And > there'll be quite a few of them. Would it be possible to make SVGImageChromeClient (or Image?) hold a WeakPersistent to the ImageObserver?
On 2016/02/11 13:49:40, haraken wrote: > https://codereview.chromium.org/1610883002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp > (right): > > https://codereview.chromium.org/1610883002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:98: if > (Heap::willObjectBeLazilySwept(m_image->imageObserver())) > On 2016/02/11 13:27:41, sof wrote: > > On 2016/02/11 13:25:19, haraken wrote: > > > > > > Can we avoid using Heap::willObjectBeLazilySwept by using a pre-finalizer? > > > > ImageLoader already is, so it gets complex if ImageResource should be also. > And > > there'll be quite a few of them. > > Would it be possible to make SVGImageChromeClient (or Image?) hold a > WeakPersistent to the ImageObserver? That's what <= ps#3 did (on Image), see #20 why I've switched away. Duplicating the ImageResource reference on another object runs the risk of getting unsync.
On 2016/02/11 13:51:43, sof wrote: > On 2016/02/11 13:49:40, haraken wrote: > > > https://codereview.chromium.org/1610883002/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp > > (right): > > > > > https://codereview.chromium.org/1610883002/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:98: if > > (Heap::willObjectBeLazilySwept(m_image->imageObserver())) > > On 2016/02/11 13:27:41, sof wrote: > > > On 2016/02/11 13:25:19, haraken wrote: > > > > > > > > Can we avoid using Heap::willObjectBeLazilySwept by using a pre-finalizer? > > > > > > ImageLoader already is, so it gets complex if ImageResource should be also. > > And > > > there'll be quite a few of them. > > > > Would it be possible to make SVGImageChromeClient (or Image?) hold a > > WeakPersistent to the ImageObserver? > > That's what <= ps#3 did (on Image), see #20 why I've switched away. > > Duplicating the ImageResource reference on another object runs the risk of > getting unsync. OK... LGTM. I hope we can move Image to the heap in a follow up.
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/1610883002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1610883002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. Thanks for your patience. Transient error: No JSON object could be decoded
The CQ bit was unchecked by commit-bot@chromium.org
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/1610883002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1610883002/100001
Message was sent while issue was closed.
Description was changed from ========== Oilpan: ImageObserver needs to be a GC mixin. R=haraken BUG= ========== to ========== Oilpan: ImageObserver needs to be a GC mixin. R=haraken BUG= ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Oilpan: ImageObserver needs to be a GC mixin. R=haraken BUG= ========== to ========== Oilpan: ImageObserver needs to be a GC mixin. R=haraken BUG= Committed: https://crrev.com/bd807b3a29d4021b98b8b3cafc06eeeae8789ded Cr-Commit-Position: refs/heads/master@{#374896} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/bd807b3a29d4021b98b8b3cafc06eeeae8789ded Cr-Commit-Position: refs/heads/master@{#374896} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
