Add CHECK()s about ImageLoader::has_pending_load_event_

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

[hiroshige, 2017-05-05 00:37:02]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 00:37:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-05 00:39:25]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 00:39:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-05 00:43:59]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 00:44:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-05 01:26:59]

Description was changed from

==========
Add CHECK()s about ImageLoader::has_pending_load_event_

BUG=
==========

to

==========
Add CHECK()s about ImageLoader::has_pending_load_event_

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

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

BUG=624697
==========

[commit-bot: I haz the power, 2017-05-05 02:36:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 02:36:19]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-05-05 20:02:53]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org, kinuko@chromium.org, kouhei@chromium.org,
yhirano@chromium.org

[hiroshige, 2017-05-05 20:02:54]

PTAL.

[kinuko, 2017-05-08 05:34:34]

lgtm

Due-diligence: Will we make these DCHECK at some point?  Or are we sure these
are not perf-hot point?

https://codereview.chromium.org/2859383002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/2859383002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:503: }
nit: no braces for one-block line for consistency

[kinuko, 2017-05-08 05:36:30]

nit: "to be hold" in the description -> "to hold" (or to be held)

[kouhei (in TOK), 2017-05-08 13:15:59]

lgtm

Would you add comments to the DCHECKs wherever possible?

[hiroshige, 2017-05-08 18:53:48]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 18:54:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-08 18:56:42]

> Would you add comments to the DCHECKs wherever possible?
I tried but found the comments would be not more helpful than just trivially
transliterating the assertion code.

> Due-diligence: Will we make these DCHECK at some point?
> Or are we sure these are not perf-hot point?
I think at most 4 CHECK()s per one image loading is not so perf-hot.
But also I think these CHECK()s are not so critical (i.e. failure doesn't lead
to security issues), so I expect we can turn them to DCHECK() before going to
beta.
Added a comment.

https://codereview.chromium.org/2859383002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/2859383002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:503: }
On 2017/05/08 05:34:34, kinuko wrote:
> nit: no braces for one-block line for consistency

Done.

[hiroshige, 2017-05-08 18:56:55]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2017-05-08 18:56:59]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-05-08 18:56:59]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2859383002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-08 18:57:26]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-08 21:13:09]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2017-05-08 21:31:00]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 21:32:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-08 23:42:38]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 23:48:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-09 00:11:06]

I found the assertions are failing in some unit tests, due to
inconsistent flag settings in the unit tests.
This is fixed by [3] and now the unit tests should pass.

Previously this CL passes on trybot because
[1] introduces a bug that made the unit tests to pass unexpectedly.
This is fixed by [2].

[1] https://codereview.chromium.org/2746343002/
[2] https://codereview.chromium.org/2869733004/
[3] https://codereview.chromium.org/2864253003/

[hiroshige, 2017-05-09 00:11:40]

Description was changed from

==========
Add CHECK()s about ImageLoader::has_pending_load_event_

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

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

BUG=624697
==========

to

==========
Add CHECK()s about ImageLoader::has_pending_load_event_

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

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

BUG=624697, 719759
==========

[yhirano, 2017-05-09 08:35:21]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 08:35:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 12:01:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 12:01:20]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-05-10 06:44:04]

Please see #13.

https://codereview.chromium.org/2859383002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/2859383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:515:
CHECK(image_complete_);
Just to confirm: This is based on the fact that ImageLoader::UpdateFromElement
is called if and only if loading_image_document_ is false, right?

https://codereview.chromium.org/2859383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:651: if
(!has_pending_error_event_)
Should this be consistent with the corresponding statement in
DispatchPendingLoadEvent?

[hiroshige, 2017-05-10 16:56:39]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 16:57:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-10 17:05:00]

Description was changed from

==========
Add CHECK()s about ImageLoader::has_pending_load_event_

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

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

BUG=624697, 719759
==========

to

==========
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
==========

[hiroshige, 2017-05-10 17:05:44]

> nit: "to be hold" in the description -> "to hold" (or to be held)
> Please see #13.
Oh, thanks. done.

https://codereview.chromium.org/2859383002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/2859383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:515:
CHECK(image_complete_);
On 2017/05/10 06:44:04, yhirano wrote:
> Just to confirm: This is based on the fact that ImageLoader::UpdateFromElement
> is called if and only if loading_image_document_ is false, right?

Yes, and SetImage() sets image_ to non-null but image_complete_ to true.

Er, this might be not the case when ImageLoader::kUpdateForcedReload is used.
I'll check whether it is covered and write a test if not.

[commit-bot: I haz the power, 2017-05-10 19:38:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 19:38:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2017-05-10 21:47:39]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 21:48:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-10 22:37:18]

On 2017/05/10 17:05:44, hiroshige wrote:
> > nit: "to be hold" in the description -> "to hold" (or to be held)
> > Please see #13.
> Oh, thanks. done.
> 
>
https://codereview.chromium.org/2859383002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):
> 
>
https://codereview.chromium.org/2859383002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/loader/ImageLoader.cpp:515:
> CHECK(image_complete_);
> On 2017/05/10 06:44:04, yhirano wrote:
> > Just to confirm: This is based on the fact that
ImageLoader::UpdateFromElement
> > is called if and only if loading_image_document_ is false, right?
> 
> Yes, and SetImage() sets image_ to non-null but image_complete_ to true.
> 
> Er, this might be not the case when ImageLoader::kUpdateForcedReload is used.
> I'll check whether it is covered and write a test if not.

Actually, the assertion is failing and no tests were covering that case.
Adding a test by https://codereview.chromium.org/2874073003/, and
fixing the assertion by https://codereview.chromium.org/2877583002/.

[hiroshige, 2017-05-10 22:40:19]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 22:41:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-10 22:43:38]

Also added a short comment about the assertion.

https://codereview.chromium.org/2859383002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/2859383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:651: if
(!has_pending_error_event_)
On 2017/05/10 06:44:04, yhirano wrote:
> Should this be consistent with the corresponding statement in
> DispatchPendingLoadEvent?

Done, and trying on bots.

[commit-bot: I haz the power, 2017-05-11 00:43:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 00:43:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2017-05-11 16:06:32]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 16:07:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 18:05:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 18:05:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2017-05-11 18:19:08]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 18:19:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 20:27:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 20:27:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2017-05-12 10:39:23]

lgtm

[kouhei (in TOK), 2017-05-12 13:06:17]

lgtm

[hiroshige, 2017-05-12 17:19:33]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 17:20:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 19:42:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 19:42:58]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-05-12 22:13:07]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-05-12 22:13:08]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
yhirano@chromium.org, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2859383002/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-12 22:14:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 22:21:25]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1494627187736410,
"parent_rev": "46858e064d251775e93c4f8fc7fe2da367ccaf36", "commit_rev":
"1b070387595668ec438ffec65185e61017300e3c"}

[commit-bot: I haz the power, 2017-05-12 22:21:45]

Description was changed from

==========
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
==========

to

==========
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/+/1b070387595668ec438ffec65185...
==========

[commit-bot: I haz the power, 2017-05-12 22:21:46]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/1b070387595668ec438ffec65185...

[hiroshige, 2017-05-13 18:03:05]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2878263002/ by hiroshige@chromium.org.

The reason for reverting is: Assertion failures in the wild and on clusterfuzz.
BUG=721994.