Oilpan: make SuspendableScriptExecutor safe.

Oilpan: make SuspendableScriptExecutor safe.

This ActiveDOMObject was missing two pieces:

 - remove itself as an Observer upon completion.
 - promptly stop itself as a timer upon finalization.

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

[sof, 2015-06-23 07:02:20]

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

[sof, 2015-06-23 07:02:20]

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

[sof, 2015-06-23 07:02:21]

please take a look.

[sof, 2015-06-23 07:02:21]

please take a look.

[haraken, 2015-06-23 08:12:01]

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

[haraken, 2015-06-23 08:12:01]

https://codereview.chromium.org/1197163003/diff/1/Source/web/SuspendableScrip...
File Source/web/SuspendableScriptExecutor.cpp (right):

https://codereview.chromium.org/1197163003/diff/1/Source/web/SuspendableScrip...
Source/web/SuspendableScriptExecutor.cpp:21: #if !ENABLE(OILPAN)

In Oilpan, who keeps the SuspendableScriptExecutor alive?

One option to keep the current lifetime relationship would be to use
RefCountedGarbageCollected. Then we can use ref/deref.

[sof, 2015-06-23 09:17:31]

https://codereview.chromium.org/1197163003/diff/1/Source/web/SuspendableScrip...
File Source/web/SuspendableScriptExecutor.cpp (right):

https://codereview.chromium.org/1197163003/diff/1/Source/web/SuspendableScrip...
Source/web/SuspendableScriptExecutor.cpp:21: #if !ENABLE(OILPAN)
On 2015/06/23 08:12:01, haraken wrote:
> 
> In Oilpan, who keeps the SuspendableScriptExecutor alive?
> 
> One option to keep the current lifetime relationship would be to use
> RefCountedGarbageCollected. Then we can use ref/deref.

Done.

[haraken, 2015-06-23 09:25:56]

https://codereview.chromium.org/1197163003/diff/20001/Source/web/SuspendableS...
File Source/web/SuspendableScriptExecutor.cpp (right):

https://codereview.chromium.org/1197163003/diff/20001/Source/web/SuspendableS...
Source/web/SuspendableScriptExecutor.cpp:91: ActiveDOMObject::clearContext();

Is this a performance optimization? Or do we need to explicitly remove the
observer for correctness?

(It seems a bit strange to call clearContext in contextDestroyed.)

[sof, 2015-06-23 09:40:59]

https://codereview.chromium.org/1197163003/diff/20001/Source/web/SuspendableS...
File Source/web/SuspendableScriptExecutor.cpp (right):

https://codereview.chromium.org/1197163003/diff/20001/Source/web/SuspendableS...
Source/web/SuspendableScriptExecutor.cpp:91: ActiveDOMObject::clearContext();
On 2015/06/23 09:25:56, haraken wrote:
> 
> Is this a performance optimization? Or do we need to explicitly remove the
> observer for correctness?
> 
> (It seems a bit strange to call clearContext in contextDestroyed.)

I think it makes sense to remove oneself here, rather than keep a weak reference
lingering. We know this observer's lifetime is up.

[haraken, 2015-06-23 09:44:55]

https://codereview.chromium.org/1197163003/diff/20001/Source/web/SuspendableS...
File Source/web/SuspendableScriptExecutor.cpp (right):

https://codereview.chromium.org/1197163003/diff/20001/Source/web/SuspendableS...
Source/web/SuspendableScriptExecutor.cpp:91: ActiveDOMObject::clearContext();
On 2015/06/23 09:40:59, sof wrote:
> On 2015/06/23 09:25:56, haraken wrote:
> > 
> > Is this a performance optimization? Or do we need to explicitly remove the
> > observer for correctness?
> > 
> > (It seems a bit strange to call clearContext in contextDestroyed.)
> 
> I think it makes sense to remove oneself here, rather than keep a weak
reference
> lingering. We know this observer's lifetime is up.

Makes sense. (I'd prefer calling ActiveDOMObject::clearContext() only in
executeAndDestroySelf() though; it's up to you.)

[haraken, 2015-06-23 09:45:02]

On 2015/06/23 09:44:55, haraken wrote:
>
https://codereview.chromium.org/1197163003/diff/20001/Source/web/SuspendableS...
> File Source/web/SuspendableScriptExecutor.cpp (right):
> 
>
https://codereview.chromium.org/1197163003/diff/20001/Source/web/SuspendableS...
> Source/web/SuspendableScriptExecutor.cpp:91: ActiveDOMObject::clearContext();
> On 2015/06/23 09:40:59, sof wrote:
> > On 2015/06/23 09:25:56, haraken wrote:
> > > 
> > > Is this a performance optimization? Or do we need to explicitly remove the
> > > observer for correctness?
> > > 
> > > (It seems a bit strange to call clearContext in contextDestroyed.)
> > 
> > I think it makes sense to remove oneself here, rather than keep a weak
> reference
> > lingering. We know this observer's lifetime is up.
> 
> Makes sense. (I'd prefer calling ActiveDOMObject::clearContext() only in
> executeAndDestroySelf() though; it's up to you.)

LGTM.

[sof, 2015-06-23 09:51:44]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-06-23 09:51:56]

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

[commit-bot: I haz the power, 2015-06-23 11:32:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-23 11:32:24]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60256)

[sof, 2015-06-23 12:20:47]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-06-23 12:21:07]

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

[commit-bot: I haz the power, 2015-06-23 12:59:46]

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