New issue
Advanced search Search tips

Issue 726091 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 726414

Blocking:
issue 624697



Sign in to add a comment

Fix leaks in https://codereview.chromium.org/2863763003

Project Member Reported by hirosh...@chromium.org, May 24 2017

Issue description

https://codereview.chromium.org/2863763003 is leaking and was reverted
(https://codereview.chromium.org/2903773002).

Leaking tests:
editing/execCommand/insert-image-changing-visibility-crash.html
editing/pasteboard/drag-drop-iframe-refresh-crash.html
editing/pasteboard/drag-image-in-about-blank-frame.html
editing/spelling/mixed_paste.html
external/wpt/html/infrastructure/urls/terminology-0/document-base-url.html
external/wpt/service-workers/service-worker/fetch-canvas-tainting.https.html
external/wpt/service-workers/service-worker/fetch-request-resources.https.html
fast/events/inputevents/beforeinput-remove-iframe-crash.html
fast/loader/ping-error.html
virtual/off-main-thread-fetch/external/wpt/service-workers/service-worker/fetch-canvas-tainting.https.html
virtual/off-main-thread-fetch/external/wpt/service-workers/service-worker/fetch-request-resources.https.html
virtual/off-main-thread-fetch/http/tests/serviceworker/chromium.fetch-request-resources.html

 
Blockedon: 726414
The ImageLoader is "leaking", because
A. it is forced to be kept alive until image load or error event.
(In these cases, actual event handlers are not set, but anyway ImageLoader is kept alive until DispatchPendingLoad/ErrorEvent())
and
B. the test is finished before the image load event.
In some tests, this is because image loading is triggered in document onload, and thus the image loading no longer delay the document load event and thus no longer blocks testrunner finish.

Before my CL, they are not detected as leaking, because the image load/error events are fired earlier, i.e. before the test finishes, due to the TaskRunner priority.

If we use TimerTaskRunner (that was used for EventSender before my CL) instead of DOMManipulation task runner, the leak is fixed.
(see Patch Set 2 of https://codereview.chromium.org/2905493005/)
ImageLoader is kept alive even after the corresponding image element is removed from the DOM tree because onload/onerror JavaScript calls are observable, but perhaps we don't have to keep ImageLoader alive after the corresponding Document can be destructed?
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/+/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 6 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

Project Member

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

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

commit 67e066b84ec7953ec06ecf252f466c2c2c063f33
Author: hiroshige <hiroshige@chromium.org>
Date: Tue May 30 15:09:31 2017

Reland of remove EventSender from ImageLoader (patchset #1 id:1 of https://codereview.chromium.org/2903773002/ )

Reason for reland:
https://codereview.chromium.org/2905233002 will fix the leak, because
ImageLoader isn't kept alive after the context is destroyed.

Original issue's description:
> Revert of Remove EventSender from ImageLoader (patchset #10 id:180001 of https://codereview.chromium.org/2863763003/ )
>
> Reason for revert:
> This patch probably causes object leaks:
>
> See details in "webkit_tests" of
> https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty%20Leak/builds/4925
>
>
> Original issue's description:
> > Remove EventSender from ImageLoader
> >
> > Instead of using EventSender, this CL schedules ImageLoader's
> > load and error events using PostCancellableTask.
> > has_pending_load_event_ and has_pending_error_event_ booleans are
> > replaced with TaskHandle to the cancellable tasks and IsActive() calls.
> >
> > This CL removes the dependencies between Document's load event and
> > <img> load events via EventSender.
> >
> > This CL removes EventSender.h as ImageLoader was the last user of that.
> >
> > Test changes:
> > stop-loading.html:
> >   testharness.js waits for the window load event before finishing.
> >   Previously, we call window.stop() in <img> load event, but window
> >   load event is fired, because the <img> load event is fired from
> >   Document::ImplicitClose() via EventSender. At that time window.stop()
> >   doesn't stop the window load event.
> >   After this CL, <img> load event is fired separately from
> >   Document::ImplicitClose(), and thus window.stop() stops the window
> >   load event.
> >   To work around this, this CL triggers window load event explicitly
> >   just to signal testharness.js to finish.
> >
> > http/tests/loading/preload-image-sizes-2x.html and
> > http/tests/loading/preload-picture-sizes-2x.html:
> >   The tests uses srcset-helper.js that reloads the page with empty
> >   cache in the window load event.
> >   Because this CL changes the timing of the window load event, this
> >   causes some asynchronous tasks that cause new image loading to be
> >   scheduled before window load event and executed after that, causing
> >   some images left on the cache before reloading starts.
> >   This CL works around the issue by waiting a little bit for those
> >   tasks to be executed, and then clearing the cache and reloading the
> >   page.
> >
> > BUG= 624697 ,  720268 
> >
> > Review-Url: https://codereview.chromium.org/2863763003
> > Cr-Commit-Position: refs/heads/master@{#474086}
> > Committed: https://chromium.googlesource.com/chromium/src/+/da70e32d811fffe4e15728ba858dbdfa3180ed59
>
> TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chromium.org,tzik@chromium.org,hiroshige@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 624697 ,  720268 
>
> Review-Url: https://codereview.chromium.org/2903773002
> Cr-Commit-Position: refs/heads/master@{#474154}
> Committed: https://chromium.googlesource.com/chromium/src/+/8e7dc7edbcacdd495eb18ff33fe88d1fb89a15d5

BUG= 624697 ,  720268 ,  726091 

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

[modify] https://crrev.com/67e066b84ec7953ec06ecf252f466c2c2c063f33/third_party/WebKit/LayoutTests/http/tests/multipart/stop-loading.html
[modify] https://crrev.com/67e066b84ec7953ec06ecf252f466c2c2c063f33/third_party/WebKit/LayoutTests/http/tests/resources/srcset-helper.js
[modify] https://crrev.com/67e066b84ec7953ec06ecf252f466c2c2c063f33/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/67e066b84ec7953ec06ecf252f466c2c2c063f33/third_party/WebKit/Source/core/events/BUILD.gn
[delete] https://crrev.com/2a48181837832ca432f87369fddfa092f0450768/third_party/WebKit/Source/core/events/EventSender.h
[modify] https://crrev.com/67e066b84ec7953ec06ecf252f466c2c2c063f33/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/67e066b84ec7953ec06ecf252f466c2c2c063f33/third_party/WebKit/Source/core/loader/ImageLoader.h

Status: Fixed (was: Started)

Sign in to add a comment