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

Issue 720268 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Layout Test http/tests/loading/preload-picture-sizes-2x.html is flaky

Project Member Reported by yhirano@chromium.org, May 10 2017

Issue description

The following layout test is flaky

http/tests/loading/preload-picture-sizes-2x.html

Probable cause:

I suspect it got flaky due to my change: https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf2441e7f0686d53

The change will be reverted soon, so I expect the flakiness will recover.
 
Status: Fixed (was: Assigned)
Seems to be fixed. The last (flaky) failure was r470819.
The CL mentioned in #0 is reverted at r470825.
Project Member

Comment 2 by bugdroid1@chromium.org, May 23 2017

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

commit da70e32d811fffe4e15728ba858dbdfa3180ed59
Author: hiroshige <hiroshige@chromium.org>
Date: Tue May 23 22:31:28 2017

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}

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

Project Member

Comment 3 by bugdroid1@chromium.org, May 24 2017

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

commit 8e7dc7edbcacdd495eb18ff33fe88d1fb89a15d5
Author: yosin <yosin@chromium.org>
Date: Wed May 24 04:07:29 2017

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}

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

Owner: hirosh...@chromium.org
Status: Assigned (was: Fixed)
Sorry, I relanded the CL, and forgot about this issue.

It seems hiroshige@ has a fix: https://codereview.chromium.org/2905233002/
Cc: y...@yoav.ws
 Issue 727545  has been merged into this issue.
> #4
https://codereview.chromium.org/2906063003 might resolve the flakiness, but not sure.
> #6

Oops, thank you for the correction.
Project Member

Comment 8 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: Assigned)
Apparently fixed. Thank you!

Sign in to add a comment