Phase II Step 1: Remove updateImage() reentrancy around decodeError()

Phase II Step 1: Remove updateImage() reentrancy around decodeError()

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit#heading=h.1rxkos5samv7

Previously, ImageResourceContent::updateImage() is reentered when
DecodeError occurred in ImageResource.

This CL removes the reentrancy and ImageResourceInfo::decodeError() by
1. making ImageResourceContent::updateImage() to return
   ShouldDecodeError without calling notifyObservers()
   when an decode error occurs,
2. then making ImageResource::updateImage() to call
   ImageResource::decodeError(), and 
3. then making ImageResource::decodeError() to call notifyObservers()
   (via ImageResourceContent::updateImage()) if necessary.

This CL also adds DCHECK() for prohibiting reentering to
ImageResourceContent::updateImage().

In the case of a decode error inside didReceiveData(),
this CL removes one imageChanged() call, which looks unnecessary.

BUG=667641, 677188
Review-Url: https://codereview.chromium.org/2642823005
Cr-Commit-Position: refs/heads/master@{#451996}
Committed: https://chromium.googlesource.com/chromium/src/+/bd8c80c437ceaa8427186bbbbd3040ae17d6d13d

[hiroshige, 2017-01-19 01:47:19]

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

[commit-bot: I haz the power, 2017-01-19 01:47:49]

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-01-19 03:07:02]

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

[commit-bot: I haz the power, 2017-01-19 03:07:03]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hiroshige, 2017-01-19 21:11:48]

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

[commit-bot: I haz the power, 2017-01-19 21:12: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-01-19 23:34:39]

Description was changed from

==========
Remove ImageResource::updateImage() reentrancy around decodeError()

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

to

==========
Remove ImageResource::updateImage() reentrancy around decodeError()

BUG=677188
==========

[hiroshige, 2017-01-19 23:35:41]

Description was changed from

==========
Remove ImageResource::updateImage() reentrancy around decodeError()

BUG=677188
==========

to

==========
Remove ImageResource::updateImage() reentrancy around decodeError()

BUG=667641, 677188
==========

[commit-bot: I haz the power, 2017-01-20 00:44:52]

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

[commit-bot: I haz the power, 2017-01-20 00:44:53]

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-01-20 19:12:15]

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

[commit-bot: I haz the power, 2017-01-20 19:12:43]

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-01-20 20:31:38]

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

[commit-bot: I haz the power, 2017-01-20 20:31:39]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-01-20 22:25:25]

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

[commit-bot: I haz the power, 2017-01-20 22:26:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-01-20 23:03:17]

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

[commit-bot: I haz the power, 2017-01-20 23:03:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-01-21 00:11:49]

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

[commit-bot: I haz the power, 2017-01-21 00:12:11]

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-01-21 01:36:56]

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

[commit-bot: I haz the power, 2017-01-21 01:36:57]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-01-21 01:38:35]

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

[commit-bot: I haz the power, 2017-01-21 01:38:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-01-21 02:30:10]

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

[commit-bot: I haz the power, 2017-01-21 02:30:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-01-21 02:39:31]

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

[commit-bot: I haz the power, 2017-01-21 02:39:40]

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-01-21 04:26:10]

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

[commit-bot: I haz the power, 2017-01-21 04:26:10]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-01-24 22:46:21]

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

[commit-bot: I haz the power, 2017-01-24 22:47:11]

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-01-24 23:40:29]

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

[commit-bot: I haz the power, 2017-01-24 23:40:30]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hiroshige, 2017-01-24 23:46:50]

Description was changed from

==========
Remove ImageResource::updateImage() reentrancy around decodeError()

BUG=667641, 677188
==========

to

==========
Phase II Step 1: Remove updateImage() reentrancy around decodeError()

BUG=667641, 677188
==========

[hiroshige, 2017-01-25 01:39:06]

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

[commit-bot: I haz the power, 2017-01-25 01:39:33]

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-01-25 01:44:26]

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

[commit-bot: I haz the power, 2017-01-25 01:44:27]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hiroshige, 2017-01-25 02:21:06]

Description was changed from

==========
Phase II Step 1: Remove updateImage() reentrancy around decodeError()

BUG=667641, 677188
==========

to

==========
Phase II Step 1: Remove updateImage() reentrancy around decodeError()

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

Previously, ImageResourceContent::updateImage() is reentered when
DecodeError occurred in ImageResource.
This CL removes the reentrancy by
1. making ImageResourceContent::updateImage() to return
   ShouldDecodeError without calling notifyObservers()
   when an decode error occurs,
2. then making ImageResource::updateImage() to call
   ImageResource::decodeError(), and 
3. then making ImageResource::decodeError() to call notifyObservers()
   (via ImageResourceContent::updateImage()) if necessary.

This CL also adds DCHECK() for prohibiting reentering to
ImageResourceContent::updateImage().

In the case of a decode error inside didReceiveData(),
this CL removes one imageChanged() call, which looks unnecessary.

BUG=667641, 677188
==========

[hiroshige, 2017-01-25 02:23:19]

Description was changed from

==========
Phase II Step 1: Remove updateImage() reentrancy around decodeError()

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

Previously, ImageResourceContent::updateImage() is reentered when
DecodeError occurred in ImageResource.
This CL removes the reentrancy by
1. making ImageResourceContent::updateImage() to return
   ShouldDecodeError without calling notifyObservers()
   when an decode error occurs,
2. then making ImageResource::updateImage() to call
   ImageResource::decodeError(), and 
3. then making ImageResource::decodeError() to call notifyObservers()
   (via ImageResourceContent::updateImage()) if necessary.

This CL also adds DCHECK() for prohibiting reentering to
ImageResourceContent::updateImage().

In the case of a decode error inside didReceiveData(),
this CL removes one imageChanged() call, which looks unnecessary.

BUG=667641, 677188
==========

to

==========
Phase II Step 1: Remove updateImage() reentrancy around decodeError()

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

Previously, ImageResourceContent::updateImage() is reentered when
DecodeError occurred in ImageResource.

This CL removes the reentrancy and ImageResourceInfo::decodeError() by
1. making ImageResourceContent::updateImage() to return
   ShouldDecodeError without calling notifyObservers()
   when an decode error occurs,
2. then making ImageResource::updateImage() to call
   ImageResource::decodeError(), and 
3. then making ImageResource::decodeError() to call notifyObservers()
   (via ImageResourceContent::updateImage()) if necessary.

This CL also adds DCHECK() for prohibiting reentering to
ImageResourceContent::updateImage().

In the case of a decode error inside didReceiveData(),
this CL removes one imageChanged() call, which looks unnecessary.

BUG=667641, 677188
==========

[hiroshige, 2017-01-25 02:31:01]

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

[commit-bot: I haz the power, 2017-01-25 02:31: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-01-25 02:49:14]

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

[hiroshige, 2017-01-25 02:49:16]

PTAL.

https://codereview.chromium.org/2642823005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
(right):

https://codereview.chromium.org/2642823005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp:925:
EXPECT_EQ(1, observer->imageChangedCount());
As mentioned in the design doc and CL description, the number of imageChanged()
is decreased by one, which probably looks safe.

https://codereview.chromium.org/2642823005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp:1135:
EXPECT_EQ(2, observer->imageChangedCount());
ditto.

[commit-bot: I haz the power, 2017-01-25 05:21:33]

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

[commit-bot: I haz the power, 2017-01-25 05:21:34]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-01-25 09:50:04]

> (in the design doc) Also, we also refactor DecodeError handling because
> there are timing dependencies between DecodeError handling and
LoFi/Placeholder
> reloading.

Can you explain this a bit (either in the doc or here)?
The reentrancy exists because updateImage handles three UpdateImageOptions; As
it doesn't call decodeError with ClearAndNotifyObservers, splitting the function
vanishes the reentrancy, right? If so, is it an option?

[hiroshige, 2017-01-26 01:14:23]

On 2017/01/25 09:50:04, yhirano wrote:
> > (in the design doc) Also, we also refactor DecodeError handling because
> > there are timing dependencies between DecodeError handling and
> LoFi/Placeholder
> > reloading.
> 
> Can you explain this a bit (either in the doc or here)?
> The reentrancy exists because updateImage handles three UpdateImageOptions; As
> it doesn't call decodeError with ClearAndNotifyObservers, splitting the
function
> vanishes the reentrancy, right?
No. updateImage(UpdateImage) is reentered.
Particularly,
- The first updateImage(UpdateImage, false) causes DecodeError, and calls
decodeError(). The Image::setData() call at Line 364 is needed here.
- The second (reentered) updateImage(UpdateImage, true) handles Placeholder
processing (Lines 373--387 in the current implementation).
Both of them (Line 364 and Lines 373--387) are done in (ClearAnd)UpdateImage
options, so splitting ClearAndNotifyObservers out as a different function
doesn't resolve the reentrancy.

Also, this CL removes a call from ImageResourceContent to ImageResource (via
ImageResourceInfo::decodeError()), i.e. removes a Content->Resource->Content
call chain.
(I'd like to make things flow in Resource->Content direction, not
Content->Resource (via ImageResourceInfo))
If we just split updateImage() into two parts, we'll still have
Content->Resource->Content call chain here (although direct reentrance to the
same method is removed).

[yhirano, 2017-01-27 10:16:19]

Thanks, lgtm.

[hiroshige, 2017-01-27 22:36:25]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-01-27 22:37:31]

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

[commit-bot: I haz the power, 2017-01-27 22:37:33]

This CL has an open dependency (Issue 2650543003 Patch 120001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[hiroshige, 2017-02-11 00:02:02]

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

[commit-bot: I haz the power, 2017-02-11 00:03:01]

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-02-11 00:05:27]

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

[commit-bot: I haz the power, 2017-02-11 00:05:28]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[hiroshige, 2017-02-11 00:05:58]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2017-02-11 00:06:35]

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

[commit-bot: I haz the power, 2017-02-11 00:06:55]

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-02-11 01:47:24]

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

[commit-bot: I haz the power, 2017-02-11 01:47:25]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-02-22 07:33:05]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-02-22 07:33:06]

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

[commit-bot: I haz the power, 2017-02-22 07:34:22]

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-02-22 11:22:07]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1487748785312520,
"parent_rev": "2c5c012360a9213b6895ce6c79fa62f941c2bd20", "commit_rev":
"bd8c80c437ceaa8427186bbbbd3040ae17d6d13d"}

[commit-bot: I haz the power, 2017-02-22 11:22:43]

Description was changed from

==========
Phase II Step 1: Remove updateImage() reentrancy around decodeError()

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

Previously, ImageResourceContent::updateImage() is reentered when
DecodeError occurred in ImageResource.

This CL removes the reentrancy and ImageResourceInfo::decodeError() by
1. making ImageResourceContent::updateImage() to return
   ShouldDecodeError without calling notifyObservers()
   when an decode error occurs,
2. then making ImageResource::updateImage() to call
   ImageResource::decodeError(), and 
3. then making ImageResource::decodeError() to call notifyObservers()
   (via ImageResourceContent::updateImage()) if necessary.

This CL also adds DCHECK() for prohibiting reentering to
ImageResourceContent::updateImage().

In the case of a decode error inside didReceiveData(),
this CL removes one imageChanged() call, which looks unnecessary.

BUG=667641, 677188
==========

to

==========
Phase II Step 1: Remove updateImage() reentrancy around decodeError()

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

Previously, ImageResourceContent::updateImage() is reentered when
DecodeError occurred in ImageResource.

This CL removes the reentrancy and ImageResourceInfo::decodeError() by
1. making ImageResourceContent::updateImage() to return
   ShouldDecodeError without calling notifyObservers()
   when an decode error occurs,
2. then making ImageResource::updateImage() to call
   ImageResource::decodeError(), and 
3. then making ImageResource::decodeError() to call notifyObservers()
   (via ImageResourceContent::updateImage()) if necessary.

This CL also adds DCHECK() for prohibiting reentering to
ImageResourceContent::updateImage().

In the case of a decode error inside didReceiveData(),
this CL removes one imageChanged() call, which looks unnecessary.

BUG=667641, 677188

Review-Url: https://codereview.chromium.org/2642823005
Cr-Commit-Position: refs/heads/master@{#451996}
Committed:
https://chromium.googlesource.com/chromium/src/+/bd8c80c437ceaa8427186bbbbd30...
==========

[commit-bot: I haz the power, 2017-02-22 11:22:45]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/bd8c80c437ceaa8427186bbbbd30...