New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 726414 link

Starred by 2 users
Status: Fixed
Owner:
hirosh...@chromium.org
Closed: Jun 2017
Cc:
kouhei@chromium.org
█ mbarowsky+blinkimage@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 327574
issue 383741
issue 726091



Sign in to add a comment

Reconsider ImageLoader lifetime

Project Member Reported by hirosh...@chromium.org, May 25 2017 
Currently, ImageLoader is forced to be alive from ImageResourceContent::Fetch() until the image load/error event is fired, using |keep_alive_| Persistent (*1).
But this causes:
- Code health issue: |keep_alive_| hack is not clean ( Issue 383741 ).
- Memory leaks ( Issue 327574  and  726091 ):
  They are "working (=leaking) as intended" if we assume (*1). So to fix the leaks,
  we have to reconsider (*1).

Basically I expect the solution will be to remove |keep_alive_| and adjust the ImageLoader lifetime.

Comment 1 by hirosh...@chromium.org, May 25 2017

Status: Assigned (was: Untriaged)

Comment 2 by schenney@chromium.org, May 25 2017

Labels: BugSource-Chromium PaintTeamTriaged-20170525

Comment 3 by hirosh...@chromium.org, May 25 2017 

Also related to cleaning up the fallback conditions in SelectSourceURL().
Project Member

Comment 4 by bugdroid1@chromium.org, May 30 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/44f1ff3ef09322690900c00580c90b841a52d73a

commit 44f1ff3ef09322690900c00580c90b841a52d73a
Author: hiroshige <hiroshige@chromium.org>
Date: Tue May 30 09:38:22 2017

Remove T:: from ActiveScriptWrappable<T>::DispatchHasPendingActivity()

Previously, for a class T : public ActiveScriptWrappable<T>,
HasPendingActivity() cannot be overridden by the subclasses of T
because DispatchHasPendingActivity() calls T::HasPendingActivity().

This CL enables the override by calling HasPendingActivity() instead,
as preparation for https://codereview.chromium.org/2905233002/.

BUG= 383741 ,  327574 ,  726091 ,  726414 

Review-Url: https://codereview.chromium.org/2907173002
Cr-Commit-Position: refs/heads/master@{#475480}

[modify] https://crrev.com/44f1ff3ef09322690900c00580c90b841a52d73a/third_party/WebKit/Source/platform/bindings/ActiveScriptWrappable.h
Project Member

Comment 5 by bugdroid1@chromium.org, May 30 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da1ebb6c4b424e74296c55b180055d1a98b73832

commit da1ebb6c4b424e74296c55b180055d1a98b73832
Author: hiroshige <hiroshige@chromium.org>
Date: Tue May 30 12:53:30 2017

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}

[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/LayoutTests/LeakExpectations
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/html/HTMLEmbedElement.idl
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/html/HTMLInputElement.cpp
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/html/HTMLInputElement.h
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/html/HTMLInputElement.idl
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/html/HTMLMediaElement.h
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/html/HTMLObjectElement.idl
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/html/HTMLPlugInElement.h
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/html/HTMLVideoElement.cpp
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/html/HTMLVideoElement.h
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/html/HTMLVideoElement.idl
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/loader/ImageLoader.h
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/svg/SVGImageElement.h
[modify] https://crrev.com/da1ebb6c4b424e74296c55b180055d1a98b73832/third_party/WebKit/Source/core/svg/SVGImageElement.idl

Comment 6 by hirosh...@chromium.org, Jun 13 2017

Status: Fixed (was: Assigned)
Closing as fixed.
Actually the removal of |keep_alive_|, i.e. allowing ImageLoader to be garbage collected after the associated context becomes invalid, resolved the all related issues.
►

Sign in to add a comment