Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
185613003:
Oilpan: Delay frameDetached() so it won't dispatch event inside ~SVGImage (Closed)
|
Created:
6 years, 9 months ago by kouhei (in TOK) Modified:
6 years, 9 months ago Reviewers:
Vyacheslav Egorov (Chromium), Mads Ager (chromium), zerny-chromium, eseidel, oilpan-reviews, haraken, pdr. CC:
blink-reviews, krit, fs, ed+blinkwatch_opera.com, f(malita), gyuyoung.kim_webkit.org, Stephen Chennney, pdr., rwlbuis, abarth-chromium, dcheng Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionOilpan: Delay frameDetached() so it won't dispatch event inside ~SVGImage
This CL moves frameDetached() call outside of ~SVGImage. Oilpan disallows any heap-allocated object creation inside destructors. SVGImageChromeClient and Page to be deleted are queued to DelayedSVGImageDestructor singleton, and oneshot timer is invoked to call |frameDetached| outside of destructor scope.
TESTS=svg/ (w/ enable_oilpan=1)
BUG=None
NOTRY=true
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168612
Patch Set 1 #
Total comments: 2
Patch Set 2 : add comments #
Total comments: 1
Messages
Total messages: 35 (0 generated)
PTAL. This CL solves SVGImage issue, however this patch is quite a workaround...
More context: - haraken's patch made Event GarbageCollected. - Event removes the last ref to Document - ~Document removes the last ref to ResourceFetcher - ~ImageResource removes the last ref to SVGImage - SVGImage invokes |frameDetached()|, which create (onclose?) Event inside.
On 2014/03/03 03:40:38, kouhei wrote: > More context: > > - haraken's patch made Event GarbageCollected. > - Event removes the last ref to Document > - ~Document removes the last ref to ResourceFetcher > - ~ImageResource removes the last ref to SVGImage > - SVGImage invokes |frameDetached()|, which create (onclose?) Event inside. Hi Kouhei, On the oilpan branch we had a related issue with frameDetached when destructing an SVGImage. Our fix [1] was to not notify cached resource clients (and thus avoid allocations in their callbacks) when the frame is anyway being torn down. Doing so also aligns better with the notification behaviour of other browsers. [1] https://codereview.chromium.org/42523003/
> Hi Kouhei, > > On the oilpan branch we had a related issue with frameDetached when destructing > an SVGImage. Our fix [1] was to not notify cached resource clients (and thus > avoid allocations in their callbacks) when the frame is anyway being torn down. > Doing so also aligns better with the notification behaviour of other browsers. > > [1] https://codereview.chromium.org/42523003/ Thanks! I'll try porting that approach.
On 2014/03/03 07:32:39, kouhei wrote: > > Hi Kouhei, > > > > On the oilpan branch we had a related issue with frameDetached when > destructing > > an SVGImage. Our fix [1] was to not notify cached resource clients (and thus > > avoid allocations in their callbacks) when the frame is anyway being torn > down. > > Doing so also aligns better with the notification behaviour of other browsers. > > > > [1] https://codereview.chromium.org/42523003/ > > Thanks! I'll try porting that approach. I just tried porting the approach, but looks like it isn't trivial as ResourceLoader has changed a lot from morrita@'s refactoring. I'll continue this work tomorrow.
Out of the several options in your document, this change seems to be our best option at the moment. SVGImage is already a special case and this fix is very local to SVGImage. Instead of creating a singleton with a queue of clean-up tasks, could we instead just off-heap allocate a clean-up callback per destruction event that fires the frameDetached call? Might simplify the code a bit and it would be nice to keep the use of OwnPtr to do the actual deleting.
> Out of the several options in your document, this change seems to be our best > option at the moment. SVGImage is already a special case and this fix is very > local to SVGImage. Yeah, I agree that this will fix our current issue at the moment, but my gut feeling is that I want to understand this issue more in general. In other words, I'd like to understand what's the best work-around for allocating objects during destructors. If we have no general work-around, it means that we always have a risk to introduce the bug and ship it without noticing it in our tests (We need to keep in mind that even an edit that looks completely unrelated to destructor call paths can trigger the bug indirectly).
On 2014/03/04 12:16:17, haraken wrote: > > Out of the several options in your document, this change seems to be our best > > option at the moment. SVGImage is already a special case and this fix is very > > local to SVGImage. > > Yeah, I agree that this will fix our current issue at the moment, but my gut > feeling is that I want to understand this issue more in general. In other words, > I'd like to understand what's the best work-around for allocating objects during > destructors. If we have no general work-around, it means that we always have a > risk to introduce the bug and ship it without noticing it in our tests (We need > to keep in mind that even an edit that looks completely unrelated to destructor > call paths can trigger the bug indirectly). Yes. But in this particular case, we are detaching an entire frame from within a destructor. In most cases the destruction of an object and its destruction code has much more local extent.
On 2014/03/04 12:04:36, zerny-chromium wrote: > Out of the several options in your document, this change seems to be our best > option at the moment. SVGImage is already a special case and this fix is very > local to SVGImage. Got it. > Instead of creating a singleton with a queue of clean-up tasks, could we instead > just off-heap allocate a clean-up callback per destruction event that fires the > frameDetached call? Might simplify the code a bit and it would be nice to keep > the use of OwnPtr to do the actual deleting. The problem is that Timer itself doesn't keep the callback alive, so we need something to hold onto those. In theory we can just leak the object and have "delete this" in the callback, but I think we should avoid it.
LGTM We might want to implement a DelayedDestructor class (which generalizes DelayedSVGImageDestructor) but we can do that when we find other objects whose destructors should be delayed as well. https://codereview.chromium.org/185613003/diff/1/Source/core/svg/graphics/SVG... File Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/185613003/diff/1/Source/core/svg/graphics/SVG... Source/core/svg/graphics/SVGImage.cpp:60: class DelayedSVGImageDestructor { Would you add a comment why you need this?
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kouhei@chromium.org/185613003/20001
https://codereview.chromium.org/185613003/diff/1/Source/core/svg/graphics/SVG... File Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/185613003/diff/1/Source/core/svg/graphics/SVG... Source/core/svg/graphics/SVGImage.cpp:60: class DelayedSVGImageDestructor { On 2014/03/05 08:13:10, haraken wrote: > > Would you add a comment why you need this? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kouhei@chromium.org/185613003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kouhei@chromium.org/185613003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kouhei@chromium.org/185613003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel
On 2014/03/06 09:03:22, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: win_blink_rel This is a timeout. Forcing commit.
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kouhei@chromium.org/185613003/20001
Message was sent while issue was closed.
Change committed as 168612
Message was sent while issue was closed.
https://codereview.chromium.org/185613003/diff/20001/Source/core/svg/graphics... File Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/185613003/diff/20001/Source/core/svg/graphics... Source/core/svg/graphics/SVGImage.cpp:71: entry.chromeClient = chromeClient.leakPtr(); Why not just use an OwnPtr and then call .clear() later instead of this manual delete?
Message was sent while issue was closed.
I don't think this was a good change, or at least an incomplete change. It worries me that we're now just waiting until the next event loop to destruct this page object, and that we're looking to paper-over lifetime confusion with changes like https://codereview.chromium.org/191913004/
Message was sent while issue was closed.
Don't GC environments normally do this kind of thing by having a synchronous finalize which is separate from the actual dealloc? I'm far from a GC expert, but it seems like we may need to expose a more general teardown/finalize call on all of blink's objects to get around this problem in the general case? We would then ASSERT that nothing trivial happens during actual deallocation?
Message was sent while issue was closed.
Nothing non-trivial, rather. :)
I actually support Eric here. Given that we are facing some crashes in the wild we might want to revert and reconsider approach unless we are adamant that we got all the possible crashes covered. > Don't GC environments normally do this kind of thing by having a synchronous finalize which is separate from the actual dealloc? That was our initial approach, but with this kind of design leads to all kinds of nasty issues e.g. permitting allocation in the finalizer results in necessity to support nested GCs, permitting accessing other objects in finalizers leads to subtle lifetime issues or requires marking through objects that await finalization which negatively impacts overall GC performance (as multiple GCs are now requires to reclaim objects). Additional issues pile up because we don't fully control our language runtime and have to operate with hacks and band aids in everything that is related to multithreading. Overall we arrived to the conclusion that requiring trivial finalizers that are self-contained and can't allocate is beneficial for both performance and stability. It allows to catch bugs early. We even have special ASAN support tailored to that: it allows to catch violation of this "trivial finalization" rule easily. Somehow this seemed to work on the branch, but does not completely work on the trunk (probably the fact that we approach hierarchies from slightly different angle, plays its role here... on the branch we attacked Node itself very early). We should probably take a step back and evaluate if we should introduce a finalization queue (that is marked through to guarantee object lifetime) at least for some objects like SVGImage and what kind of architectural impact it will have on the overall system. Vyacheslav Egorov On Mon, Mar 10, 2014 at 10:55 PM, <eseidel@chromium.org> wrote: > Don't GC environments normally do this kind of thing by having a > synchronous > finalize which is separate from the actual dealloc? I'm far from a GC > expert, > but it seems like we may need to expose a more general teardown/finalize > call on > all of blink's objects to get around this problem in the general case? > > We would then ASSERT that nothing trivial happens during actual > deallocation? > > https://codereview.chromium.org/185613003/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
I agree with Eric. As I commented in #7, we'll need a more general way to handle object allocations inside destructors (nested GC, delayed finalizer etc). At the moment I'm OK with handling the issue with some hacks (e.g., delay destructors to the next event loop) until we find that a more general solution is necessary, but I bet that we'll find it in the near future. The same issue is already happening in ~HTMLMediaElement() and ~Event().
Message was sent while issue was closed.
I think we are all in agreement. At least for the time being we need to look into allowing allocation during finalization. We are encountering this issue repeatedly right now (I'm seeing other cases where the Document is destructed during finalization because the last RefPtr is in an oilpan managed object; the Document destructor does a lot of stuff which leads to allocations.) We'll start looking into that and we should revert these changes ASAP. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
