Fix a memory leak in the app banner prompt layout tests.

Fix a memory leak in the app banner prompt layout tests.

Calling prompt() in a closure after the userChoice promise appears to
create a circular reference and a memory leak. Lifting the event variable
to global scope prevents another leak.

BUG=504675
R=mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-06-29 14:12:53]

Could you explain what created the circular dependency?

[dominickn, 2015-06-30 04:34:30]

On 2015/06/29 14:12:53, Mounir Lamouri wrote:
> Could you explain what created the circular dependency?

The main issue is that declaring the event variable at function scope at line 84
seems to cause a cyclic reference with the enclosed scope of the event handler;
moving this variable to global scope fixes this. Moving the variable then causes
the verify_prompt_reject call at line 71 to fail as the event object has become
undefined at that point; removing the second promise call and simply making the
call in line fixes this. The other change also makes sure that preventDefault is
called within the handler, and not after the handler has returned.

[mlamouri (slow - plz ping), 2015-06-30 10:06:56]

On 2015/06/30 at 04:34:30, dominickn wrote:
> On 2015/06/29 14:12:53, Mounir Lamouri wrote:
> > Could you explain what created the circular dependency?
> 
> The main issue is that declaring the event variable at function scope at line
84 seems to cause a cyclic reference with the enclosed scope of the event
handler; moving this variable to global scope fixes this. Moving the variable
then causes the verify_prompt_reject call at line 71 to fail as the event object
has become undefined at that point; removing the second promise call and simply
making the call in line fixes this. The other change also makes sure that
preventDefault is called within the handler, and not after the handler has
returned.

I'm a bit concerned by the changes in very_userChoice because you are breaking
the guaranteed order of the calls. userChoice and prompt are now called one
after the other, making the test potentially flaky. Maybe you could keep an
array of events instead of the last event so running the next test will not set
the event to null? Or did I misunderstood the reason why the event becomes null?

[dominickn, 2015-07-02 01:34:21]

On 2015/06/30 10:06:56, Mounir Lamouri wrote:
> I'm a bit concerned by the changes in very_userChoice because you are breaking
> the guaranteed order of the calls. userChoice and prompt are now called one
> after the other, making the test potentially flaky. Maybe you could keep an
> array of events instead of the last event so running the next test will not
set
> the event to null? Or did I misunderstood the reason why the event becomes
null?

Having an array stops the event from becoming null, but it leaks again. I now
think that there is a circular reference caused by the anonymous function at
line 70, which captures the event and the test objects in its scope and is
attached to the promise returned by userChoice. Does this explanation make
sense?

[mlamouri (slow - plz ping), 2015-07-05 17:04:57]

On 2015/07/02 at 01:34:21, dominickn wrote:
> On 2015/06/30 10:06:56, Mounir Lamouri wrote:
> > I'm a bit concerned by the changes in very_userChoice because you are
breaking
> > the guaranteed order of the calls. userChoice and prompt are now called one
> > after the other, making the test potentially flaky. Maybe you could keep an
> > array of events instead of the last event so running the next test will not
set
> > the event to null? Or did I misunderstood the reason why the event becomes
null?
> 
> Having an array stops the event from becoming null, but it leaks again. I now
think that there is a circular reference caused by the anonymous function at
line 70, which captures the event and the test objects in its scope and is
attached to the promise returned by userChoice. Does this explanation make
sense?

My concern is that it sounds like a real leak and the changes in the test might
only be working around the real issue. Given that you said that the leak comes
from the event, I had a quick look into the class and I wonder if that could
come from m_userChoice. A ScriptPromise is referencing a lot of things like the
World object. I wouldn't be surprised if that would create a cycle that would
create a leak.

Could you look into this? Maybe you could check if the leak is still present
when removing m_userChoice from the class (and ::userChoice would return a basic
Promise<bool>)? FWIW, there is a class made for properties returning a promise
called called ScriptPromiseProperty. If the leak comes from there I think you
could easily fix it using that helper.

[dominickn, 2015-07-06 09:12:19]

On 2015/07/05 17:04:57, Mounir Lamouri wrote:
> On 2015/07/02 at 01:34:21, dominickn wrote:
> > On 2015/06/30 10:06:56, Mounir Lamouri wrote:
> > > I'm a bit concerned by the changes in very_userChoice because you are
> breaking
> > > the guaranteed order of the calls. userChoice and prompt are now called
one
> > > after the other, making the test potentially flaky. Maybe you could keep
an
> > > array of events instead of the last event so running the next test will
not
> set
> > > the event to null? Or did I misunderstood the reason why the event becomes
> null?
> > 
> > Having an array stops the event from becoming null, but it leaks again. I
now
> think that there is a circular reference caused by the anonymous function at
> line 70, which captures the event and the test objects in its scope and is
> attached to the promise returned by userChoice. Does this explanation make
> sense?
> 
> My concern is that it sounds like a real leak and the changes in the test
might
> only be working around the real issue. Given that you said that the leak comes
> from the event, I had a quick look into the class and I wonder if that could
> come from m_userChoice. A ScriptPromise is referencing a lot of things like
the
> World object. I wouldn't be surprised if that would create a cycle that would
> create a leak.
> 
> Could you look into this? Maybe you could check if the leak is still present
> when removing m_userChoice from the class (and ::userChoice would return a
basic
> Promise<bool>)? FWIW, there is a class made for properties returning a promise
> called called ScriptPromiseProperty. If the leak comes from there I think you
> could easily fix it using that helper.

Thanks for the intuition! I've refactored by storing a resolver object in the
event, rather than a script promise (as the CallbackPromiseAdapter needs a
resolver, which the ScriptPromiseProperty doesn't seem to have access to). This
seems to address the leak problems in the test, but I'm not entirely convinced
that this has the correct semantics (same promise each time userChoice() is
called). Please let me know your thoughts.

[mlamouri (slow - plz ping), 2015-07-06 10:29:32]

mlamouri@chromium.org changed reviewers:
+ tasak@google.com

[mlamouri (slow - plz ping), 2015-07-06 10:29:33]

+tasak@, do you have any idea how to safely have ScriptPromiseProperty work with
CallbackPromiseAdapter? ie. we need to pass the resolver from the former to the
latter.

I was wondering if it wouldn't be easier to have a callback that doesn't carry
the resolver but instead keeps a Persistent reference to the
BeforeInstallPromptEvent object. Then it could notify that the user choice was
made which would resolve the promise. If the object was kept alive by the
callback, it would be destroyed immediately.

Otherwise, I think it would be better to not change the test file in this CL to
make sure the leak is fixed by the implementation changes.

[dominickn, 2015-07-10 06:07:35]

On 2015/07/06 10:29:33, Mounir Lamouri wrote:
> +tasak@, do you have any idea how to safely have ScriptPromiseProperty work
with
> CallbackPromiseAdapter? ie. we need to pass the resolver from the former to
the
> latter.
> 
> I was wondering if it wouldn't be easier to have a callback that doesn't carry
> the resolver but instead keeps a Persistent reference to the
> BeforeInstallPromptEvent object. Then it could notify that the user choice was
> made which would resolve the promise. If the object was kept alive by the
> callback, it would be destroyed immediately.
> 
> Otherwise, I think it would be better to not change the test file in this CL
to
> make sure the leak is fixed by the implementation changes.

tasak@, can you please comment on this CL?

[tasak, 2015-07-10 06:34:35]

tasak@google.com changed reviewers:
+ haraken@chromium.org

[tasak, 2015-07-10 06:34:35]

+haraken

[haraken, 2015-07-10 06:40:09]

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

[haraken, 2015-07-10 06:40:10]

+yhirano for ScriptPromise

[yhirano, 2015-07-10 06:51:41]

I do not understand the spec, but perhaps what you want may be
ScriptPromiseProperty.
https://docs.google.com/document/d/1WphFrSM18-m6b4RFaBxwLL_zNlpOdCtEbuRclQ-S_ts

[yhirano, 2015-07-10 07:47:44]

On 2015/07/10 06:51:41, yhirano wrote:
> I do not understand the spec, but perhaps what you want may be
> ScriptPromiseProperty.
>
https://docs.google.com/document/d/1WphFrSM18-m6b4RFaBxwLL_zNlpOdCtEbuRclQ-S_ts

Oh, sorry, I didn't read the discussion carefully.
Currently CallbackPromiseAdapter cannot be used with ScriptPromiseResolver. You
can define your own WebCallbacks subclass, I think.

[dominickn, 2015-07-10 07:49:00]

On 2015/07/10 06:51:41, yhirano wrote:
> I do not understand the spec, but perhaps what you want may be
> ScriptPromiseProperty.
>
https://docs.google.com/document/d/1WphFrSM18-m6b4RFaBxwLL_zNlpOdCtEbuRclQ-S_ts

@yhriano how do I get ScriptPromiseProperty to work with CallbackPromiseAdapter?
The latter needs a ScriptPromiseResolver, which I'm not sure how to get from the
Property.

[dominickn, 2015-07-14 05:53:27]

Just following up here - is there any more advice on using
CallbackPromiseAdapter with ScriptPromiseProperty?

[yhirano, 2015-07-14 06:01:56]

>#18 

Just in case you didn't notice: As I said at #16 you can define your own
WebCallbacks.

[yhirano, 2015-07-14 09:11:24]

By the way, I'm planning WebCallbacks / CallbackPromiseAdapter refactoring.
Their interface may change.
http://code.google.com/p/chromium/issues/detail?id=493531

[dominickn, 2015-07-23 04:10:37]

Continuing this in a new CL, where I've refactored the implementation to use
ScriptPromiseProperty: https://codereview.chromium.org/1247283004/