Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1159583002:
Eagerly sweep MediaKeySession objects. (Closed)
|
Created:
5 years, 7 months ago by sof Modified:
5 years, 6 months ago CC:
blink-reviews, feature-media-reviews_chromium.org, philipj_slow, eric.carlson_apple.com Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionEagerly sweep MediaKeySession objects.
MediaKeySession owns an instance of the WebContentDecryptionModuleSession
bridge object, which allows for bidirectional access with the
embedder-side object.
With Oilpan GC's lazy sweeping in effect, we need to tear down that
bridge as soon as the MediaKeySession object has been deemed to be garbage,
and slated for sweeping. Otherwise the embedder may attempt to access the
MediaKeySession's client interface methods while MediaKeySession is waiting
to be lazily swept, and potentially touching other heap objects that
MediaKeySession refers to which might have been independently swept
and finalized.
That is, the GC finalization order isn't fixed and deterministic. With
lazy sweeping, that lack of order is observable not just to the objects
that are on the heap, but also external, non-heap objects. Like the
embedder side object in this case.
Arrange for that eager finalization to happen by annotating MediaKeySession
as eagerly swept.
R=haraken
BUG=491488
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196480
Patch Set 1 #
Total comments: 2
Patch Set 2 : Updated to use EAGER_FINALIZE() #
Total comments: 2
Patch Set 3 : improve comment #
Messages
Total messages: 24 (4 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look.
Forgot to include.. encryptedmedia/ has been audited for lazy sweeping issues; this is the only one found. (Added a clarifying comment + tidied header files while doing so, also.)
haraken@chromium.org changed reviewers: + haraken@chromium.org
Thanks for looking into this! It seems that the underlying issue is that the embedder side is holding a raw pointer to Blink... (i.e., WebContentDecryptionModuleSessionImpl::client_) I might prefer using a pre-finalizer instead of eager sweeping for the following reasons: - We don't want to have two mechanisms that do similar things in the code base. - I'd prefer a pre-finalier than eager sweeping, because the semantics of the pre-finalizer is clearer -- i.e., the pre-finalizer is a function that is always called before the object gets destructed. On the other hand, you need to understand GC internals to understand what eager sweeping is. One concern about the pre-finalizer is that it cannot be nested. I understand the concern but we can easily detect nested pre-finalizers because we have a NoAllocationScope before calling pre-finalizers. Also it would be easy to support nested pre-finalizers if needed -- we can just forbid GCs while calling pre-finalizers. What do you think? https://codereview.chromium.org/1159583002/diff/1/Source/modules/encryptedmed... File Source/modules/encryptedmedia/MediaKeysController.h (right): https://codereview.chromium.org/1159583002/diff/1/Source/modules/encryptedmed... Source/modules/encryptedmedia/MediaKeysController.h:33: // It is not on the Oilpan heap. Just help me understand: - Is this related to the MediaKeySession issue somehow? - MediaKeysController and Page die at the same time. Page does not outlive the WebViewImpl. Thus this the m_client cannot be a stale pointer. Is my understanding correct?
On 2015/05/23 23:58:37, haraken wrote: > Thanks for looking into this! > It won't fix itself! > It seems that the underlying issue is that the embedder side is holding a raw > pointer to Blink... (i.e., WebContentDecryptionModuleSessionImpl::client_) > > I might prefer using a pre-finalizer instead of eager sweeping for the following > reasons: > > - We don't want to have two mechanisms that do similar things in the code base. > Right, we don't want destructors and dispose(). With the latter additionally inlining the destructors of certain fields, something the destructor implicitly does (e.g., clear the OwnPtr fields.) You have to consider the end-user, other Blink developers, here also. > - I'd prefer a pre-finalier than eager sweeping, because the semantics of the > pre-finalizer is clearer -- i.e., the pre-finalizer is a function that is always > called before the object gets destructed. On the other hand, you need to > understand GC internals to understand what eager sweeping is. > The name of the macro doesn't offer additional help in understanding, I agree ;) To even understand the need to finesse the timing of finalization requires some understanding of the Oilpan GC, I think. > One concern about the pre-finalizer is that it cannot be nested. I understand > the concern but we can easily detect nested pre-finalizers because we have a > NoAllocationScope before calling pre-finalizers. Also it would be easy to > support nested pre-finalizers if needed -- we can just forbid GCs while calling > pre-finalizers. > An obscure assert will trigger on initial registration atm. Will you guarantee that the nested pre-finalizers are run in the same order as the destructors for an object of type T? > What do you think? > I think it is worth considering not using lazy sweeping. This hidden & subtle detail surrounding finalization is not pleasant for anyone to reason about, lacking a static method for identifying the problem esp. Hoping that ASan uncovers the problem at some point isn't satisfactory to me. But, if we assume it is an integral part of what the GC provides, we should aim to minimize the "Oilpan programming tax" for developers. You may consider it otherwise, but an annotation to capture the common case of running the destructor early/ier gets us closer to that goal. Should you _additionally_ require the ability to access other heap objects while finalizing, manually put that finalization code in a dispose() and use a prefinalizer. This feels like a re-run of a previous discussion/exchange.
https://codereview.chromium.org/1159583002/diff/1/Source/modules/encryptedmed... File Source/modules/encryptedmedia/MediaKeysController.h (right): https://codereview.chromium.org/1159583002/diff/1/Source/modules/encryptedmed... Source/modules/encryptedmedia/MediaKeysController.h:33: // It is not on the Oilpan heap. On 2015/05/23 23:58:37, haraken wrote: > > Just help me understand: > > - Is this related to the MediaKeySession issue somehow? > > - MediaKeysController and Page die at the same time. Page does not outlive the > WebViewImpl. Thus this the m_client cannot be a stale pointer. Is my > understanding correct? That matches mine, essentially. The last part is the key one, the non-heap WebViewImpl lives longest, hence raw pointers to it from the heap "will work out".
> But, if we assume it is an integral part of what the GC provides, we should aim > to minimize the "Oilpan programming tax" for developers. You may consider it > otherwise, but an annotation to capture the common case of running the > destructor early/ier gets us closer to that goal. Should you _additionally_ > require the ability to access other heap objects while finalizing, manually put > that finalization code in a dispose() and use a prefinalizer. Totally agreed on the point -- we should minimize the Oilpan programming tax. From that perspective, it is important not to mix pre-finalizers and eager sweeping. In other words, I want to adopt a rule either of: (a) "Use pre-finalizers for destructor-timing problems; you may need to use eager sweeping in some rare scenarios though"; or (b) "Use eager sweeping for destructor-timing problems; you may need to use pre-finalizers in some rare scenarios though" Given that pre-finalizers are "stronger" than eager sweeping (i.e., pre-finalizers can access on-heap objects), (a) will work but (b) won't work. This is the reason I'm leaning toward (a). Another rule (which I think you want to adopt) would be "Use pre-finalizers only if you need to touch other on-heap objects; otherwise use eager sweeping". This rule would fine in a sense that unnecessary abilities are not exposed to the code base, but it imposes a burden on developers to reason about the distincition between pre-finalizers and eager sweepings. I don't see much benefit of restricting the abilities here, so I guess it wouldn't be worth having developers reason about the distinction. > > It seems that the underlying issue is that the embedder side is holding a raw > > pointer to Blink... (i.e., WebContentDecryptionModuleSessionImpl::client_) > > > > I might prefer using a pre-finalizer instead of eager sweeping for the > following > > reasons: > > > > One concern about the pre-finalizer is that it cannot be nested. I understand > > the concern but we can easily detect nested pre-finalizers because we have a > > NoAllocationScope before calling pre-finalizers. Also it would be easy to > > support nested pre-finalizers if needed -- we can just forbid GCs while > calling > > pre-finalizers. > > > > An obscure assert will trigger on initial registration atm. Will you guarantee > that the nested pre-finalizers are run in the same order as the destructors for > an object of type T? My hope is that we don't need to support nested pre-finalizers :) If I'm understanding things correctly, nested pre-finalizers can happen only when a GC is triggered in a nested manner. So an easy fix would be just to forbid a GC while invoking pre-finalizers. However, my hope is that we can just leave the NoAllocationScope around the pre-finalizers and guarantee that no allocation happens (and thus no GC happens) while invoking the pre-finalizers.
On 2015/05/24 09:59:00, haraken wrote: > > But, if we assume it is an integral part of what the GC provides, we should > aim > > to minimize the "Oilpan programming tax" for developers. You may consider it > > otherwise, but an annotation to capture the common case of running the > > destructor early/ier gets us closer to that goal. Should you _additionally_ > > require the ability to access other heap objects while finalizing, manually > put > > that finalization code in a dispose() and use a prefinalizer. > > Totally agreed on the point -- we should minimize the Oilpan programming tax. > > From that perspective, it is important not to mix pre-finalizers and eager > sweeping. In other words, I want to adopt a rule either of: > > (a) "Use pre-finalizers for destructor-timing problems; you may need to use > eager sweeping in some rare scenarios though"; or > (b) "Use eager sweeping for destructor-timing problems; you may need to use > pre-finalizers in some rare scenarios though" > > Given that pre-finalizers are "stronger" than eager sweeping (i.e., > pre-finalizers can access on-heap objects), (a) will work but (b) won't work. > This is the reason I'm leaning toward (a). > > Another rule (which I think you want to adopt) would be "Use pre-finalizers only > if you need to touch other on-heap objects; otherwise use eager sweeping". This > rule would fine in a sense that unnecessary abilities are not exposed to the > code base, but it imposes a burden on developers to reason about the > distincition between pre-finalizers and eager sweepings. I don't see much > benefit of restricting the abilities here, so I guess it wouldn't be worth > having developers reason about the distinction. > > > > > It seems that the underlying issue is that the embedder side is holding a > raw > > > pointer to Blink... (i.e., WebContentDecryptionModuleSessionImpl::client_) > > > > > > I might prefer using a pre-finalizer instead of eager sweeping for the > > following > > > reasons: > > > > > > One concern about the pre-finalizer is that it cannot be nested. I > understand > > > the concern but we can easily detect nested pre-finalizers because we have a > > > NoAllocationScope before calling pre-finalizers. Also it would be easy to > > > support nested pre-finalizers if needed -- we can just forbid GCs while > > calling > > > pre-finalizers. > > > > > > > An obscure assert will trigger on initial registration atm. Will you guarantee > > that the nested pre-finalizers are run in the same order as the destructors > for > > an object of type T? > > My hope is that we don't need to support nested pre-finalizers :) > > If I'm understanding things correctly, nested pre-finalizers can happen only > when a GC is triggered in a nested manner. So an easy fix would be just to > forbid a GC while invoking pre-finalizers. However, my hope is that we can just > leave the NoAllocationScope around the pre-finalizers and guarantee that no > allocation happens (and thus no GC happens) while invoking the pre-finalizers. Another idea might be just to replace all pre-finalizers with eager sweepings. This needs an assumption that the pre-finalizers don't touch any object on EagerSweepHeap. I guess the assumption would be holding currently, but that is a nasty assumption.
On 2015/05/24 10:05:13, haraken wrote: > On 2015/05/24 09:59:00, haraken wrote: > > > But, if we assume it is an integral part of what the GC provides, we should > > aim > > > to minimize the "Oilpan programming tax" for developers. You may consider it > > > otherwise, but an annotation to capture the common case of running the > > > destructor early/ier gets us closer to that goal. Should you _additionally_ > > > require the ability to access other heap objects while finalizing, manually > > put > > > that finalization code in a dispose() and use a prefinalizer. > > > > Totally agreed on the point -- we should minimize the Oilpan programming tax. > > > > From that perspective, it is important not to mix pre-finalizers and eager > > sweeping. In other words, I want to adopt a rule either of: > > > > (a) "Use pre-finalizers for destructor-timing problems; you may need to use > > eager sweeping in some rare scenarios though"; or > > (b) "Use eager sweeping for destructor-timing problems; you may need to use > > pre-finalizers in some rare scenarios though" > > > > Given that pre-finalizers are "stronger" than eager sweeping (i.e., > > pre-finalizers can access on-heap objects), (a) will work but (b) won't work. > > This is the reason I'm leaning toward (a). > > > > Another rule (which I think you want to adopt) would be "Use pre-finalizers > only > > if you need to touch other on-heap objects; otherwise use eager sweeping". > This > > rule would fine in a sense that unnecessary abilities are not exposed to the > > code base, but it imposes a burden on developers to reason about the > > distincition between pre-finalizers and eager sweepings. I don't see much > > benefit of restricting the abilities here, so I guess it wouldn't be worth > > having developers reason about the distinction. > > > > > > > > It seems that the underlying issue is that the embedder side is holding a > > raw > > > > pointer to Blink... (i.e., > WebContentDecryptionModuleSessionImpl::client_) > > > > > > > > I might prefer using a pre-finalizer instead of eager sweeping for the > > > following > > > > reasons: > > > > > > > > One concern about the pre-finalizer is that it cannot be nested. I > > understand > > > > the concern but we can easily detect nested pre-finalizers because we have > a > > > > NoAllocationScope before calling pre-finalizers. Also it would be easy to > > > > support nested pre-finalizers if needed -- we can just forbid GCs while > > > calling > > > > pre-finalizers. > > > > > > > > > > An obscure assert will trigger on initial registration atm. Will you > guarantee > > > that the nested pre-finalizers are run in the same order as the destructors > > for > > > an object of type T? > > > > My hope is that we don't need to support nested pre-finalizers :) > > > > If I'm understanding things correctly, nested pre-finalizers can happen only > > when a GC is triggered in a nested manner. So an easy fix would be just to > > forbid a GC while invoking pre-finalizers. However, my hope is that we can > just > > leave the NoAllocationScope around the pre-finalizers and guarantee that no > > allocation happens (and thus no GC happens) while invoking the pre-finalizers. > > Another idea might be just to replace all pre-finalizers with eager sweepings. > This needs an assumption that the pre-finalizers don't touch any object on > EagerSweepHeap. I guess the assumption would be holding currently, but that is a > nasty assumption. That is a really interesting idea. If the eagerly swept heap (only) has its unmarked objects poisoned before it is swept, then you get a hybrid -- finalizers being allowed to touch other objects, but not other eager objects. ASan should be effective in catching violations of that. I have to think a bit longer about the implications of that model.. As regards prefinalizers, I do hope new Blink code will be designed such that they have no use there.
On 2015/05/24 11:44:36, sof wrote: > On 2015/05/24 10:05:13, haraken wrote: > > On 2015/05/24 09:59:00, haraken wrote: > > > > But, if we assume it is an integral part of what the GC provides, we > should > > > aim > > > > to minimize the "Oilpan programming tax" for developers. You may consider > it > > > > otherwise, but an annotation to capture the common case of running the > > > > destructor early/ier gets us closer to that goal. Should you > _additionally_ > > > > require the ability to access other heap objects while finalizing, > manually > > > put > > > > that finalization code in a dispose() and use a prefinalizer. > > > > > > Totally agreed on the point -- we should minimize the Oilpan programming > tax. > > > > > > From that perspective, it is important not to mix pre-finalizers and eager > > > sweeping. In other words, I want to adopt a rule either of: > > > > > > (a) "Use pre-finalizers for destructor-timing problems; you may need to use > > > eager sweeping in some rare scenarios though"; or > > > (b) "Use eager sweeping for destructor-timing problems; you may need to use > > > pre-finalizers in some rare scenarios though" > > > > > > Given that pre-finalizers are "stronger" than eager sweeping (i.e., > > > pre-finalizers can access on-heap objects), (a) will work but (b) won't > work. > > > This is the reason I'm leaning toward (a). > > > > > > Another rule (which I think you want to adopt) would be "Use pre-finalizers > > only > > > if you need to touch other on-heap objects; otherwise use eager sweeping". > > This > > > rule would fine in a sense that unnecessary abilities are not exposed to the > > > code base, but it imposes a burden on developers to reason about the > > > distincition between pre-finalizers and eager sweepings. I don't see much > > > benefit of restricting the abilities here, so I guess it wouldn't be worth > > > having developers reason about the distinction. > > > > > > > > > > > It seems that the underlying issue is that the embedder side is holding > a > > > raw > > > > > pointer to Blink... (i.e., > > WebContentDecryptionModuleSessionImpl::client_) > > > > > > > > > > I might prefer using a pre-finalizer instead of eager sweeping for the > > > > following > > > > > reasons: > > > > > > > > > > One concern about the pre-finalizer is that it cannot be nested. I > > > understand > > > > > the concern but we can easily detect nested pre-finalizers because we > have > > a > > > > > NoAllocationScope before calling pre-finalizers. Also it would be easy > to > > > > > support nested pre-finalizers if needed -- we can just forbid GCs while > > > > calling > > > > > pre-finalizers. > > > > > > > > > > > > > An obscure assert will trigger on initial registration atm. Will you > > guarantee > > > > that the nested pre-finalizers are run in the same order as the > destructors > > > for > > > > an object of type T? > > > > > > My hope is that we don't need to support nested pre-finalizers :) > > > > > > If I'm understanding things correctly, nested pre-finalizers can happen only > > > when a GC is triggered in a nested manner. So an easy fix would be just to > > > forbid a GC while invoking pre-finalizers. However, my hope is that we can > > just > > > leave the NoAllocationScope around the pre-finalizers and guarantee that no > > > allocation happens (and thus no GC happens) while invoking the > pre-finalizers. > > > > Another idea might be just to replace all pre-finalizers with eager sweepings. > > This needs an assumption that the pre-finalizers don't touch any object on > > EagerSweepHeap. I guess the assumption would be holding currently, but that is > a > > nasty assumption. > > That is a really interesting idea. If the eagerly swept heap (only) has its > unmarked objects poisoned before it is swept, then you get a hybrid -- > finalizers being allowed to touch other objects, but not other eager objects. > ASan should be effective in catching violations of that. I have to think a bit > longer about the implications of that model.. > > As regards prefinalizers, I do hope new Blink code will be designed such that > they have no use there. I think we should try to adopt that model, it simplifies. EAGERLY_FINALIZED() would be a more appropriate (and easier to understand) class-level annotation. The GC plugin needs to adjust its handling of destructors to accommodate & support such eager finalizers; https://codereview.chromium.org/1158623002/ does that.
On 2015/05/24 21:17:43, sof wrote: > On 2015/05/24 11:44:36, sof wrote: > > On 2015/05/24 10:05:13, haraken wrote: > > > On 2015/05/24 09:59:00, haraken wrote: > > > > > But, if we assume it is an integral part of what the GC provides, we > > should > > > > aim > > > > > to minimize the "Oilpan programming tax" for developers. You may > consider > > it > > > > > otherwise, but an annotation to capture the common case of running the > > > > > destructor early/ier gets us closer to that goal. Should you > > _additionally_ > > > > > require the ability to access other heap objects while finalizing, > > manually > > > > put > > > > > that finalization code in a dispose() and use a prefinalizer. > > > > > > > > Totally agreed on the point -- we should minimize the Oilpan programming > > tax. > > > > > > > > From that perspective, it is important not to mix pre-finalizers and eager > > > > sweeping. In other words, I want to adopt a rule either of: > > > > > > > > (a) "Use pre-finalizers for destructor-timing problems; you may need to > use > > > > eager sweeping in some rare scenarios though"; or > > > > (b) "Use eager sweeping for destructor-timing problems; you may need to > use > > > > pre-finalizers in some rare scenarios though" > > > > > > > > Given that pre-finalizers are "stronger" than eager sweeping (i.e., > > > > pre-finalizers can access on-heap objects), (a) will work but (b) won't > > work. > > > > This is the reason I'm leaning toward (a). > > > > > > > > Another rule (which I think you want to adopt) would be "Use > pre-finalizers > > > only > > > > if you need to touch other on-heap objects; otherwise use eager sweeping". > > > This > > > > rule would fine in a sense that unnecessary abilities are not exposed to > the > > > > code base, but it imposes a burden on developers to reason about the > > > > distincition between pre-finalizers and eager sweepings. I don't see much > > > > benefit of restricting the abilities here, so I guess it wouldn't be worth > > > > having developers reason about the distinction. > > > > > > > > > > > > > > It seems that the underlying issue is that the embedder side is > holding > > a > > > > raw > > > > > > pointer to Blink... (i.e., > > > WebContentDecryptionModuleSessionImpl::client_) > > > > > > > > > > > > I might prefer using a pre-finalizer instead of eager sweeping for the > > > > > following > > > > > > reasons: > > > > > > > > > > > > One concern about the pre-finalizer is that it cannot be nested. I > > > > understand > > > > > > the concern but we can easily detect nested pre-finalizers because we > > have > > > a > > > > > > NoAllocationScope before calling pre-finalizers. Also it would be easy > > to > > > > > > support nested pre-finalizers if needed -- we can just forbid GCs > while > > > > > calling > > > > > > pre-finalizers. > > > > > > > > > > > > > > > > An obscure assert will trigger on initial registration atm. Will you > > > guarantee > > > > > that the nested pre-finalizers are run in the same order as the > > destructors > > > > for > > > > > an object of type T? > > > > > > > > My hope is that we don't need to support nested pre-finalizers :) > > > > > > > > If I'm understanding things correctly, nested pre-finalizers can happen > only > > > > when a GC is triggered in a nested manner. So an easy fix would be just to > > > > forbid a GC while invoking pre-finalizers. However, my hope is that we can > > > just > > > > leave the NoAllocationScope around the pre-finalizers and guarantee that > no > > > > allocation happens (and thus no GC happens) while invoking the > > pre-finalizers. > > > > > > Another idea might be just to replace all pre-finalizers with eager > sweepings. > > > This needs an assumption that the pre-finalizers don't touch any object on > > > EagerSweepHeap. I guess the assumption would be holding currently, but that > is > > a > > > nasty assumption. > > > > That is a really interesting idea. If the eagerly swept heap (only) has its > > unmarked objects poisoned before it is swept, then you get a hybrid -- > > finalizers being allowed to touch other objects, but not other eager objects. > > ASan should be effective in catching violations of that. I have to think a bit > > longer about the implications of that model.. > > > > As regards prefinalizers, I do hope new Blink code will be designed such that > > they have no use there. > > I think we should try to adopt that model, it simplifies. EAGERLY_FINALIZED() > would be a more appropriate (and easier to understand) class-level annotation. Let's do that. I'm on the same page :) Just to clarify: - Touch a not-eagerly-swept object in a destructor of a not-eagerly-swept object => Invalid. Detected by the ASan poisoning. - Touch a not-eagerly-swept object in a destructor of an eagerly-swept object => Valid. - Touch an eagerly-swept object in a destructor of a not-eagerly-swept object => Invalid. Detected by another ASan poisoning on the free list. Also non-ASan build will crash. - Touch an eagerly-swept object in a destructor of an eagerly-swept object => Invalid. Detected by the ASan poisoning. I hope EAGERLY_FINALIZED() will replace all EAGER_SWEEP() and USE_PRE_FINALIZER().
On 2015/05/25 00:16:28, haraken wrote: > On 2015/05/24 21:17:43, sof wrote: > > On 2015/05/24 11:44:36, sof wrote: > > > On 2015/05/24 10:05:13, haraken wrote: > > > > On 2015/05/24 09:59:00, haraken wrote: > > > > > > But, if we assume it is an integral part of what the GC provides, we > > > should > > > > > aim > > > > > > to minimize the "Oilpan programming tax" for developers. You may > > consider > > > it > > > > > > otherwise, but an annotation to capture the common case of running the > > > > > > destructor early/ier gets us closer to that goal. Should you > > > _additionally_ > > > > > > require the ability to access other heap objects while finalizing, > > > manually > > > > > put > > > > > > that finalization code in a dispose() and use a prefinalizer. > > > > > > > > > > Totally agreed on the point -- we should minimize the Oilpan programming > > > tax. > > > > > > > > > > From that perspective, it is important not to mix pre-finalizers and > eager > > > > > sweeping. In other words, I want to adopt a rule either of: > > > > > > > > > > (a) "Use pre-finalizers for destructor-timing problems; you may need to > > use > > > > > eager sweeping in some rare scenarios though"; or > > > > > (b) "Use eager sweeping for destructor-timing problems; you may need to > > use > > > > > pre-finalizers in some rare scenarios though" > > > > > > > > > > Given that pre-finalizers are "stronger" than eager sweeping (i.e., > > > > > pre-finalizers can access on-heap objects), (a) will work but (b) won't > > > work. > > > > > This is the reason I'm leaning toward (a). > > > > > > > > > > Another rule (which I think you want to adopt) would be "Use > > pre-finalizers > > > > only > > > > > if you need to touch other on-heap objects; otherwise use eager > sweeping". > > > > This > > > > > rule would fine in a sense that unnecessary abilities are not exposed to > > the > > > > > code base, but it imposes a burden on developers to reason about the > > > > > distincition between pre-finalizers and eager sweepings. I don't see > much > > > > > benefit of restricting the abilities here, so I guess it wouldn't be > worth > > > > > having developers reason about the distinction. > > > > > > > > > > > > > > > > > It seems that the underlying issue is that the embedder side is > > holding > > > a > > > > > raw > > > > > > > pointer to Blink... (i.e., > > > > WebContentDecryptionModuleSessionImpl::client_) > > > > > > > > > > > > > > I might prefer using a pre-finalizer instead of eager sweeping for > the > > > > > > following > > > > > > > reasons: > > > > > > > > > > > > > > One concern about the pre-finalizer is that it cannot be nested. I > > > > > understand > > > > > > > the concern but we can easily detect nested pre-finalizers because > we > > > have > > > > a > > > > > > > NoAllocationScope before calling pre-finalizers. Also it would be > easy > > > to > > > > > > > support nested pre-finalizers if needed -- we can just forbid GCs > > while > > > > > > calling > > > > > > > pre-finalizers. > > > > > > > > > > > > > > > > > > > An obscure assert will trigger on initial registration atm. Will you > > > > guarantee > > > > > > that the nested pre-finalizers are run in the same order as the > > > destructors > > > > > for > > > > > > an object of type T? > > > > > > > > > > My hope is that we don't need to support nested pre-finalizers :) > > > > > > > > > > If I'm understanding things correctly, nested pre-finalizers can happen > > only > > > > > when a GC is triggered in a nested manner. So an easy fix would be just > to > > > > > forbid a GC while invoking pre-finalizers. However, my hope is that we > can > > > > just > > > > > leave the NoAllocationScope around the pre-finalizers and guarantee that > > no > > > > > allocation happens (and thus no GC happens) while invoking the > > > pre-finalizers. > > > > > > > > Another idea might be just to replace all pre-finalizers with eager > > sweepings. > > > > This needs an assumption that the pre-finalizers don't touch any object on > > > > EagerSweepHeap. I guess the assumption would be holding currently, but > that > > is > > > a > > > > nasty assumption. > > > > > > That is a really interesting idea. If the eagerly swept heap (only) has its > > > unmarked objects poisoned before it is swept, then you get a hybrid -- > > > finalizers being allowed to touch other objects, but not other eager > objects. > > > ASan should be effective in catching violations of that. I have to think a > bit > > > longer about the implications of that model.. > > > > > > As regards prefinalizers, I do hope new Blink code will be designed such > that > > > they have no use there. > > > > I think we should try to adopt that model, it simplifies. EAGERLY_FINALIZED() > > would be a more appropriate (and easier to understand) class-level annotation. > > Let's do that. I'm on the same page :) > > Just to clarify: > > - Touch a not-eagerly-swept object in a destructor of a not-eagerly-swept object > => Invalid. Detected by the ASan poisoning. > - Touch a not-eagerly-swept object in a destructor of an eagerly-swept object => > Valid. > - Touch an eagerly-swept object in a destructor of a not-eagerly-swept object => > Invalid. Detected by another ASan poisoning on the free list. Also non-ASan > build will crash. > - Touch an eagerly-swept object in a destructor of an eagerly-swept object => > Invalid. Detected by the ASan poisoning. > Those will all hold, but to strengthen ASan checking, do you want to poison the entire eagerly swept heap pre-sweep and unpoison what's alive afterwards? > I hope EAGERLY_FINALIZED() will replace all EAGER_SWEEP() and > USE_PRE_FINALIZER(). Let's see where it goes.
On 2015/05/25 06:32:43, sof wrote: > On 2015/05/25 00:16:28, haraken wrote: > > On 2015/05/24 21:17:43, sof wrote: > > > On 2015/05/24 11:44:36, sof wrote: > > > > On 2015/05/24 10:05:13, haraken wrote: > > > > > On 2015/05/24 09:59:00, haraken wrote: > > > > > > > But, if we assume it is an integral part of what the GC provides, we > > > > should > > > > > > aim > > > > > > > to minimize the "Oilpan programming tax" for developers. You may > > > consider > > > > it > > > > > > > otherwise, but an annotation to capture the common case of running > the > > > > > > > destructor early/ier gets us closer to that goal. Should you > > > > _additionally_ > > > > > > > require the ability to access other heap objects while finalizing, > > > > manually > > > > > > put > > > > > > > that finalization code in a dispose() and use a prefinalizer. > > > > > > > > > > > > Totally agreed on the point -- we should minimize the Oilpan > programming > > > > tax. > > > > > > > > > > > > From that perspective, it is important not to mix pre-finalizers and > > eager > > > > > > sweeping. In other words, I want to adopt a rule either of: > > > > > > > > > > > > (a) "Use pre-finalizers for destructor-timing problems; you may need > to > > > use > > > > > > eager sweeping in some rare scenarios though"; or > > > > > > (b) "Use eager sweeping for destructor-timing problems; you may need > to > > > use > > > > > > pre-finalizers in some rare scenarios though" > > > > > > > > > > > > Given that pre-finalizers are "stronger" than eager sweeping (i.e., > > > > > > pre-finalizers can access on-heap objects), (a) will work but (b) > won't > > > > work. > > > > > > This is the reason I'm leaning toward (a). > > > > > > > > > > > > Another rule (which I think you want to adopt) would be "Use > > > pre-finalizers > > > > > only > > > > > > if you need to touch other on-heap objects; otherwise use eager > > sweeping". > > > > > This > > > > > > rule would fine in a sense that unnecessary abilities are not exposed > to > > > the > > > > > > code base, but it imposes a burden on developers to reason about the > > > > > > distincition between pre-finalizers and eager sweepings. I don't see > > much > > > > > > benefit of restricting the abilities here, so I guess it wouldn't be > > worth > > > > > > having developers reason about the distinction. > > > > > > > > > > > > > > > > > > > > It seems that the underlying issue is that the embedder side is > > > holding > > > > a > > > > > > raw > > > > > > > > pointer to Blink... (i.e., > > > > > WebContentDecryptionModuleSessionImpl::client_) > > > > > > > > > > > > > > > > I might prefer using a pre-finalizer instead of eager sweeping for > > the > > > > > > > following > > > > > > > > reasons: > > > > > > > > > > > > > > > > One concern about the pre-finalizer is that it cannot be nested. I > > > > > > understand > > > > > > > > the concern but we can easily detect nested pre-finalizers because > > we > > > > have > > > > > a > > > > > > > > NoAllocationScope before calling pre-finalizers. Also it would be > > easy > > > > to > > > > > > > > support nested pre-finalizers if needed -- we can just forbid GCs > > > while > > > > > > > calling > > > > > > > > pre-finalizers. > > > > > > > > > > > > > > > > > > > > > > An obscure assert will trigger on initial registration atm. Will you > > > > > guarantee > > > > > > > that the nested pre-finalizers are run in the same order as the > > > > destructors > > > > > > for > > > > > > > an object of type T? > > > > > > > > > > > > My hope is that we don't need to support nested pre-finalizers :) > > > > > > > > > > > > If I'm understanding things correctly, nested pre-finalizers can > happen > > > only > > > > > > when a GC is triggered in a nested manner. So an easy fix would be > just > > to > > > > > > forbid a GC while invoking pre-finalizers. However, my hope is that we > > can > > > > > just > > > > > > leave the NoAllocationScope around the pre-finalizers and guarantee > that > > > no > > > > > > allocation happens (and thus no GC happens) while invoking the > > > > pre-finalizers. > > > > > > > > > > Another idea might be just to replace all pre-finalizers with eager > > > sweepings. > > > > > This needs an assumption that the pre-finalizers don't touch any object > on > > > > > EagerSweepHeap. I guess the assumption would be holding currently, but > > that > > > is > > > > a > > > > > nasty assumption. > > > > > > > > That is a really interesting idea. If the eagerly swept heap (only) has > its > > > > unmarked objects poisoned before it is swept, then you get a hybrid -- > > > > finalizers being allowed to touch other objects, but not other eager > > objects. > > > > ASan should be effective in catching violations of that. I have to think a > > bit > > > > longer about the implications of that model.. > > > > > > > > As regards prefinalizers, I do hope new Blink code will be designed such > > that > > > > they have no use there. > > > > > > I think we should try to adopt that model, it simplifies. > EAGERLY_FINALIZED() > > > would be a more appropriate (and easier to understand) class-level > annotation. > > > > Let's do that. I'm on the same page :) > > > > Just to clarify: > > > > - Touch a not-eagerly-swept object in a destructor of a not-eagerly-swept > object > > => Invalid. Detected by the ASan poisoning. > > - Touch a not-eagerly-swept object in a destructor of an eagerly-swept object > => > > Valid. > > - Touch an eagerly-swept object in a destructor of a not-eagerly-swept object > => > > Invalid. Detected by another ASan poisoning on the free list. Also non-ASan > > build will crash. > > - Touch an eagerly-swept object in a destructor of an eagerly-swept object => > > Invalid. Detected by the ASan poisoning. > > > > Those will all hold, but to strengthen ASan checking, do you want to poison the > entire eagerly swept heap pre-sweep and unpoison what's alive afterwards? Sounds nicer :)
On 2015/05/25 06:32:43, sof wrote: > On 2015/05/25 00:16:28, haraken wrote: > ........................... > > > I hope EAGERLY_FINALIZED() will replace all EAGER_SWEEP() and > > USE_PRE_FINALIZER(). > > Let's see where it goes. Looks like it will work out reasonably well. I've split this up into three CLs (but the last could be divided into two, if needed): - https://codereview.chromium.org/1150863003/ (EAGERLY_FINALIZED() introduction.) - https://codereview.chromium.org/1158623002/ (clang Oilpan/GC plugin extensions.) - https://codereview.chromium.org/1157933002/ (recast prefinalizers as eagerly finalized classes + changes needed to support that; ASan & otherwise.) Needs to land in that order.
On 2015/05/25 21:11:56, sof wrote: > On 2015/05/25 06:32:43, sof wrote: > > On 2015/05/25 00:16:28, haraken wrote: > > > ........................... > > > > > I hope EAGERLY_FINALIZED() will replace all EAGER_SWEEP() and > > > USE_PRE_FINALIZER(). > > > > Let's see where it goes. > > Looks like it will work out reasonably well. > > I've split this up into three CLs (but the last could be divided into two, if > needed): > > - https://codereview.chromium.org/1150863003/ (EAGERLY_FINALIZED() > introduction.) > - https://codereview.chromium.org/1158623002/ (clang Oilpan/GC plugin > extensions.) > - https://codereview.chromium.org/1157933002/ (recast prefinalizers as eagerly > finalized classes + changes needed to support that; ASan & otherwise.) > > Needs to land in that order. Great news! Let's go along the way :)
On 2015/05/25 23:25:12, haraken wrote: > On 2015/05/25 21:11:56, sof wrote: > > On 2015/05/25 06:32:43, sof wrote: > > > On 2015/05/25 00:16:28, haraken wrote: > > > > > ........................... > > > > > > > I hope EAGERLY_FINALIZED() will replace all EAGER_SWEEP() and > > > > USE_PRE_FINALIZER(). > > > > > > Let's see where it goes. > > > > Looks like it will work out reasonably well. > > > > I've split this up into three CLs (but the last could be divided into two, if > > needed): > > > > - https://codereview.chromium.org/1150863003/ (EAGERLY_FINALIZED() > > introduction.) > > - https://codereview.chromium.org/1158623002/ (clang Oilpan/GC plugin > > extensions.) > > - https://codereview.chromium.org/1157933002/ (recast prefinalizers as > eagerly > > finalized classes + changes needed to support that; ASan & otherwise.) > > > > Needs to land in that order. > > Great news! Let's go along the way :) Updated to use EAGER_FINALIZE() - this one does not depend on GC plugin support.
LGTM https://codereview.chromium.org/1159583002/diff/20001/Source/modules/encrypte... File Source/modules/encryptedmedia/MediaKeySession.h (right): https://codereview.chromium.org/1159583002/diff/20001/Source/modules/encrypte... Source/modules/encryptedmedia/MediaKeySession.h:92: // Oilpan: eagerly release owned m_session. It would be nice to elaborate on this a bit more. // ... in order to clear WebContentDecryptionModuleSessionImpl::client_.
https://codereview.chromium.org/1159583002/diff/20001/Source/modules/encrypte... File Source/modules/encryptedmedia/MediaKeySession.h (right): https://codereview.chromium.org/1159583002/diff/20001/Source/modules/encrypte... Source/modules/encryptedmedia/MediaKeySession.h:92: // Oilpan: eagerly release owned m_session. On 2015/06/04 00:49:12, haraken wrote: > > It would be nice to elaborate on this a bit more. > > // ... in order to clear WebContentDecryptionModuleSessionImpl::client_. Expanded the comment.
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1159583002/#ps40001 (title: "improve comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1159583002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196480 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
