Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
923443004:
Oilpan: avoid access to to-be-swept ImageLoader object. (Closed)
DescriptionOilpan: avoid access to to-be-swept ImageLoader object.
If the ImageLoader update microtask holds onto an ImageLoader object
that's slated to be swept out, do not invoke its update method. Doing so
might in turn risk accessing some of its already swept&finalized
references.
Also tidy up WeakFactory field ordering for ImageLoader::Task.
R=haraken,kouhei
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190140
Patch Set 1 #
Total comments: 11
Patch Set 2 : Added explanatory comment #Messages
Total messages: 16 (5 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
Please take a look.
kouhei@chromium.org changed reviewers: + kouhei@chromium.org
https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... Source/core/loader/ImageLoader.cpp:83: , m_updateBehavior(updateBehavior) unintended change? https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... Source/core/loader/ImageLoader.cpp:93: return; Is this needed for all places where non-Oilpan object access Oilpan object via raw ptr? In particular, would this be needed for the below too?: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... Source/core/loader/ImageLoader.cpp:112: UpdateFromElementBehavior m_updateBehavior; Ditto.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... Source/core/loader/ImageLoader.cpp:99: void clearLoader() Another possible fix would be to call clearLoader() in a pre-finalizer of ImageLoader and thus make sure that m_loader is 0 before we start sweeping. What do you think?
https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... Source/core/loader/ImageLoader.cpp:93: return; On 2015/02/13 00:25:58, kouhei wrote: > Is this needed for all places where non-Oilpan object access Oilpan object via > raw ptr? > In particular, would this be needed for the below too?: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... In most cases, no. This hack is needed only when you cannot clear the raw pointer (for some reason) before you start sweeping. Unless you have a strong reason, you should try hard to clear the raw pointer before starting a sweep phase that collects the on-heap object.
https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... Source/core/loader/ImageLoader.cpp:83: , m_updateBehavior(updateBehavior) On 2015/02/13 00:25:58, kouhei wrote: > unintended change? It helps to be have the weak factory last, and keep that as a pattern through the codebase. Which is what I tidied here. https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... Source/core/loader/ImageLoader.cpp:93: return; On 2015/02/13 01:41:59, haraken wrote: > On 2015/02/13 00:25:58, kouhei wrote: > > Is this needed for all places where non-Oilpan object access Oilpan object via > > raw ptr? > > In particular, would this be needed for the below too?: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > In most cases, no. > > This hack is needed only when you cannot clear the raw pointer (for some reason) > before you start sweeping. Unless you have a strong reason, you should try hard > to clear the raw pointer before starting a sweep phase that collects the on-heap > object. I will go through the Microtasks and check if they're similarly vulnerable to this lazy-sweep condition. https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... Source/core/loader/ImageLoader.cpp:99: void clearLoader() On 2015/02/13 01:38:53, haraken wrote: > > Another possible fix would be to call clearLoader() in a pre-finalizer of > ImageLoader and thus make sure that m_loader is 0 before we start sweeping. What > do you think? Wouldn't that lead to more unavoidable GC work, work that almost always could just as well be done in the finalizer? These task objects aren't entirely uncommon. This implements a weak reference from the task, in a way. An alternative is to have that be strong (Persistent), but I'm not sure I can convince myself that will hold on to objects for too long/longer than needed. Thoughts?
I'm ok with both approaches (PS1 or haraken's). Delegating to haraken@ https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... Source/core/loader/ImageLoader.cpp:83: , m_updateBehavior(updateBehavior) On 2015/02/13 06:40:48, sof wrote: > On 2015/02/13 00:25:58, kouhei wrote: > > unintended change? > > It helps to be have the weak factory last, and keep that as a pattern through > the codebase. Which is what I tidied here. sgtm. It might be helpful to mention about this change in CL description.
LGTM https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... Source/core/loader/ImageLoader.cpp:99: void clearLoader() On 2015/02/13 06:40:48, sof wrote: > On 2015/02/13 01:38:53, haraken wrote: > > > > Another possible fix would be to call clearLoader() in a pre-finalizer of > > ImageLoader and thus make sure that m_loader is 0 before we start sweeping. > What > > do you think? > > Wouldn't that lead to more unavoidable GC work, work that almost always could > just as well be done in the finalizer? These task objects aren't entirely > uncommon. > > This implements a weak reference from the task, in a way. An alternative is to > have that be strong (Persistent), but I'm not sure I can convince myself that > will hold on to objects for too long/longer than needed. Thoughts? Whether we should use Heap::willObjectBeLazilySwept or prefinalizer depends on how many objects are created. If it's too many, we should avoid using prefinalizers (and use Heap::willObjectBeLazilySwept). Timers are in this category. Otherwise, I'd prefer prefinalizers and explicit pointer clear. If ImageLoader is created too many, I'm fine with Heap::willObjectBeLazilySwept.
https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... File Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... Source/core/loader/ImageLoader.cpp:99: void clearLoader() On 2015/02/13 09:20:20, haraken wrote: > On 2015/02/13 06:40:48, sof wrote: > > On 2015/02/13 01:38:53, haraken wrote: > > > > > > Another possible fix would be to call clearLoader() in a pre-finalizer of > > > ImageLoader and thus make sure that m_loader is 0 before we start sweeping. > > What > > > do you think? > > > > Wouldn't that lead to more unavoidable GC work, work that almost always could > > just as well be done in the finalizer? These task objects aren't entirely > > uncommon. > > > > This implements a weak reference from the task, in a way. An alternative is to > > have that be strong (Persistent), but I'm not sure I can convince myself that > > will hold on to objects for too long/longer than needed. Thoughts? > > Whether we should use Heap::willObjectBeLazilySwept or prefinalizer depends on > how many objects are created. If it's too many, we should avoid using > prefinalizers (and use Heap::willObjectBeLazilySwept). Timers are in this > category. Otherwise, I'd prefer prefinalizers and explicit pointer clear. > > If ImageLoader is created too many, I'm fine with Heap::willObjectBeLazilySwept. When time, I need to look into what I thought were surprisingly many ImageLoaders that were kept alive for dom-modify.html (which is the repro'ing test for this one, along with external allocations triggering GCs.) We do have additional lifetime handling in place for ImageLoader with Oilpan.
New patchsets have been uploaded after l-g-t-m from haraken@chromium.org
On 2015/02/13 09:20:20, haraken wrote: > LGTM > > https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... > File Source/core/loader/ImageLoader.cpp (right): > > https://codereview.chromium.org/923443004/diff/1/Source/core/loader/ImageLoad... > Source/core/loader/ImageLoader.cpp:99: void clearLoader() > On 2015/02/13 06:40:48, sof wrote: > > On 2015/02/13 01:38:53, haraken wrote: > > > > > > Another possible fix would be to call clearLoader() in a pre-finalizer of > > > ImageLoader and thus make sure that m_loader is 0 before we start sweeping. > > What > > > do you think? > > > > Wouldn't that lead to more unavoidable GC work, work that almost always could > > just as well be done in the finalizer? These task objects aren't entirely > > uncommon. > > > > This implements a weak reference from the task, in a way. An alternative is to > > have that be strong (Persistent), but I'm not sure I can convince myself that > > will hold on to objects for too long/longer than needed. Thoughts? > > Whether we should use Heap::willObjectBeLazilySwept or prefinalizer depends on > how many objects are created. If it's too many, we should avoid using > prefinalizers (and use Heap::willObjectBeLazilySwept). Timers are in this > category. Otherwise, I'd prefer prefinalizers and explicit pointer clear. > > If ImageLoader is created too many, I'm fine with Heap::willObjectBeLazilySwept. Going with the explicit will*Swept() check; added a comment why this kind of custom handling is needed. (Verified that this doesn't apply to other Microtask uses.)
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/923443004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190140 |
