|
|
DescriptionAdd 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
Patch Set 1 #Patch Set 2 : Refine assertions for ImageDocument #Patch Set 3 : bug fix #
Total comments: 2
Patch Set 4 : Rebase #Patch Set 5 : Rebase #Patch Set 6 : Rebase #Patch Set 7 : Rebase #
Total comments: 4
Patch Set 8 : Rebase #Patch Set 9 : Comment #Patch Set 10 : Place CHECK() in DispatchPendingErrorEvent() #Patch Set 11 : Rebase #Patch Set 12 : Rebase #Messages
Total messages: 68 (52 generated)
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...
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...
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 ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hiroshige@chromium.org changed reviewers: + japhet@chromium.org, kinuko@chromium.org, kouhei@chromium.org, yhirano@chromium.org
PTAL.
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
nit: "to be hold" in the description -> "to hold" (or to be held)
lgtm Would you add comments to the DCHECKs wherever possible?
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...
> 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.
The CQ bit was unchecked by hiroshige@chromium.org
The CQ bit was checked by hiroshige@chromium.org
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")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by hiroshige@chromium.org
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...
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...
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/
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 ==========
The CQ bit was checked by yhirano@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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?
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 ========== 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 ==========
> 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.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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...
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/.
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...
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.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
lgtm
lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hiroshige@chromium.org
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")
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": 220001, "attempt_start_ts": 1494627187736410, "parent_rev": "46858e064d251775e93c4f8fc7fe2da367ccaf36", "commit_rev": "1b070387595668ec438ffec65185e61017300e3c"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/1b070387595668ec438ffec65185...
Message was sent while issue was closed.
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. |