Remove the lifetime hack in ImageLoader where it keeps its assoc element alive

Remove the lifetime hack in ImageLoader where it keeps its assoc element alive

Instead of keeping Element alive by |ImageLoader::keep_alive_|, this
CL makes the Elements keep themselves alive by HasPendingActivity().

This makes ImageLoader NOT kept alive after the corresponding context
is destroyed, which is the general behavior of HasPendingActivity().
Previously, ImageLoader is forced to be kept alive until load/error
events are fired, because |keep_alive_| is not cleared, regardless of
whether the context is still valid or not.
This lifetime change fixes memory leaks in tests (Issue 327574 and
Issue 726091).

This CL is based on kouhei@'s CL:
https://codereview.chromium.org/2344563002/.
In addition to that, this CL also applies HasPendingActivity() changes
to Input, PlugIn/Embed/Object, and Media/Video elements because they
create ImageLoader and retain references to ImageLoader.

BUG=383741, 327574, 726091, 726414
Review-Url: https://codereview.chromium.org/2905233002
Cr-Commit-Position: refs/heads/master@{#475505}
Committed: https://chromium.googlesource.com/chromium/src/+/da1ebb6c4b424e74296c55b180055d1a98b73832

[hiroshige, 2017-05-26 01:16:24]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[hiroshige, 2017-05-26 01:17:00]

Description was changed from

==========
Remove ImageLoader::keep_alive_

BUG=
==========

to

==========
Remove ImageLoader::keep_alive_

Local rebase of kouhei's CL:
https://codereview.chromium.org/2344563002/

BUG=
==========

[hiroshige, 2017-05-26 01:17:15]

Description was changed from

==========
Remove ImageLoader::keep_alive_

Local rebase of kouhei's CL:
https://codereview.chromium.org/2344563002/

BUG=
==========

to

==========
Remove ImageLoader::keep_alive_

Local rebase of kouhei's CL:
https://codereview.chromium.org/2344563002/

BUG=383741
==========

[commit-bot: I haz the power, 2017-05-26 01:17:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 03:08:53]

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

[commit-bot: I haz the power, 2017-05-26 03:08:59]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hiroshige, 2017-05-26 21:28:32]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 21:29:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-26 21:37:49]

Description was changed from

==========
Remove ImageLoader::keep_alive_

Local rebase of kouhei's CL:
https://codereview.chromium.org/2344563002/

BUG=383741
==========

to

==========
Remove the lifetime hack in ImageLoader where it keeps its assoc element alive

Instead of keeping Element alive by |ImageLoader::keep_alive_|, this
CL makes the Elements keep themselves alive by HasPendingActivity().

This makes ImageLoader NOT kept alive after the corresponding context
is destroyed, which is the general behavior of HasPendingActivity().
Previously, ImageLoader is forced to be kept alive until load/error
events are fired, because |keep_alive_| is not cleared, regardless of
whether the context is still valid or not.
This lifetime change fixes memory leaks in tests (Issue 327574 and
Issue 726091).

This CL is based on kouhei@'s CL:
https://codereview.chromium.org/2344563002/.
In addition to that, this CL also applies HasPendingActivity() changes
to Input, PlugIn/Embed/Object, and Media/Video elements because they
create ImageLoader and retain references to ImageLoader.

BUG=383741, 327574, 726091, 726414
==========

[hiroshige, 2017-05-26 21:37:49]

hiroshige@chromium.org changed reviewers:
+ kouhei@chromium.org

[hiroshige, 2017-05-26 21:41:18]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 21:42:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 23:35:55]

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

[commit-bot: I haz the power, 2017-05-26 23:35:56]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2017-05-29 05:27:34]

https://codereview.chromium.org/2905233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/2905233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:343: virtual bool
HasPendingActivityInternal() const { return false; }
Why is this needed?
HTMLVideoElement inherits from HTMLMediaElement so sounds like it could directly
use HasPendingActivity

[hiroshige, 2017-05-29 10:36:59]

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

[hiroshige, 2017-05-29 10:37:00]

PTAL.

https://codereview.chromium.org/2905233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/2905233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:343: virtual bool
HasPendingActivityInternal() const { return false; }
On 2017/05/29 05:27:34, kouhei (from MUC) wrote:
> Why is this needed?
> HTMLVideoElement inherits from HTMLMediaElement so sounds like it could
directly
> use HasPendingActivity

I did so in Patch Set 2 but it didn't work, perhaps because
T::HasPendingActivity is called directly for ActiveScriptWrappable<T> via
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/platfo...,
which doesn't consider overrides.

[hiroshige, 2017-05-29 15:20:28]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 15:20:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-29 17:08:23]

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

[commit-bot: I haz the power, 2017-05-29 17:08:24]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-05-30 01:36:45]

lgtm

[kinuko, 2017-05-30 02:06:59]

This looks a lot nicer!

[hiroshige, 2017-05-30 08:57:06]

https://codereview.chromium.org/2905233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/2905233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:343: virtual bool
HasPendingActivityInternal() const { return false; }
On 2017/05/29 10:37:00, hiroshige wrote:
> On 2017/05/29 05:27:34, kouhei (from MUC) wrote:
> > Why is this needed?
> > HTMLVideoElement inherits from HTMLMediaElement so sounds like it could
> directly
> > use HasPendingActivity
> 
> I did so in Patch Set 2 but it didn't work, perhaps because
> T::HasPendingActivity is called directly for ActiveScriptWrappable<T> via
>
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/platfo...,
> which doesn't consider overrides.

According to offline discussion with kouhei@, I modify ActiveScriptWrappable to
allow HasPendingActivity() overriding
(https://codereview.chromium.org/2907173002) and remove
HasPendingActivityInternal() hack in this CL in Patch Set 4.

[kouhei (in TOK), 2017-05-30 08:58:03]

lgtm thanks!

[hiroshige, 2017-05-30 09:53:01]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-05-30 09:53:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-30 09:53:18]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2017-05-30 12:48:51]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-05-30 12:49:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 12:53:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496148531282860,
"parent_rev": "3f731c7c47954c7c824f0d157af020f5ee5b57ce", "commit_rev":
"da1ebb6c4b424e74296c55b180055d1a98b73832"}

[commit-bot: I haz the power, 2017-05-30 12:53:43]

Description was changed from

==========
Remove the lifetime hack in ImageLoader where it keeps its assoc element alive

Instead of keeping Element alive by |ImageLoader::keep_alive_|, this
CL makes the Elements keep themselves alive by HasPendingActivity().

This makes ImageLoader NOT kept alive after the corresponding context
is destroyed, which is the general behavior of HasPendingActivity().
Previously, ImageLoader is forced to be kept alive until load/error
events are fired, because |keep_alive_| is not cleared, regardless of
whether the context is still valid or not.
This lifetime change fixes memory leaks in tests (Issue 327574 and
Issue 726091).

This CL is based on kouhei@'s CL:
https://codereview.chromium.org/2344563002/.
In addition to that, this CL also applies HasPendingActivity() changes
to Input, PlugIn/Embed/Object, and Media/Video elements because they
create ImageLoader and retain references to ImageLoader.

BUG=383741, 327574, 726091, 726414
==========

to

==========
Remove the lifetime hack in ImageLoader where it keeps its assoc element alive

Instead of keeping Element alive by |ImageLoader::keep_alive_|, this
CL makes the Elements keep themselves alive by HasPendingActivity().

This makes ImageLoader NOT kept alive after the corresponding context
is destroyed, which is the general behavior of HasPendingActivity().
Previously, ImageLoader is forced to be kept alive until load/error
events are fired, because |keep_alive_| is not cleared, regardless of
whether the context is still valid or not.
This lifetime change fixes memory leaks in tests (Issue 327574 and
Issue 726091).

This CL is based on kouhei@'s CL:
https://codereview.chromium.org/2344563002/.
In addition to that, this CL also applies HasPendingActivity() changes
to Input, PlugIn/Embed/Object, and Media/Video elements because they
create ImageLoader and retain references to ImageLoader.

BUG=383741, 327574, 726091, 726414

Review-Url: https://codereview.chromium.org/2905233002
Cr-Commit-Position: refs/heads/master@{#475505}
Committed:
https://chromium.googlesource.com/chromium/src/+/da1ebb6c4b424e74296c55b18005...
==========

[commit-bot: I haz the power, 2017-05-30 12:53:45]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/da1ebb6c4b424e74296c55b18005...