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

Issue 719759 link

Starred by 2 users

Issue metadata

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

Blocked on:
issue 722500

Blocking:
issue 624697



Sign in to add a comment

Clean up flags and assumptions of ImageLoader

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

Issue description

Currently the flags and image_ (and other states) are set/updated in somehow inconsistent/confusing ways.
- image_
- has_pending_load_event_
- image_complete_

This issue aims to make them cleaner and consnstent, to unblock refactoring and put stronger assertions.

 
Blocking: 624697
Project Member

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

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

commit d5af742666d2acde551ac33e82f3d8662ffa0f15
Author: hiroshige <hiroshige@chromium.org>
Date: Fri May 12 01:56:02 2017

Split ImageLoader::SetImage() and set flags consistently in tests

Previously, ImageLoader::SetImage() is used in three ways:
1. SetImage(nullptr),
2. SetImage(non-null) for ImageDocument, and
3. SetImage(non-null) for unit tests for non-ImageDocument.
and SetImage() sets has_pending_load_event_ = false and
image_complete_ = true for all of these.

This flag setting is consistent for 1., but not for 3., and causes
assertion failures when we apply stronger assertions in [1].

This CL fixes this by splitting SetImage() for separate methods:
1. ClearImage(),
2. SetImageForImageDocument(), and
3. SetImageForTest(),
and introducing UpdateImageState() that sets:
- image_
- has_pending_load_event_
- image_complete_

This CL
- Doesn't change non-test behavior, i.e. keeps the behavior for
  Cases 1 and 2.
  Particularly, this leaves the flag values that are apparently
  inconsistent but needed for the current implementation in Case 2 in
  SetImageForImageDocument().
- Changes the test-only behavior of Case 3, to set
    has_pending_load_event_ = true and
    image_complete_ = false in SetImageForTest().
  This doesn't affect the tested behavior but fixes the assertion
  failures in [1].

[1] https://codereview.chromium.org/2859383002

BUG= 624697 ,  719759 

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

[modify] https://crrev.com/d5af742666d2acde551ac33e82f3d8662ffa0f15/third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp
[modify] https://crrev.com/d5af742666d2acde551ac33e82f3d8662ffa0f15/third_party/WebKit/Source/core/html/HTMLImageElement.h
[modify] https://crrev.com/d5af742666d2acde551ac33e82f3d8662ffa0f15/third_party/WebKit/Source/core/html/HTMLObjectElement.cpp
[modify] https://crrev.com/d5af742666d2acde551ac33e82f3d8662ffa0f15/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/d5af742666d2acde551ac33e82f3d8662ffa0f15/third_party/WebKit/Source/core/loader/ImageLoader.h

Project Member

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

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

commit f33a3ba6de05eaed0115636c6dd552b9f8c80333
Author: hiroshige <hiroshige@chromium.org>
Date: Fri May 12 19:18:35 2017

Add layout tests for ImageLoader::UpdateFromElement(kUpdateForcedReload)

HTMLImageElement::ForceReload() and
ImageLoader::UpdateFromElement(kUpdateForcedReload) were introduced by
[1] but are not covered by any tests.
This CL adds layout tests for this code path.

This is also confirming the correctness of [2] and [3].

[1] https://codereview.chromium.org/1112513005/
[2] https://codereview.chromium.org/2877583002/
[3] https://codereview.chromium.org/2859383002/

BUG=485295,  624697 ,  719759 

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

[add] https://crrev.com/f33a3ba6de05eaed0115636c6dd552b9f8c80333/third_party/WebKit/LayoutTests/http/tests/cache/resources/random-cached-image.php
[add] https://crrev.com/f33a3ba6de05eaed0115636c6dd552b9f8c80333/third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html
[add] https://crrev.com/f33a3ba6de05eaed0115636c6dd552b9f8c80333/third_party/WebKit/LayoutTests/http/tests/images/force-reload.html
[modify] https://crrev.com/f33a3ba6de05eaed0115636c6dd552b9f8c80333/third_party/WebKit/Source/core/testing/Internals.cpp
[modify] https://crrev.com/f33a3ba6de05eaed0115636c6dd552b9f8c80333/third_party/WebKit/Source/core/testing/Internals.h
[modify] https://crrev.com/f33a3ba6de05eaed0115636c6dd552b9f8c80333/third_party/WebKit/Source/core/testing/Internals.idl

Project Member

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

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

commit 1b070387595668ec438ffec65185e61017300e3c
Author: hiroshige <hiroshige@chromium.org>
Date: Fri May 12 22:21:22 2017

Add CHECK()s about ImageLoader::has_pending_load_event_

As preparation for [1], this CL checks some assumptions to hold that
are helpful for proving the equivalences in [1].

[1] https://codereview.chromium.org/2859093003/

BUG= 624697 ,  719759 

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

[modify] https://crrev.com/1b070387595668ec438ffec65185e61017300e3c/third_party/WebKit/Source/core/loader/ImageLoader.cpp

Project Member

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

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

commit 9d5bcf58d9932d151f12231a6544b67903780fed
Author: hiroshige <hiroshige@chromium.org>
Date: Sat May 13 18:03:28 2017

Revert of Add CHECK()s about ImageLoader::has_pending_load_event_ (patchset #12 id:220001 of https://codereview.chromium.org/2859383002/ )

Reason for revert:
Assertion failures in the wild and on clusterfuzz.
BUG= 721994 

Original issue's description:
> Add CHECK()s about ImageLoader::has_pending_load_event_
>
> As preparation for [1], this CL checks some assumptions to hold that
> are helpful for proving the equivalences in [1].
>
> [1] https://codereview.chromium.org/2859093003/
>
> BUG= 624697 ,  719759 
>
> Review-Url: https://codereview.chromium.org/2859383002
> Cr-Commit-Position: refs/heads/master@{#471464}
> Committed: https://chromium.googlesource.com/chromium/src/+/1b070387595668ec438ffec65185e61017300e3c

TBR=japhet@chromium.org,yhirano@chromium.org,kinuko@chromium.org,kouhei@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 624697 ,  719759 

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

[modify] https://crrev.com/9d5bcf58d9932d151f12231a6544b67903780fed/third_party/WebKit/Source/core/loader/ImageLoader.cpp

Blockedon: 722500
Project Member

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

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

commit 1c5d3f11a80d88d65f5fae8398ed97a7b5ea1508
Author: hiroshige <hiroshige@chromium.org>
Date: Mon May 15 22:28:51 2017

Partial reland of "Add CHECK()s about ImageLoader::has_pending_load_event_"

Original CL:
https://codereview.chromium.org/2859383002/#ps220001

This CL relands the CHECK()s except for the failing one in
DispatchPendingErrorEvent().

 Issue 722500  tracks the issue around DispatchPendingErrorEvent() and relanding
of the remaining CHECK().

Original CL description:
> As preparation for [1], this CL checks some assumptions to hold that
> are helpful for proving the equivalences in [1].
>
> [1] https://codereview.chromium.org/2859093003/
>
> BUG= 624697 ,  719759 
>
> Review-Url: https://codereview.chromium.org/2859383002
> Cr-Commit-Position: refs/heads/master@{#471464}
> Committed: https://chromium.googlesource.com/chromium/src/+/1b070387595668ec438ffec65185e61017300e3c

BUG= 624697 ,  719759 ,  722500 

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

[modify] https://crrev.com/1c5d3f11a80d88d65f5fae8398ed97a7b5ea1508/third_party/WebKit/Source/core/loader/ImageLoader.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, May 19 2017

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

commit 84fb9e0a3e168b81e538a5ff8ce79e3690396a10
Author: hiroshige <hiroshige@chromium.org>
Date: Fri May 19 16:53:20 2017

Make ImageLoader have at most one pending error event on EventSender

Relanding the CHECK() that asserts
ImageLoader::DispatchPendingErrorEvent() should always called when
has_pending_error_event_ is true.

Previously, we sometimes schedule multiple error events on EventSender:
1. DispatchErrorEvent() is called.
2. DispatchErrorEvent() is called.
3. DispatchPendingErrorEvent() is called (corresponding to Step 1)
   and actual processing is done.
4. DispatchPendingErrorEvent() is called (corresponding to Step 2)
   but ignored because has_pending_error_event_ is already false.

This CL makes DispatchErrorEvent() cancel previous scheduled error
events if any, ensures there can be at most scheduled error event on
EventSender, and makes the assertion hold.

The scenario above will be processed as follows, and thus this CL
shouldn't change any observable behavior.
1. DispatchErrorEvent() is called.
2. DispatchErrorEvent() is called.
   The error event scheduled by Step 1 is cancelled.
3. DispatchPendingErrorEvent() is called (corresponding to Step 2)
   and actual processing is done.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2884773002/

BUG= 624697 ,  719759 ,  722500 

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

[add] https://crrev.com/84fb9e0a3e168b81e538a5ff8ce79e3690396a10/third_party/WebKit/LayoutTests/images/multiple-inflight-error-event-crash.html
[modify] https://crrev.com/84fb9e0a3e168b81e538a5ff8ce79e3690396a10/third_party/WebKit/Source/core/loader/ImageLoader.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, May 22 2017

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

commit 2e6f72c80e0106f348fd9437536edc2e9f3b7acc
Author: hiroshige <hiroshige@chromium.org>
Date: Mon May 22 20:49:24 2017

Change the semantics of ImageLoader::has_pending_load_event_

ImageLoader works like:
(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3), i.e.
only while there is a pending task on EventSender.

Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".
This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by [1].

This is preparation for [2] that replaces has_pending_load_event_ with
a reference to a task that corresponds to the pending load event.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2863763003/

BUG= 624697 ,  719759 

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

[modify] https://crrev.com/2e6f72c80e0106f348fd9437536edc2e9f3b7acc/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/2e6f72c80e0106f348fd9437536edc2e9f3b7acc/third_party/WebKit/Source/core/loader/ImageLoader.h

Status: Fixed (was: Started)

Sign in to add a comment