Oilpan: cancel MIDI permission request when prefinalizing

Oilpan: cancel MIDI permission request when prefinalizing

Cancelling an access permission request during finalization is a little
bit too late for the now-GCed MIDIAccessInitializer object. Arrange or a
prefinalizer to handle it instead.

R=haraken
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187515

[sof, 2014-12-18 14:36:03]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org, tasak@google.com

[sof, 2014-12-18 14:36:03]

Please take a look.

webmidi tests crash reliably with leak detection enabled.

[haraken, 2014-12-18 14:42:45]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2014-12-18 14:42:46]

https://codereview.chromium.org/805263004/diff/1/Source/modules/webmidi/MIDIA...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/805263004/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessInitializer.cpp:29: {

What happens if MIDIAccessInitializer gets destructed before
ActiveDOMObject::stop() is called?

https://codereview.chromium.org/805263004/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessInitializer.cpp:43: // It is safe to cancel a
request which is already finished or canceld.

canceled

[sof, 2014-12-18 14:53:54]

https://codereview.chromium.org/805263004/diff/1/Source/modules/webmidi/MIDIA...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/805263004/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessInitializer.cpp:29: {
On 2014/12/18 14:42:46, haraken wrote:
> 
> What happens if MIDIAccessInitializer gets destructed before
> ActiveDOMObject::stop() is called?
> 

A permissions request will be made with the controller as part of
MIDIAccessInitializer start()ing. At that point the ActiveDOMObject is in a
started or suspended state, right? I assume that it is not possible for this
ActiveDOMObject to be finalized before a stop() is issued by the execution
context.

[haraken, 2014-12-18 15:02:28]

https://codereview.chromium.org/805263004/diff/1/Source/modules/webmidi/MIDIA...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/805263004/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessInitializer.cpp:29: {
On 2014/12/18 14:53:54, sof wrote:
> On 2014/12/18 14:42:46, haraken wrote:
> > 
> > What happens if MIDIAccessInitializer gets destructed before
> > ActiveDOMObject::stop() is called?
> > 
> 
> A permissions request will be made with the controller as part of
> MIDIAccessInitializer start()ing. At that point the ActiveDOMObject is in a
> started or suspended state, right? I assume that it is not possible for this
> ActiveDOMObject to be finalized before a stop() is issued by the execution
> context.

I'm wondering if it's possible that the MIDIAccessInitializer object dies before
the ExecutionContext stops. For example:

(1) MIDIAccessInitializer is created.
(2) The last reference to the MIDIAccessInitializer is gone.
(3) GC collects the MIDIAccessInitializer.
(4) ExecutionContext stops.

?

[sof, 2014-12-18 15:17:58]

https://codereview.chromium.org/805263004/diff/1/Source/modules/webmidi/MIDIA...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/805263004/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessInitializer.cpp:29: {
On 2014/12/18 15:02:28, haraken wrote:
> On 2014/12/18 14:53:54, sof wrote:
> > On 2014/12/18 14:42:46, haraken wrote:
> > > 
> > > What happens if MIDIAccessInitializer gets destructed before
> > > ActiveDOMObject::stop() is called?
> > > 
> > 
> > A permissions request will be made with the controller as part of
> > MIDIAccessInitializer start()ing. At that point the ActiveDOMObject is in a
> > started or suspended state, right? I assume that it is not possible for this
> > ActiveDOMObject to be finalized before a stop() is issued by the execution
> > context.
> 
> I'm wondering if it's possible that the MIDIAccessInitializer object dies
before
> the ExecutionContext stops. For example:
> 
> (1) MIDIAccessInitializer is created.
> (2) The last reference to the MIDIAccessInitializer is gone.
> (3) GC collects the MIDIAccessInitializer.
> (4) ExecutionContext stops.
> 
> ?

If that's a concern, I can rephrase this to be a reg/unreg protocol between the
controller and this object instead, when there is time. Using a controller-weak
reference, or something.

[haraken, 2014-12-18 15:44:02]

https://codereview.chromium.org/805263004/diff/1/Source/modules/webmidi/MIDIA...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/805263004/diff/1/Source/modules/webmidi/MIDIA...
Source/modules/webmidi/MIDIAccessInitializer.cpp:29: {
On 2014/12/18 15:17:57, sof wrote:
> On 2014/12/18 15:02:28, haraken wrote:
> > On 2014/12/18 14:53:54, sof wrote:
> > > On 2014/12/18 14:42:46, haraken wrote:
> > > > 
> > > > What happens if MIDIAccessInitializer gets destructed before
> > > > ActiveDOMObject::stop() is called?
> > > > 
> > > 
> > > A permissions request will be made with the controller as part of
> > > MIDIAccessInitializer start()ing. At that point the ActiveDOMObject is in
a
> > > started or suspended state, right? I assume that it is not possible for
this
> > > ActiveDOMObject to be finalized before a stop() is issued by the execution
> > > context.
> > 
> > I'm wondering if it's possible that the MIDIAccessInitializer object dies
> before
> > the ExecutionContext stops. For example:
> > 
> > (1) MIDIAccessInitializer is created.
> > (2) The last reference to the MIDIAccessInitializer is gone.
> > (3) GC collects the MIDIAccessInitializer.
> > (4) ExecutionContext stops.
> > 
> > ?
> 
> If that's a concern, I can rephrase this to be a reg/unreg protocol between
the
> controller and this object instead, when there is time. Using a
controller-weak
> reference, or something.
> 

Yeah, I guess a more straightforward approach would be to have MIDIController
have a weak reference to the MIDIAccessInitializer so that the MIDIController
can call cancelSysexPermissionRequest() when the MIDIAccessInitializer is going
to be destructed.

In your approach, it's still not clear to me who keeps the MIDIAccessInitializer
object alive until the ExecutionContext stops.

[sof, 2014-12-18 15:52:19]

On 2014/12/18 15:44:02, haraken wrote:
>
https://codereview.chromium.org/805263004/diff/1/Source/modules/webmidi/MIDIA...
> File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):
> 
>
https://codereview.chromium.org/805263004/diff/1/Source/modules/webmidi/MIDIA...
> Source/modules/webmidi/MIDIAccessInitializer.cpp:29: {
> On 2014/12/18 15:17:57, sof wrote:
> > On 2014/12/18 15:02:28, haraken wrote:
> > > On 2014/12/18 14:53:54, sof wrote:
> > > > On 2014/12/18 14:42:46, haraken wrote:
> > > > > 
> > > > > What happens if MIDIAccessInitializer gets destructed before
> > > > > ActiveDOMObject::stop() is called?
> > > > > 
> > > > 
> > > > A permissions request will be made with the controller as part of
> > > > MIDIAccessInitializer start()ing. At that point the ActiveDOMObject is
in
> a
> > > > started or suspended state, right? I assume that it is not possible for
> this
> > > > ActiveDOMObject to be finalized before a stop() is issued by the
execution
> > > > context.
> > > 
> > > I'm wondering if it's possible that the MIDIAccessInitializer object dies
> > before
> > > the ExecutionContext stops. For example:
> > > 
> > > (1) MIDIAccessInitializer is created.
> > > (2) The last reference to the MIDIAccessInitializer is gone.
> > > (3) GC collects the MIDIAccessInitializer.
> > > (4) ExecutionContext stops.
> > > 
> > > ?
> > 
> > If that's a concern, I can rephrase this to be a reg/unreg protocol between
> the
> > controller and this object instead, when there is time. Using a
> controller-weak
> > reference, or something.
> > 
> 
> Yeah, I guess a more straightforward approach would be to have MIDIController
> have a weak reference to the MIDIAccessInitializer so that the MIDIController
> can call cancelSysexPermissionRequest() when the MIDIAccessInitializer is
going
> to be destructed.
> 
> In your approach, it's still not clear to me who keeps the
MIDIAccessInitializer
> object alive until the ExecutionContext stops.

I put a prefinalizer on it instead of stop() overriding.

[haraken, 2014-12-19 03:47:16]

LGTM

https://codereview.chromium.org/805263004/diff/20001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccessInitializer.h (right):

https://codereview.chromium.org/805263004/diff/20001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.h:23:
USING_PRE_FINALIZER(MIDIAccessInitializer, dispose);

Just to confirm: The reason we cannot use WeakMember (i.e., have MIDIController
hold WeakMember<MIDIAccessInitializer>) is that the weak processing does not run
when the MIDIController and the MIDIAccessInitializer die together, right?
MIDIController always needs to cancel the permission request when the
MIDIAccessInitializer dies (regardless of whether the MIDIController dies at the
same time or not). Thus we need to use a pre-finalizer. Am I understanding
correctly?

[sof, 2014-12-19 08:06:46]

On 2014/12/19 03:47:16, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/805263004/diff/20001/Source/modules/webmidi/M...
> File Source/modules/webmidi/MIDIAccessInitializer.h (right):
> 
>
https://codereview.chromium.org/805263004/diff/20001/Source/modules/webmidi/M...
> Source/modules/webmidi/MIDIAccessInitializer.h:23:
> USING_PRE_FINALIZER(MIDIAccessInitializer, dispose);
> 
> Just to confirm: The reason we cannot use WeakMember (i.e., have
MIDIController
> hold WeakMember<MIDIAccessInitializer>) is that the weak processing does not
run
> when the MIDIController and the MIDIAccessInitializer die together, right?
> MIDIController always needs to cancel the permission request when the
> MIDIAccessInitializer dies (regardless of whether the MIDIController dies at
the
> same time or not). Thus we need to use a pre-finalizer. Am I understanding
> correctly?

Yes, see the WebMIDIPermissionRequest comment for the lifetimes assumed and when
cancellation must happen.

[sof, 2014-12-19 08:07:14]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-12-19 08:08:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/805263004/20001

[commit-bot: I haz the power, 2014-12-19 08:11:14]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187515