Fix leaks in https://codereview.chromium.org/2863763003 |
||
Issue descriptionhttps://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
,
May 26 2017
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.
,
May 26 2017
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/)
,
May 26 2017
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?
,
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
,
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
,
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
,
Jun 13 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by hirosh...@chromium.org
, May 25 2017