Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2906063003:
Reland of remove EventSender from ImageLoader (Closed)
DescriptionReland 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}
Committed: https://chromium.googlesource.com/chromium/src/+/67e066b84ec7953ec06ecf252f466c2c2c063f33
Patch Set 1 #Patch Set 2 : Rebase with fix. #
Depends on Patchset: Messages
Total messages: 28 (17 generated)
Created Reland of move EventSender from ImageLoader
Description was changed from ========== Reland of move EventSender from ImageLoader (patchset #1 id:1 of https://codereview.chromium.org/2903773002/ ) Reason for revert: Trying reland with fixes. 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=624697, 720268 ========== to ========== Reland of move EventSender from ImageLoader (patchset #1 id:1 of https://codereview.chromium.org/2903773002/ ) Reason for revert: 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=624697, 720268 ==========
Patchset #3 (id:190001) has been deleted
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reland of move EventSender from ImageLoader (patchset #1 id:1 of https://codereview.chromium.org/2903773002/ ) Reason for revert: 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=624697, 720268 ========== to ========== Reland of move 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=624697, 720268 ==========
Description was changed from ========== Reland of move 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=624697, 720268 ========== to ========== Reland of move 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... BUG=624697, 720268, 726091 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/05/26 21:24:28, hiroshige wrote: > Created Reland of move EventSender from ImageLoader Is https://codereview.chromium.org/2905233002 ready for review?
On 2017/05/29 01:23:39, yhirano wrote: > On 2017/05/26 21:24:28, hiroshige wrote: > > Created Reland of move EventSender from ImageLoader > > Is https://codereview.chromium.org/2905233002 ready for review? Yes.
PTAL. This is a reland, fixing leaks by rebasing on https://codereview.chromium.org/2905233002. This CL itself doesn't contain non-trivial changes from the original.
yosin@chromium.org changed reviewers: - yosin@chromium.org
lgtm
lgtm
lgtm
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reland of move 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... BUG=624697, 720268, 726091 ========== to ========== Reland of move 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... BUG=624697, 720268, 726091, 720268 ==========
Description was changed from ========== Reland of move 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... BUG=624697, 720268, 726091, 720268 ========== to ========== Reland of move 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... BUG=624697, 720268, 726091 ==========
Description was changed from ========== Reland of move 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... BUG=624697, 720268, 726091 ========== to ========== 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... BUG=624697, 720268, 726091 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 170001, "attempt_start_ts": 1496154099705260,
"parent_rev": "2a48181837832ca432f87369fddfa092f0450768", "commit_rev":
"67e066b84ec7953ec06ecf252f466c2c2c063f33"}
Message was sent while issue was closed.
Description was changed from ========== 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... BUG=624697, 720268, 726091 ========== to ========== 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... > > > 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/+/da70e32d811fffe4e15728ba858d... > > TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chrom... > # 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/+/8e7dc7edbcacdd495eb18ff33fe8... BUG=624697, 720268, 726091 Review-Url: https://codereview.chromium.org/2906063003 Cr-Commit-Position: refs/heads/master@{#475528} Committed: https://chromium.googlesource.com/chromium/src/+/67e066b84ec7953ec06ecf252f46... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:170001) as https://chromium.googlesource.com/chromium/src/+/67e066b84ec7953ec06ecf252f46... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
